| Message ID | 20210326142946.5263-4-ciara.loftus@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | AF_XDP Socket Creation Fixes | expand |
On Fri, Mar 26, 2021 at 02:29:46PM +0000, Ciara Loftus wrote: > During xsk_socket__create the XDP_RX_RING and XDP_TX_RING setsockopts > are called to create the rx and tx rings for the AF_XDP socket. If the ring > has already been set up, the setsockopt will return an error. However, > in the event of a failure during xsk_socket__create(_shared) after the > rings have been set up, the user may wish to retry the socket creation > using these pre-existing rings. In this case we can ignore the error > returned by the setsockopts. If there is a true error, the subsequent > call to mmap() will catch it. > > Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets") > > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > --- > tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ > 1 file changed, 16 insertions(+), 18 deletions(-) > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index d4991ddff05a..cfc4abf505c3 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, > } > xsk->ctx = ctx; > > - if (rx) { > - err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, > - &xsk->config.rx_size, > - sizeof(xsk->config.rx_size)); > - if (err) { > - err = -errno; > - goto out_put_ctx; > - } > - } > - if (tx) { > - err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, > - &xsk->config.tx_size, > - sizeof(xsk->config.tx_size)); > - if (err) { > - err = -errno; > - goto out_put_ctx; > - } > - } > + /* The return values of these setsockopt calls are intentionally not checked. > + * If the ring has already been set up setsockopt will return an error. However, > + * this scenario is acceptable as the user may be retrying the socket creation > + * with rings which were set up in a previous but ultimately unsuccessful call > + * to xsk_socket__create(_shared). The call later to mmap() will fail if there > + * is a real issue and we handle that return value appropriately there. > + */ > + if (rx) > + setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, > + &xsk->config.rx_size, > + sizeof(xsk->config.rx_size)); > + > + if (tx) > + setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, > + &xsk->config.tx_size, > + sizeof(xsk->config.tx_size)); Instead of ignoring the error can you remember that setsockopt was done in struct xsk_socket and don't do it the second time?
> > On Fri, Mar 26, 2021 at 02:29:46PM +0000, Ciara Loftus wrote: > > During xsk_socket__create the XDP_RX_RING and XDP_TX_RING > setsockopts > > are called to create the rx and tx rings for the AF_XDP socket. If the ring > > has already been set up, the setsockopt will return an error. However, > > in the event of a failure during xsk_socket__create(_shared) after the > > rings have been set up, the user may wish to retry the socket creation > > using these pre-existing rings. In this case we can ignore the error > > returned by the setsockopts. If there is a true error, the subsequent > > call to mmap() will catch it. > > > > Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets") > > > > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > --- > > tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ > > 1 file changed, 16 insertions(+), 18 deletions(-) > > > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > > index d4991ddff05a..cfc4abf505c3 100644 > > --- a/tools/lib/bpf/xsk.c > > +++ b/tools/lib/bpf/xsk.c > > @@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct xsk_socket > **xsk_ptr, > > } > > xsk->ctx = ctx; > > > > - if (rx) { > > - err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, > > - &xsk->config.rx_size, > > - sizeof(xsk->config.rx_size)); > > - if (err) { > > - err = -errno; > > - goto out_put_ctx; > > - } > > - } > > - if (tx) { > > - err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, > > - &xsk->config.tx_size, > > - sizeof(xsk->config.tx_size)); > > - if (err) { > > - err = -errno; > > - goto out_put_ctx; > > - } > > - } > > + /* The return values of these setsockopt calls are intentionally not > checked. > > + * If the ring has already been set up setsockopt will return an error. > However, > > + * this scenario is acceptable as the user may be retrying the socket > creation > > + * with rings which were set up in a previous but ultimately > unsuccessful call > > + * to xsk_socket__create(_shared). The call later to mmap() will fail if > there > > + * is a real issue and we handle that return value appropriately there. > > + */ > > + if (rx) > > + setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, > > + &xsk->config.rx_size, > > + sizeof(xsk->config.rx_size)); > > + > > + if (tx) > > + setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, > > + &xsk->config.tx_size, > > + sizeof(xsk->config.tx_size)); > > Instead of ignoring the error can you remember that setsockopt was done > in struct xsk_socket and don't do it the second time? Ideally we don't have to ignore the error. However in the event of failure struct xsk_socket is freed at the end of xsk_socket__create so we can't use it to remember state between subsequent calls to __create().
On Mon, Mar 29, 2021 at 1:41 AM Loftus, Ciara <ciara.loftus@intel.com> wrote: > > > > > On Fri, Mar 26, 2021 at 02:29:46PM +0000, Ciara Loftus wrote: > > > During xsk_socket__create the XDP_RX_RING and XDP_TX_RING > > setsockopts > > > are called to create the rx and tx rings for the AF_XDP socket. If the ring > > > has already been set up, the setsockopt will return an error. However, > > > in the event of a failure during xsk_socket__create(_shared) after the > > > rings have been set up, the user may wish to retry the socket creation > > > using these pre-existing rings. In this case we can ignore the error > > > returned by the setsockopts. If there is a true error, the subsequent > > > call to mmap() will catch it. > > > > > > Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets") > > > > > > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > > --- > > > tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ > > > 1 file changed, 16 insertions(+), 18 deletions(-) > > > > > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > > > index d4991ddff05a..cfc4abf505c3 100644 > > > --- a/tools/lib/bpf/xsk.c > > > +++ b/tools/lib/bpf/xsk.c > > > @@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct xsk_socket > > **xsk_ptr, > > > } > > > xsk->ctx = ctx; > > > > > > - if (rx) { > > > - err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, > > > - &xsk->config.rx_size, > > > - sizeof(xsk->config.rx_size)); > > > - if (err) { > > > - err = -errno; > > > - goto out_put_ctx; > > > - } > > > - } > > > - if (tx) { > > > - err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, > > > - &xsk->config.tx_size, > > > - sizeof(xsk->config.tx_size)); > > > - if (err) { > > > - err = -errno; > > > - goto out_put_ctx; > > > - } > > > - } > > > + /* The return values of these setsockopt calls are intentionally not > > checked. > > > + * If the ring has already been set up setsockopt will return an error. > > However, > > > + * this scenario is acceptable as the user may be retrying the socket > > creation > > > + * with rings which were set up in a previous but ultimately > > unsuccessful call > > > + * to xsk_socket__create(_shared). The call later to mmap() will fail if > > there > > > + * is a real issue and we handle that return value appropriately there. > > > + */ > > > + if (rx) > > > + setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, > > > + &xsk->config.rx_size, > > > + sizeof(xsk->config.rx_size)); > > > + > > > + if (tx) > > > + setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, > > > + &xsk->config.tx_size, > > > + sizeof(xsk->config.tx_size)); > > > > Instead of ignoring the error can you remember that setsockopt was done > > in struct xsk_socket and don't do it the second time? > > Ideally we don't have to ignore the error. However in the event of failure struct xsk_socket is freed at the end of xsk_socket__create so we can't use it to remember state between subsequent calls to __create(). but umem is not, right? and fd is taken from there.
> > > > > > > > On Fri, Mar 26, 2021 at 02:29:46PM +0000, Ciara Loftus wrote: > > > > During xsk_socket__create the XDP_RX_RING and XDP_TX_RING > > > setsockopts > > > > are called to create the rx and tx rings for the AF_XDP socket. If the ring > > > > has already been set up, the setsockopt will return an error. However, > > > > in the event of a failure during xsk_socket__create(_shared) after the > > > > rings have been set up, the user may wish to retry the socket creation > > > > using these pre-existing rings. In this case we can ignore the error > > > > returned by the setsockopts. If there is a true error, the subsequent > > > > call to mmap() will catch it. > > > > > > > > Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets") > > > > > > > > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > > > > Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> > > > > --- > > > > tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ > > > > 1 file changed, 16 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > > > > index d4991ddff05a..cfc4abf505c3 100644 > > > > --- a/tools/lib/bpf/xsk.c > > > > +++ b/tools/lib/bpf/xsk.c > > > > @@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct > xsk_socket > > > **xsk_ptr, > > > > } > > > > xsk->ctx = ctx; > > > > > > > > - if (rx) { > > > > - err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, > > > > - &xsk->config.rx_size, > > > > - sizeof(xsk->config.rx_size)); > > > > - if (err) { > > > > - err = -errno; > > > > - goto out_put_ctx; > > > > - } > > > > - } > > > > - if (tx) { > > > > - err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, > > > > - &xsk->config.tx_size, > > > > - sizeof(xsk->config.tx_size)); > > > > - if (err) { > > > > - err = -errno; > > > > - goto out_put_ctx; > > > > - } > > > > - } > > > > + /* The return values of these setsockopt calls are intentionally not > > > checked. > > > > + * If the ring has already been set up setsockopt will return an error. > > > However, > > > > + * this scenario is acceptable as the user may be retrying the socket > > > creation > > > > + * with rings which were set up in a previous but ultimately > > > unsuccessful call > > > > + * to xsk_socket__create(_shared). The call later to mmap() will fail if > > > there > > > > + * is a real issue and we handle that return value appropriately there. > > > > + */ > > > > + if (rx) > > > > + setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, > > > > + &xsk->config.rx_size, > > > > + sizeof(xsk->config.rx_size)); > > > > + > > > > + if (tx) > > > > + setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, > > > > + &xsk->config.tx_size, > > > > + sizeof(xsk->config.tx_size)); > > > > > > Instead of ignoring the error can you remember that setsockopt was > done > > > in struct xsk_socket and don't do it the second time? > > > > Ideally we don't have to ignore the error. However in the event of failure > struct xsk_socket is freed at the end of xsk_socket__create so we can't use it > to remember state between subsequent calls to __create(). > > but umem is not, right? and fd is taken from there. Yes, got it. We can add a new field to struct xsk_umem. It's much better than ignoring the return values. I'll add this in the v3. Thanks for your suggestion! Ciara
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c index d4991ddff05a..cfc4abf505c3 100644 --- a/tools/lib/bpf/xsk.c +++ b/tools/lib/bpf/xsk.c @@ -900,24 +900,22 @@ int xsk_socket__create_shared(struct xsk_socket **xsk_ptr, } xsk->ctx = ctx; - if (rx) { - err = setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, - &xsk->config.rx_size, - sizeof(xsk->config.rx_size)); - if (err) { - err = -errno; - goto out_put_ctx; - } - } - if (tx) { - err = setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, - &xsk->config.tx_size, - sizeof(xsk->config.tx_size)); - if (err) { - err = -errno; - goto out_put_ctx; - } - } + /* The return values of these setsockopt calls are intentionally not checked. + * If the ring has already been set up setsockopt will return an error. However, + * this scenario is acceptable as the user may be retrying the socket creation + * with rings which were set up in a previous but ultimately unsuccessful call + * to xsk_socket__create(_shared). The call later to mmap() will fail if there + * is a real issue and we handle that return value appropriately there. + */ + if (rx) + setsockopt(xsk->fd, SOL_XDP, XDP_RX_RING, + &xsk->config.rx_size, + sizeof(xsk->config.rx_size)); + + if (tx) + setsockopt(xsk->fd, SOL_XDP, XDP_TX_RING, + &xsk->config.tx_size, + sizeof(xsk->config.tx_size)); err = xsk_get_mmap_offsets(xsk->fd, &off); if (err) {
During xsk_socket__create the XDP_RX_RING and XDP_TX_RING setsockopts are called to create the rx and tx rings for the AF_XDP socket. If the ring has already been set up, the setsockopt will return an error. However, in the event of a failure during xsk_socket__create(_shared) after the rings have been set up, the user may wish to retry the socket creation using these pre-existing rings. In this case we can ignore the error returned by the setsockopts. If there is a true error, the subsequent call to mmap() will catch it. Fixes: 1cad07884239 ("libbpf: add support for using AF_XDP sockets") Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> Signed-off-by: Ciara Loftus <ciara.loftus@intel.com> --- tools/lib/bpf/xsk.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)