| Message ID | 20220405110631.404427-6-maciej.fijalkowski@intel.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | xsk: stop softirq processing on full XSK Rx queue | expand |
On 05/04/2022 13.06, Maciej Fijalkowski wrote: > Correlate -ENOBUFS that was returned from xdp_do_redirect() with a XSK > Rx queue being full. In such case, terminate the softirq processing and > let the user space to consume descriptors from XSK Rx queue so that > there is room that driver can use later on. > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > --- > .../ethernet/intel/ixgbe/ixgbe_txrx_common.h | 1 + > drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 23 ++++++++++++------- > 2 files changed, 16 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h > index bba3feaf3318..f1f69ce67420 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h > @@ -8,6 +8,7 @@ > #define IXGBE_XDP_CONSUMED BIT(0) > #define IXGBE_XDP_TX BIT(1) > #define IXGBE_XDP_REDIR BIT(2) > +#define IXGBE_XDP_EXIT BIT(3) > > #define IXGBE_TXD_CMD (IXGBE_TXD_CMD_EOP | \ > IXGBE_TXD_CMD_RS) > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c > index dd7ff66d422f..475244a2c6e4 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c > @@ -109,9 +109,10 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter, > > if (likely(act == XDP_REDIRECT)) { > err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog); > - if (err) > - goto out_failure; > - return IXGBE_XDP_REDIR; > + if (!err) > + return IXGBE_XDP_REDIR; > + result = (err == -ENOBUFS) ? IXGBE_XDP_EXIT : IXGBE_XDP_CONSUMED; > + goto out_failure; > } > > switch (act) { > @@ -130,6 +131,9 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter, > if (result == IXGBE_XDP_CONSUMED) > goto out_failure; > break; > + case XDP_DROP: > + result = IXGBE_XDP_CONSUMED; > + break; > default: > bpf_warn_invalid_xdp_action(rx_ring->netdev, xdp_prog, act); > fallthrough; > @@ -137,9 +141,6 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter, > out_failure: > trace_xdp_exception(rx_ring->netdev, xdp_prog, act); > fallthrough; /* handle aborts by dropping packet */ > - case XDP_DROP: > - result = IXGBE_XDP_CONSUMED; > - break; > } > return result; > } > @@ -304,10 +305,16 @@ int ixgbe_clean_rx_irq_zc(struct ixgbe_q_vector *q_vector, > xdp_res = ixgbe_run_xdp_zc(adapter, rx_ring, bi->xdp); > > if (xdp_res) { > - if (xdp_res & (IXGBE_XDP_TX | IXGBE_XDP_REDIR)) > + if (xdp_res == IXGBE_XDP_EXIT) { Micro optimization note: Having this as the first if()-statement defaults the compiler to think this is the likely() case. (But compilers can obviously be smarter and can easily choose that the IXGBE_XDP_REDIR branch is so simple that it takes it as the likely case). Just wanted to mention this, given this is fash-path code. > + failure = true; > + xsk_buff_free(bi->xdp); > + ixgbe_inc_ntc(rx_ring); > + break; I was wondering if we have a situation where we should set xdp_xmit bit to trigger the call to xdp_do_flush_map later in function, but I assume you have this covered. > + } else if (xdp_res & (IXGBE_XDP_TX | IXGBE_XDP_REDIR)) { > xdp_xmit |= xdp_res; > - else > + } else { > xsk_buff_free(bi->xdp); > + } > > bi->xdp = NULL; > total_rx_packets++;
On Tue, Apr 05, 2022 at 02:36:41PM +0200, Jesper Dangaard Brouer wrote: > > On 05/04/2022 13.06, Maciej Fijalkowski wrote: > > Correlate -ENOBUFS that was returned from xdp_do_redirect() with a XSK > > Rx queue being full. In such case, terminate the softirq processing and > > let the user space to consume descriptors from XSK Rx queue so that > > there is room that driver can use later on. > > > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > --- > > .../ethernet/intel/ixgbe/ixgbe_txrx_common.h | 1 + > > drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 23 ++++++++++++------- > > 2 files changed, 16 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h > > index bba3feaf3318..f1f69ce67420 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h > > @@ -8,6 +8,7 @@ > > #define IXGBE_XDP_CONSUMED BIT(0) > > #define IXGBE_XDP_TX BIT(1) > > #define IXGBE_XDP_REDIR BIT(2) > > +#define IXGBE_XDP_EXIT BIT(3) > > #define IXGBE_TXD_CMD (IXGBE_TXD_CMD_EOP | \ > > IXGBE_TXD_CMD_RS) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c > > index dd7ff66d422f..475244a2c6e4 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c > > @@ -109,9 +109,10 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter, > > if (likely(act == XDP_REDIRECT)) { > > err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog); > > - if (err) > > - goto out_failure; > > - return IXGBE_XDP_REDIR; > > + if (!err) > > + return IXGBE_XDP_REDIR; > > + result = (err == -ENOBUFS) ? IXGBE_XDP_EXIT : IXGBE_XDP_CONSUMED; > > + goto out_failure; > > } > > switch (act) { > > @@ -130,6 +131,9 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter, > > if (result == IXGBE_XDP_CONSUMED) > > goto out_failure; > > break; > > + case XDP_DROP: > > + result = IXGBE_XDP_CONSUMED; > > + break; > > default: > > bpf_warn_invalid_xdp_action(rx_ring->netdev, xdp_prog, act); > > fallthrough; > > @@ -137,9 +141,6 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter, > > out_failure: > > trace_xdp_exception(rx_ring->netdev, xdp_prog, act); > > fallthrough; /* handle aborts by dropping packet */ > > - case XDP_DROP: > > - result = IXGBE_XDP_CONSUMED; > > - break; > > } > > return result; > > } > > @@ -304,10 +305,16 @@ int ixgbe_clean_rx_irq_zc(struct ixgbe_q_vector *q_vector, > > xdp_res = ixgbe_run_xdp_zc(adapter, rx_ring, bi->xdp); > > if (xdp_res) { > > - if (xdp_res & (IXGBE_XDP_TX | IXGBE_XDP_REDIR)) > > + if (xdp_res == IXGBE_XDP_EXIT) { > > Micro optimization note: Having this as the first if()-statement > defaults the compiler to think this is the likely() case. (But compilers > can obviously be smarter and can easily choose that the IXGBE_XDP_REDIR > branch is so simple that it takes it as the likely case). > Just wanted to mention this, given this is fash-path code. Good point. Since we're 'likely-fying' redirect path in ixgbe_run_xdp_zc(), maybe it would make sense to make the branch that does xdp_res & IXGBE_XDP_REDIR check as the likely() one. > > > + failure = true; > > + xsk_buff_free(bi->xdp); > > + ixgbe_inc_ntc(rx_ring); > > + break; > > I was wondering if we have a situation where we should set xdp_xmit bit > to trigger the call to xdp_do_flush_map later in function, but I assume > you have this covered. For every previous successful redirect xdp_xmit will be set with corresponding bits that came out of ixgbe_run_xdp_zc(), so if we got to the point of full XSK Rx queue, xdp_do_flush_map() will be called eventually. Before doing so, idea is to give the current buffer back to the XSK buffer pool and increment the "next_to_clean" which acts as the head pointer on HW Rx ring. IOW, consume the current buffer/descriptor and yield the CPU to the user space. > > > + } else if (xdp_res & (IXGBE_XDP_TX | IXGBE_XDP_REDIR)) { > > xdp_xmit |= xdp_res; > > - else > > + } else { > > xsk_buff_free(bi->xdp); > > + } > > bi->xdp = NULL; > > total_rx_packets++; >
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h index bba3feaf3318..f1f69ce67420 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_txrx_common.h @@ -8,6 +8,7 @@ #define IXGBE_XDP_CONSUMED BIT(0) #define IXGBE_XDP_TX BIT(1) #define IXGBE_XDP_REDIR BIT(2) +#define IXGBE_XDP_EXIT BIT(3) #define IXGBE_TXD_CMD (IXGBE_TXD_CMD_EOP | \ IXGBE_TXD_CMD_RS) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c index dd7ff66d422f..475244a2c6e4 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c @@ -109,9 +109,10 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter, if (likely(act == XDP_REDIRECT)) { err = xdp_do_redirect(rx_ring->netdev, xdp, xdp_prog); - if (err) - goto out_failure; - return IXGBE_XDP_REDIR; + if (!err) + return IXGBE_XDP_REDIR; + result = (err == -ENOBUFS) ? IXGBE_XDP_EXIT : IXGBE_XDP_CONSUMED; + goto out_failure; } switch (act) { @@ -130,6 +131,9 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter, if (result == IXGBE_XDP_CONSUMED) goto out_failure; break; + case XDP_DROP: + result = IXGBE_XDP_CONSUMED; + break; default: bpf_warn_invalid_xdp_action(rx_ring->netdev, xdp_prog, act); fallthrough; @@ -137,9 +141,6 @@ static int ixgbe_run_xdp_zc(struct ixgbe_adapter *adapter, out_failure: trace_xdp_exception(rx_ring->netdev, xdp_prog, act); fallthrough; /* handle aborts by dropping packet */ - case XDP_DROP: - result = IXGBE_XDP_CONSUMED; - break; } return result; } @@ -304,10 +305,16 @@ int ixgbe_clean_rx_irq_zc(struct ixgbe_q_vector *q_vector, xdp_res = ixgbe_run_xdp_zc(adapter, rx_ring, bi->xdp); if (xdp_res) { - if (xdp_res & (IXGBE_XDP_TX | IXGBE_XDP_REDIR)) + if (xdp_res == IXGBE_XDP_EXIT) { + failure = true; + xsk_buff_free(bi->xdp); + ixgbe_inc_ntc(rx_ring); + break; + } else if (xdp_res & (IXGBE_XDP_TX | IXGBE_XDP_REDIR)) { xdp_xmit |= xdp_res; - else + } else { xsk_buff_free(bi->xdp); + } bi->xdp = NULL; total_rx_packets++;
Correlate -ENOBUFS that was returned from xdp_do_redirect() with a XSK Rx queue being full. In such case, terminate the softirq processing and let the user space to consume descriptors from XSK Rx queue so that there is room that driver can use later on. Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- .../ethernet/intel/ixgbe/ixgbe_txrx_common.h | 1 + drivers/net/ethernet/intel/ixgbe/ixgbe_xsk.c | 23 ++++++++++++------- 2 files changed, 16 insertions(+), 8 deletions(-)