| Message ID | 20240124191602.566724-5-maciej.fijalkowski@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ad2047cf5d9313200e308612aed516548873d124 |
| Delegated to: | BPF |
| Headers | show |
| Series | net: bpf_xdp_adjust_tail() and Intel mbuf fixes | expand |
On 1/24/2024 11:15 AM, Maciej Fijalkowski wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > Fix an OOM panic in XDP_DRV mode when a XDP program shrinks a > multi-buffer packet by 4k bytes and then redirects it to an AF_XDP > socket. > > Since support for handling multi-buffer frames was added to XDP, usage > of bpf_xdp_adjust_tail() helper within XDP program can free the page > that given fragment occupies and in turn decrease the fragment count > within skb_shared_info that is embedded in xdp_buff struct. In current > ice driver codebase, it can become problematic when page recycling logic > decides not to reuse the page. In such case, __page_frag_cache_drain() > is used with ice_rx_buf::pagecnt_bias that was not adjusted after > refcount of page was changed by XDP prog which in turn does not drain > the refcount to 0 and page is never freed. > > To address this, let us store the count of frags before the XDP program > was executed on Rx ring struct. This will be used to compare with > current frag count from skb_shared_info embedded in xdp_buff. A smaller > value in the latter indicates that XDP prog freed frag(s). Then, for > given delta decrement pagecnt_bias for XDP_DROP verdict. > > While at it, let us also handle the EOP frag within > ice_set_rx_bufs_act() to make our life easier, so all of the adjustments > needed to be applied against freed frags are performed in the single > place. > > Fixes: 2fba7dc5157b ("ice: Add support for XDP multi-buffer on Rx side") > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > --- > drivers/net/ethernet/intel/ice/ice_txrx.c | 14 ++++++--- > drivers/net/ethernet/intel/ice/ice_txrx.h | 1 + > drivers/net/ethernet/intel/ice/ice_txrx_lib.h | 31 +++++++++++++------ > 3 files changed, 32 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c > index 74d13cc5a3a7..0c9b4aa8a049 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c > @@ -603,9 +603,7 @@ ice_run_xdp(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp, > ret = ICE_XDP_CONSUMED; > } > exit: > - rx_buf->act = ret; > - if (unlikely(xdp_buff_has_frags(xdp))) > - ice_set_rx_bufs_act(xdp, rx_ring, ret); > + ice_set_rx_bufs_act(xdp, rx_ring, ret); > } > > /** > @@ -893,14 +891,17 @@ ice_add_xdp_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp, > } > > if (unlikely(sinfo->nr_frags == MAX_SKB_FRAGS)) { > - if (unlikely(xdp_buff_has_frags(xdp))) > - ice_set_rx_bufs_act(xdp, rx_ring, ICE_XDP_CONSUMED); > + ice_set_rx_bufs_act(xdp, rx_ring, ICE_XDP_CONSUMED); > return -ENOMEM; > } > > __skb_fill_page_desc_noacc(sinfo, sinfo->nr_frags++, rx_buf->page, > rx_buf->page_offset, size); > sinfo->xdp_frags_size += size; > + /* remember frag count before XDP prog execution; bpf_xdp_adjust_tail() > + * can pop off frags but driver has to handle it on its own > + */ > + rx_ring->nr_frags = sinfo->nr_frags; > > if (page_is_pfmemalloc(rx_buf->page)) > xdp_buff_set_frag_pfmemalloc(xdp); > @@ -1251,6 +1252,7 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget) > > xdp->data = NULL; > rx_ring->first_desc = ntc; > + rx_ring->nr_frags = 0; > continue; > construct_skb: > if (likely(ice_ring_uses_build_skb(rx_ring))) > @@ -1266,10 +1268,12 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget) > ICE_XDP_CONSUMED); > xdp->data = NULL; > rx_ring->first_desc = ntc; > + rx_ring->nr_frags = 0; > break; > } > xdp->data = NULL; > rx_ring->first_desc = ntc; > + rx_ring->nr_frags = 0; > > stat_err_bits = BIT(ICE_RX_FLEX_DESC_STATUS0_RXE_S); > if (unlikely(ice_test_staterr(rx_desc->wb.status_error0, > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h > index b3379ff73674..af955b0e5dc5 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx.h > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h > @@ -358,6 +358,7 @@ struct ice_rx_ring { > struct ice_tx_ring *xdp_ring; > struct ice_rx_ring *next; /* pointer to next ring in q_vector */ > struct xsk_buff_pool *xsk_pool; > + u32 nr_frags; > dma_addr_t dma; /* physical address of ring */ > u16 rx_buf_len; > u8 dcb_tc; /* Traffic class of ring */ > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h > index 762047508619..afcead4baef4 100644 > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h > @@ -12,26 +12,39 @@ > * act: action to store onto Rx buffers related to XDP buffer parts > * > * Set action that should be taken before putting Rx buffer from first frag > - * to one before last. Last one is handled by caller of this function as it > - * is the EOP frag that is currently being processed. This function is > - * supposed to be called only when XDP buffer contains frags. > + * to the last. > */ > static inline void > ice_set_rx_bufs_act(struct xdp_buff *xdp, const struct ice_rx_ring *rx_ring, > const unsigned int act) > { > - const struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp); > - u32 first = rx_ring->first_desc; > - u32 nr_frags = sinfo->nr_frags; > + u32 sinfo_frags = xdp_get_shared_info_from_buff(xdp)->nr_frags; > + u32 nr_frags = rx_ring->nr_frags + 1; > + u32 idx = rx_ring->first_desc; > u32 cnt = rx_ring->count; > struct ice_rx_buf *buf; > > for (int i = 0; i < nr_frags; i++) { > - buf = &rx_ring->rx_buf[first]; > + buf = &rx_ring->rx_buf[idx]; > buf->act = act; > > - if (++first == cnt) > - first = 0; > + if (++idx == cnt) > + idx = 0; > + } > + > + /* adjust pagecnt_bias on frags freed by XDP prog */ > + if (sinfo_frags < rx_ring->nr_frags && act == ICE_XDP_CONSUMED) { > + u32 delta = rx_ring->nr_frags - sinfo_frags; > + > + while (delta) { > + if (idx == 0) > + idx = cnt - 1; > + else > + idx--; > + buf = &rx_ring->rx_buf[idx]; > + buf->pagecnt_bias--; > + delta--; > + } Nit, but the function name ice_set_rx_bufs_act() doesn't completely align with what it's doing anymore due to the additional pagecnt_bias changes. > } > } > > -- > 2.34.1 > >
On Wed, Jan 24, 2024 at 6:26 PM Brett Creeley <bcreeley@amd.com> wrote: > > > > On 1/24/2024 11:15 AM, Maciej Fijalkowski wrote: > > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > > > > Fix an OOM panic in XDP_DRV mode when a XDP program shrinks a > > multi-buffer packet by 4k bytes and then redirects it to an AF_XDP > > socket. > > > > Since support for handling multi-buffer frames was added to XDP, usage > > of bpf_xdp_adjust_tail() helper within XDP program can free the page > > that given fragment occupies and in turn decrease the fragment count > > within skb_shared_info that is embedded in xdp_buff struct. In current > > ice driver codebase, it can become problematic when page recycling logic > > decides not to reuse the page. In such case, __page_frag_cache_drain() > > is used with ice_rx_buf::pagecnt_bias that was not adjusted after > > refcount of page was changed by XDP prog which in turn does not drain > > the refcount to 0 and page is never freed. > > > > To address this, let us store the count of frags before the XDP program > > was executed on Rx ring struct. This will be used to compare with > > current frag count from skb_shared_info embedded in xdp_buff. A smaller > > value in the latter indicates that XDP prog freed frag(s). Then, for > > given delta decrement pagecnt_bias for XDP_DROP verdict. > > > > While at it, let us also handle the EOP frag within > > ice_set_rx_bufs_act() to make our life easier, so all of the adjustments > > needed to be applied against freed frags are performed in the single > > place. > > > > Fixes: 2fba7dc5157b ("ice: Add support for XDP multi-buffer on Rx side") > > Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > --- > > drivers/net/ethernet/intel/ice/ice_txrx.c | 14 ++++++--- > > drivers/net/ethernet/intel/ice/ice_txrx.h | 1 + > > drivers/net/ethernet/intel/ice/ice_txrx_lib.h | 31 +++++++++++++------ > > 3 files changed, 32 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c > > index 74d13cc5a3a7..0c9b4aa8a049 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c > > @@ -603,9 +603,7 @@ ice_run_xdp(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp, > > ret = ICE_XDP_CONSUMED; > > } > > exit: > > - rx_buf->act = ret; > > - if (unlikely(xdp_buff_has_frags(xdp))) > > - ice_set_rx_bufs_act(xdp, rx_ring, ret); > > + ice_set_rx_bufs_act(xdp, rx_ring, ret); > > } > > > > /** > > @@ -893,14 +891,17 @@ ice_add_xdp_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp, > > } > > > > if (unlikely(sinfo->nr_frags == MAX_SKB_FRAGS)) { > > - if (unlikely(xdp_buff_has_frags(xdp))) > > - ice_set_rx_bufs_act(xdp, rx_ring, ICE_XDP_CONSUMED); > > + ice_set_rx_bufs_act(xdp, rx_ring, ICE_XDP_CONSUMED); > > return -ENOMEM; > > } > > > > __skb_fill_page_desc_noacc(sinfo, sinfo->nr_frags++, rx_buf->page, > > rx_buf->page_offset, size); > > sinfo->xdp_frags_size += size; > > + /* remember frag count before XDP prog execution; bpf_xdp_adjust_tail() > > + * can pop off frags but driver has to handle it on its own > > + */ > > + rx_ring->nr_frags = sinfo->nr_frags; > > > > if (page_is_pfmemalloc(rx_buf->page)) > > xdp_buff_set_frag_pfmemalloc(xdp); > > @@ -1251,6 +1252,7 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget) > > > > xdp->data = NULL; > > rx_ring->first_desc = ntc; > > + rx_ring->nr_frags = 0; > > continue; > > construct_skb: > > if (likely(ice_ring_uses_build_skb(rx_ring))) > > @@ -1266,10 +1268,12 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget) > > ICE_XDP_CONSUMED); > > xdp->data = NULL; > > rx_ring->first_desc = ntc; > > + rx_ring->nr_frags = 0; > > break; > > } > > xdp->data = NULL; > > rx_ring->first_desc = ntc; > > + rx_ring->nr_frags = 0; > > > > stat_err_bits = BIT(ICE_RX_FLEX_DESC_STATUS0_RXE_S); > > if (unlikely(ice_test_staterr(rx_desc->wb.status_error0, > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h > > index b3379ff73674..af955b0e5dc5 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_txrx.h > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h > > @@ -358,6 +358,7 @@ struct ice_rx_ring { > > struct ice_tx_ring *xdp_ring; > > struct ice_rx_ring *next; /* pointer to next ring in q_vector */ > > struct xsk_buff_pool *xsk_pool; > > + u32 nr_frags; > > dma_addr_t dma; /* physical address of ring */ > > u16 rx_buf_len; > > u8 dcb_tc; /* Traffic class of ring */ > > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h > > index 762047508619..afcead4baef4 100644 > > --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h > > +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h > > @@ -12,26 +12,39 @@ > > * act: action to store onto Rx buffers related to XDP buffer parts > > * > > * Set action that should be taken before putting Rx buffer from first frag > > - * to one before last. Last one is handled by caller of this function as it > > - * is the EOP frag that is currently being processed. This function is > > - * supposed to be called only when XDP buffer contains frags. > > + * to the last. > > */ > > static inline void > > ice_set_rx_bufs_act(struct xdp_buff *xdp, const struct ice_rx_ring *rx_ring, > > const unsigned int act) > > { > > - const struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp); > > - u32 first = rx_ring->first_desc; > > - u32 nr_frags = sinfo->nr_frags; > > + u32 sinfo_frags = xdp_get_shared_info_from_buff(xdp)->nr_frags; > > + u32 nr_frags = rx_ring->nr_frags + 1; > > + u32 idx = rx_ring->first_desc; > > u32 cnt = rx_ring->count; > > struct ice_rx_buf *buf; > > > > for (int i = 0; i < nr_frags; i++) { > > - buf = &rx_ring->rx_buf[first]; > > + buf = &rx_ring->rx_buf[idx]; > > buf->act = act; > > > > - if (++first == cnt) > > - first = 0; > > + if (++idx == cnt) > > + idx = 0; > > + } > > + > > + /* adjust pagecnt_bias on frags freed by XDP prog */ > > + if (sinfo_frags < rx_ring->nr_frags && act == ICE_XDP_CONSUMED) { > > + u32 delta = rx_ring->nr_frags - sinfo_frags; > > + > > + while (delta) { > > + if (idx == 0) > > + idx = cnt - 1; > > + else > > + idx--; > > + buf = &rx_ring->rx_buf[idx]; > > + buf->pagecnt_bias--; > > + delta--; > > + } > > Nit, but the function name ice_set_rx_bufs_act() doesn't completely > align with what it's doing anymore due to the additional pagecnt_bias > changes. The patch set was applied. Please advise whether this can stay as-is or follow up is absolutely necessary.
On 1/24/2024 6:34 PM, Alexei Starovoitov wrote: > Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. > > > On Wed, Jan 24, 2024 at 6:26 PM Brett Creeley <bcreeley@amd.com> wrote: >> >> >> >> On 1/24/2024 11:15 AM, Maciej Fijalkowski wrote: >>> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding. >>> >>> >>> Fix an OOM panic in XDP_DRV mode when a XDP program shrinks a >>> multi-buffer packet by 4k bytes and then redirects it to an AF_XDP >>> socket. >>> >>> Since support for handling multi-buffer frames was added to XDP, usage >>> of bpf_xdp_adjust_tail() helper within XDP program can free the page >>> that given fragment occupies and in turn decrease the fragment count >>> within skb_shared_info that is embedded in xdp_buff struct. In current >>> ice driver codebase, it can become problematic when page recycling logic >>> decides not to reuse the page. In such case, __page_frag_cache_drain() >>> is used with ice_rx_buf::pagecnt_bias that was not adjusted after >>> refcount of page was changed by XDP prog which in turn does not drain >>> the refcount to 0 and page is never freed. >>> >>> To address this, let us store the count of frags before the XDP program >>> was executed on Rx ring struct. This will be used to compare with >>> current frag count from skb_shared_info embedded in xdp_buff. A smaller >>> value in the latter indicates that XDP prog freed frag(s). Then, for >>> given delta decrement pagecnt_bias for XDP_DROP verdict. >>> >>> While at it, let us also handle the EOP frag within >>> ice_set_rx_bufs_act() to make our life easier, so all of the adjustments >>> needed to be applied against freed frags are performed in the single >>> place. >>> >>> Fixes: 2fba7dc5157b ("ice: Add support for XDP multi-buffer on Rx side") >>> Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> >>> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> >>> --- >>> drivers/net/ethernet/intel/ice/ice_txrx.c | 14 ++++++--- >>> drivers/net/ethernet/intel/ice/ice_txrx.h | 1 + >>> drivers/net/ethernet/intel/ice/ice_txrx_lib.h | 31 +++++++++++++------ >>> 3 files changed, 32 insertions(+), 14 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c >>> index 74d13cc5a3a7..0c9b4aa8a049 100644 >>> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c >>> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c >>> @@ -603,9 +603,7 @@ ice_run_xdp(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp, >>> ret = ICE_XDP_CONSUMED; >>> } >>> exit: >>> - rx_buf->act = ret; >>> - if (unlikely(xdp_buff_has_frags(xdp))) >>> - ice_set_rx_bufs_act(xdp, rx_ring, ret); >>> + ice_set_rx_bufs_act(xdp, rx_ring, ret); >>> } >>> >>> /** >>> @@ -893,14 +891,17 @@ ice_add_xdp_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp, >>> } >>> >>> if (unlikely(sinfo->nr_frags == MAX_SKB_FRAGS)) { >>> - if (unlikely(xdp_buff_has_frags(xdp))) >>> - ice_set_rx_bufs_act(xdp, rx_ring, ICE_XDP_CONSUMED); >>> + ice_set_rx_bufs_act(xdp, rx_ring, ICE_XDP_CONSUMED); >>> return -ENOMEM; >>> } >>> >>> __skb_fill_page_desc_noacc(sinfo, sinfo->nr_frags++, rx_buf->page, >>> rx_buf->page_offset, size); >>> sinfo->xdp_frags_size += size; >>> + /* remember frag count before XDP prog execution; bpf_xdp_adjust_tail() >>> + * can pop off frags but driver has to handle it on its own >>> + */ >>> + rx_ring->nr_frags = sinfo->nr_frags; >>> >>> if (page_is_pfmemalloc(rx_buf->page)) >>> xdp_buff_set_frag_pfmemalloc(xdp); >>> @@ -1251,6 +1252,7 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget) >>> >>> xdp->data = NULL; >>> rx_ring->first_desc = ntc; >>> + rx_ring->nr_frags = 0; >>> continue; >>> construct_skb: >>> if (likely(ice_ring_uses_build_skb(rx_ring))) >>> @@ -1266,10 +1268,12 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget) >>> ICE_XDP_CONSUMED); >>> xdp->data = NULL; >>> rx_ring->first_desc = ntc; >>> + rx_ring->nr_frags = 0; >>> break; >>> } >>> xdp->data = NULL; >>> rx_ring->first_desc = ntc; >>> + rx_ring->nr_frags = 0; >>> >>> stat_err_bits = BIT(ICE_RX_FLEX_DESC_STATUS0_RXE_S); >>> if (unlikely(ice_test_staterr(rx_desc->wb.status_error0, >>> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h >>> index b3379ff73674..af955b0e5dc5 100644 >>> --- a/drivers/net/ethernet/intel/ice/ice_txrx.h >>> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h >>> @@ -358,6 +358,7 @@ struct ice_rx_ring { >>> struct ice_tx_ring *xdp_ring; >>> struct ice_rx_ring *next; /* pointer to next ring in q_vector */ >>> struct xsk_buff_pool *xsk_pool; >>> + u32 nr_frags; >>> dma_addr_t dma; /* physical address of ring */ >>> u16 rx_buf_len; >>> u8 dcb_tc; /* Traffic class of ring */ >>> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h >>> index 762047508619..afcead4baef4 100644 >>> --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h >>> +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h >>> @@ -12,26 +12,39 @@ >>> * act: action to store onto Rx buffers related to XDP buffer parts >>> * >>> * Set action that should be taken before putting Rx buffer from first frag >>> - * to one before last. Last one is handled by caller of this function as it >>> - * is the EOP frag that is currently being processed. This function is >>> - * supposed to be called only when XDP buffer contains frags. >>> + * to the last. >>> */ >>> static inline void >>> ice_set_rx_bufs_act(struct xdp_buff *xdp, const struct ice_rx_ring *rx_ring, >>> const unsigned int act) >>> { >>> - const struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp); >>> - u32 first = rx_ring->first_desc; >>> - u32 nr_frags = sinfo->nr_frags; >>> + u32 sinfo_frags = xdp_get_shared_info_from_buff(xdp)->nr_frags; >>> + u32 nr_frags = rx_ring->nr_frags + 1; >>> + u32 idx = rx_ring->first_desc; >>> u32 cnt = rx_ring->count; >>> struct ice_rx_buf *buf; >>> >>> for (int i = 0; i < nr_frags; i++) { >>> - buf = &rx_ring->rx_buf[first]; >>> + buf = &rx_ring->rx_buf[idx]; >>> buf->act = act; >>> >>> - if (++first == cnt) >>> - first = 0; >>> + if (++idx == cnt) >>> + idx = 0; >>> + } >>> + >>> + /* adjust pagecnt_bias on frags freed by XDP prog */ >>> + if (sinfo_frags < rx_ring->nr_frags && act == ICE_XDP_CONSUMED) { >>> + u32 delta = rx_ring->nr_frags - sinfo_frags; >>> + >>> + while (delta) { >>> + if (idx == 0) >>> + idx = cnt - 1; >>> + else >>> + idx--; >>> + buf = &rx_ring->rx_buf[idx]; >>> + buf->pagecnt_bias--; >>> + delta--; >>> + } >> >> Nit, but the function name ice_set_rx_bufs_act() doesn't completely >> align with what it's doing anymore due to the additional pagecnt_bias >> changes. > > The patch set was applied. Please advise whether this can stay as-is > or follow up is absolutely necessary. I'm okay with it as is. If Maciej agrees he could always provide a follow up. Thanks, Brett
diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c index 74d13cc5a3a7..0c9b4aa8a049 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx.c +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c @@ -603,9 +603,7 @@ ice_run_xdp(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp, ret = ICE_XDP_CONSUMED; } exit: - rx_buf->act = ret; - if (unlikely(xdp_buff_has_frags(xdp))) - ice_set_rx_bufs_act(xdp, rx_ring, ret); + ice_set_rx_bufs_act(xdp, rx_ring, ret); } /** @@ -893,14 +891,17 @@ ice_add_xdp_frag(struct ice_rx_ring *rx_ring, struct xdp_buff *xdp, } if (unlikely(sinfo->nr_frags == MAX_SKB_FRAGS)) { - if (unlikely(xdp_buff_has_frags(xdp))) - ice_set_rx_bufs_act(xdp, rx_ring, ICE_XDP_CONSUMED); + ice_set_rx_bufs_act(xdp, rx_ring, ICE_XDP_CONSUMED); return -ENOMEM; } __skb_fill_page_desc_noacc(sinfo, sinfo->nr_frags++, rx_buf->page, rx_buf->page_offset, size); sinfo->xdp_frags_size += size; + /* remember frag count before XDP prog execution; bpf_xdp_adjust_tail() + * can pop off frags but driver has to handle it on its own + */ + rx_ring->nr_frags = sinfo->nr_frags; if (page_is_pfmemalloc(rx_buf->page)) xdp_buff_set_frag_pfmemalloc(xdp); @@ -1251,6 +1252,7 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget) xdp->data = NULL; rx_ring->first_desc = ntc; + rx_ring->nr_frags = 0; continue; construct_skb: if (likely(ice_ring_uses_build_skb(rx_ring))) @@ -1266,10 +1268,12 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget) ICE_XDP_CONSUMED); xdp->data = NULL; rx_ring->first_desc = ntc; + rx_ring->nr_frags = 0; break; } xdp->data = NULL; rx_ring->first_desc = ntc; + rx_ring->nr_frags = 0; stat_err_bits = BIT(ICE_RX_FLEX_DESC_STATUS0_RXE_S); if (unlikely(ice_test_staterr(rx_desc->wb.status_error0, diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.h b/drivers/net/ethernet/intel/ice/ice_txrx.h index b3379ff73674..af955b0e5dc5 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx.h +++ b/drivers/net/ethernet/intel/ice/ice_txrx.h @@ -358,6 +358,7 @@ struct ice_rx_ring { struct ice_tx_ring *xdp_ring; struct ice_rx_ring *next; /* pointer to next ring in q_vector */ struct xsk_buff_pool *xsk_pool; + u32 nr_frags; dma_addr_t dma; /* physical address of ring */ u16 rx_buf_len; u8 dcb_tc; /* Traffic class of ring */ diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h index 762047508619..afcead4baef4 100644 --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.h +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.h @@ -12,26 +12,39 @@ * act: action to store onto Rx buffers related to XDP buffer parts * * Set action that should be taken before putting Rx buffer from first frag - * to one before last. Last one is handled by caller of this function as it - * is the EOP frag that is currently being processed. This function is - * supposed to be called only when XDP buffer contains frags. + * to the last. */ static inline void ice_set_rx_bufs_act(struct xdp_buff *xdp, const struct ice_rx_ring *rx_ring, const unsigned int act) { - const struct skb_shared_info *sinfo = xdp_get_shared_info_from_buff(xdp); - u32 first = rx_ring->first_desc; - u32 nr_frags = sinfo->nr_frags; + u32 sinfo_frags = xdp_get_shared_info_from_buff(xdp)->nr_frags; + u32 nr_frags = rx_ring->nr_frags + 1; + u32 idx = rx_ring->first_desc; u32 cnt = rx_ring->count; struct ice_rx_buf *buf; for (int i = 0; i < nr_frags; i++) { - buf = &rx_ring->rx_buf[first]; + buf = &rx_ring->rx_buf[idx]; buf->act = act; - if (++first == cnt) - first = 0; + if (++idx == cnt) + idx = 0; + } + + /* adjust pagecnt_bias on frags freed by XDP prog */ + if (sinfo_frags < rx_ring->nr_frags && act == ICE_XDP_CONSUMED) { + u32 delta = rx_ring->nr_frags - sinfo_frags; + + while (delta) { + if (idx == 0) + idx = cnt - 1; + else + idx--; + buf = &rx_ring->rx_buf[idx]; + buf->pagecnt_bias--; + delta--; + } } }
Fix an OOM panic in XDP_DRV mode when a XDP program shrinks a multi-buffer packet by 4k bytes and then redirects it to an AF_XDP socket. Since support for handling multi-buffer frames was added to XDP, usage of bpf_xdp_adjust_tail() helper within XDP program can free the page that given fragment occupies and in turn decrease the fragment count within skb_shared_info that is embedded in xdp_buff struct. In current ice driver codebase, it can become problematic when page recycling logic decides not to reuse the page. In such case, __page_frag_cache_drain() is used with ice_rx_buf::pagecnt_bias that was not adjusted after refcount of page was changed by XDP prog which in turn does not drain the refcount to 0 and page is never freed. To address this, let us store the count of frags before the XDP program was executed on Rx ring struct. This will be used to compare with current frag count from skb_shared_info embedded in xdp_buff. A smaller value in the latter indicates that XDP prog freed frag(s). Then, for given delta decrement pagecnt_bias for XDP_DROP verdict. While at it, let us also handle the EOP frag within ice_set_rx_bufs_act() to make our life easier, so all of the adjustments needed to be applied against freed frags are performed in the single place. Fixes: 2fba7dc5157b ("ice: Add support for XDP multi-buffer on Rx side") Acked-by: Magnus Karlsson <magnus.karlsson@intel.com> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- drivers/net/ethernet/intel/ice/ice_txrx.c | 14 ++++++--- drivers/net/ethernet/intel/ice/ice_txrx.h | 1 + drivers/net/ethernet/intel/ice/ice_txrx_lib.h | 31 +++++++++++++------ 3 files changed, 32 insertions(+), 14 deletions(-)