[PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

[PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

[Liang Chen]

In the generic XDP processing flow, if an skb with a page pool page
(skb->pp_recycle == 1) fails to meet XDP packet requirements, it will
undergo head expansion and linearization of fragment data. As a result,
skb->head points to a reallocated buffer without any fragments. At this
point, the skb will not contain any page pool pages. However, the
skb->pp_recycle flag is still set to 1, which is inconsistent with the
actual situation. Although it doesn't seem to cause much real harm at the
moment(a little nagetive impact on skb_try_coalesce), to avoid potential
issues associated with using incorrect skb->pp_recycle information,
setting skb->pp_recycle to 0 to reflect the pp state of the skb.

Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
---
 net/core/dev.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/core/dev.c b/net/core/dev.c
index 10e5a036c706..07baf72be7d7 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4934,6 +4934,8 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
 			goto do_drop;
 		if (skb_linearize(skb))
 			goto do_drop;
+		if (skb->pp_recycle)
+			skb->pp_recycle = 0;
 	}
 
 	act = bpf_prog_run_generic_xdp(skb, xdp, xdp_prog);
-- 
2.40.1

Re: [PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

[Alexander Lobakin]

From: Liang Chen <liangchen.linux@gmail.com>
Date: Wed,  2 Aug 2023 15:04:54 +0800

> In the generic XDP processing flow, if an skb with a page pool page
> (skb->pp_recycle == 1) fails to meet XDP packet requirements, it will
> undergo head expansion and linearization of fragment data. As a result,
> skb->head points to a reallocated buffer without any fragments. At this
> point, the skb will not contain any page pool pages. However, the
> skb->pp_recycle flag is still set to 1, which is inconsistent with the
> actual situation. Although it doesn't seem to cause much real harm at the

This means it must be handled in the function which replaces the head,
i.e. pskb_expand_head(). Your change only suppresses one symptom of the
issue.

> moment(a little nagetive impact on skb_try_coalesce), to avoid potential

                  ^^^^^^^^
                  negative

> issues associated with using incorrect skb->pp_recycle information,
> setting skb->pp_recycle to 0 to reflect the pp state of the skb.
> 
> Signed-off-by: Liang Chen <liangchen.linux@gmail.com>

I won't say for sure, but may be a candidate for the fixes tree, not
next. This way it would need a "Fixes:" tag here (above the SoB).

> ---
>  net/core/dev.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 10e5a036c706..07baf72be7d7 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4934,6 +4934,8 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
>  			goto do_drop;
>  		if (skb_linearize(skb))
>  			goto do_drop;
> +		if (skb->pp_recycle)
> +			skb->pp_recycle = 0;
>  	}
>  
>  	act = bpf_prog_run_generic_xdp(skb, xdp, xdp_prog);

Thanks,
Olek

Re: [PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

[Jakub Kicinski]

On Wed,  2 Aug 2023 15:04:54 +0800 Liang Chen wrote:
> In the generic XDP processing flow, if an skb with a page pool page
> (skb->pp_recycle == 1) fails to meet XDP packet requirements, it will
> undergo head expansion and linearization of fragment data. As a result,
> skb->head points to a reallocated buffer without any fragments. At this
> point, the skb will not contain any page pool pages. However, the
> skb->pp_recycle flag is still set to 1, which is inconsistent with the
> actual situation. Although it doesn't seem to cause much real harm at the
> moment(a little nagetive impact on skb_try_coalesce), to avoid potential
> issues associated with using incorrect skb->pp_recycle information,
> setting skb->pp_recycle to 0 to reflect the pp state of the skb.

pp_recycle just means that the skb is "page pool aware", there's
absolutely no harm in having an skb with pp_recycle = 1 and no
page pool pages attached.

I vote not to apply this.

Re: [PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

[Liang Chen]

On Thu, Aug 3, 2023 at 1:11 AM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Liang Chen <liangchen.linux@gmail.com>
> Date: Wed,  2 Aug 2023 15:04:54 +0800
>
> > In the generic XDP processing flow, if an skb with a page pool page
> > (skb->pp_recycle == 1) fails to meet XDP packet requirements, it will
> > undergo head expansion and linearization of fragment data. As a result,
> > skb->head points to a reallocated buffer without any fragments. At this
> > point, the skb will not contain any page pool pages. However, the
> > skb->pp_recycle flag is still set to 1, which is inconsistent with the
> > actual situation. Although it doesn't seem to cause much real harm at the
>
> This means it must be handled in the function which replaces the head,
> i.e. pskb_expand_head(). Your change only suppresses one symptom of the
> issue.
>

I attempted to do so. But after pskb_expand_head, there may still be
skb frags with pp pages left. It is after skb_linearize those frags
are removed.

Thanks,
Liang

> > moment(a little nagetive impact on skb_try_coalesce), to avoid potential
>
>                   ^^^^^^^^
>                   negative
>

Sure, Thanks.


> > issues associated with using incorrect skb->pp_recycle information,
> > setting skb->pp_recycle to 0 to reflect the pp state of the skb.
> >
> > Signed-off-by: Liang Chen <liangchen.linux@gmail.com>
>
> I won't say for sure, but may be a candidate for the fixes tree, not
> next. This way it would need a "Fixes:" tag here (above the SoB).
>
> > ---
> >  net/core/dev.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 10e5a036c706..07baf72be7d7 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -4934,6 +4934,8 @@ static u32 netif_receive_generic_xdp(struct sk_buff *skb,
> >                       goto do_drop;
> >               if (skb_linearize(skb))
> >                       goto do_drop;
> > +             if (skb->pp_recycle)
> > +                     skb->pp_recycle = 0;
> >       }
> >
> >       act = bpf_prog_run_generic_xdp(skb, xdp, xdp_prog);
>
> Thanks,
> Olek

Re: [PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

[Liang Chen]

On Thu, Aug 3, 2023 at 2:37 AM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Wed,  2 Aug 2023 15:04:54 +0800 Liang Chen wrote:
> > In the generic XDP processing flow, if an skb with a page pool page
> > (skb->pp_recycle == 1) fails to meet XDP packet requirements, it will
> > undergo head expansion and linearization of fragment data. As a result,
> > skb->head points to a reallocated buffer without any fragments. At this
> > point, the skb will not contain any page pool pages. However, the
> > skb->pp_recycle flag is still set to 1, which is inconsistent with the
> > actual situation. Although it doesn't seem to cause much real harm at the
> > moment(a little nagetive impact on skb_try_coalesce), to avoid potential
> > issues associated with using incorrect skb->pp_recycle information,
> > setting skb->pp_recycle to 0 to reflect the pp state of the skb.
>
> pp_recycle just means that the skb is "page pool aware", there's
> absolutely no harm in having an skb with pp_recycle = 1 and no
> page pool pages attached.

I don't see it causing an error right now either. But it affects
skb_try_coalesce in a negative way from a performance perspective -
from->pp_recycle can be falsely true leading to a coalescing failure
in "(from->pp_recycle && skb_cloned(from))" test, which otherwise
would let the coalesce continue if from->pp_recycle was false. I
wonder if that justifies the need for a fix.

Thanks,
Liang


>
> I vote not to apply this.

Re: [PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

[Ilias Apalodimas]

Hi Liang

On Thu, 3 Aug 2023 at 11:26, Liang Chen <liangchen.linux@gmail.com> wrote:
>
> On Thu, Aug 3, 2023 at 2:37 AM Jakub Kicinski <kuba@kernel.org> wrote:
> >
> > On Wed,  2 Aug 2023 15:04:54 +0800 Liang Chen wrote:
> > > In the generic XDP processing flow, if an skb with a page pool page
> > > (skb->pp_recycle == 1) fails to meet XDP packet requirements, it will
> > > undergo head expansion and linearization of fragment data. As a result,
> > > skb->head points to a reallocated buffer without any fragments. At this
> > > point, the skb will not contain any page pool pages. However, the
> > > skb->pp_recycle flag is still set to 1, which is inconsistent with the
> > > actual situation. Although it doesn't seem to cause much real harm at the
> > > moment(a little nagetive impact on skb_try_coalesce), to avoid potential
> > > issues associated with using incorrect skb->pp_recycle information,
> > > setting skb->pp_recycle to 0 to reflect the pp state of the skb.
> >
> > pp_recycle just means that the skb is "page pool aware", there's
> > absolutely no harm in having an skb with pp_recycle = 1 and no
> > page pool pages attached.
>
> I don't see it causing an error right now either. But it affects
> skb_try_coalesce in a negative way from a performance perspective -
> from->pp_recycle can be falsely true leading to a coalescing failure
> in "(from->pp_recycle && skb_cloned(from))" test, which otherwise
> would let the coalesce continue if from->pp_recycle was false. I
> wonder if that justifies the need for a fix.

This applies to non-native XDP code only though, right?  IIRC that
code was to make xdp easier to use but it's not used when xdp-like
performance is needed.

Thanks
/Ilias
>
> Thanks,
> Liang
>
>
> >
> > I vote not to apply this.

Re: [PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

[Liang Chen]

On Thu, Aug 3, 2023 at 4:59 PM Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Liang
>
> On Thu, 3 Aug 2023 at 11:26, Liang Chen <liangchen.linux@gmail.com> wrote:
> >
> > On Thu, Aug 3, 2023 at 2:37 AM Jakub Kicinski <kuba@kernel.org> wrote:
> > >
> > > On Wed,  2 Aug 2023 15:04:54 +0800 Liang Chen wrote:
> > > > In the generic XDP processing flow, if an skb with a page pool page
> > > > (skb->pp_recycle == 1) fails to meet XDP packet requirements, it will
> > > > undergo head expansion and linearization of fragment data. As a result,
> > > > skb->head points to a reallocated buffer without any fragments. At this
> > > > point, the skb will not contain any page pool pages. However, the
> > > > skb->pp_recycle flag is still set to 1, which is inconsistent with the
> > > > actual situation. Although it doesn't seem to cause much real harm at the
> > > > moment(a little nagetive impact on skb_try_coalesce), to avoid potential
> > > > issues associated with using incorrect skb->pp_recycle information,
> > > > setting skb->pp_recycle to 0 to reflect the pp state of the skb.
> > >
> > > pp_recycle just means that the skb is "page pool aware", there's
> > > absolutely no harm in having an skb with pp_recycle = 1 and no
> > > page pool pages attached.
> >
> > I don't see it causing an error right now either. But it affects
> > skb_try_coalesce in a negative way from a performance perspective -
> > from->pp_recycle can be falsely true leading to a coalescing failure
> > in "(from->pp_recycle && skb_cloned(from))" test, which otherwise
> > would let the coalesce continue if from->pp_recycle was false. I
> > wonder if that justifies the need for a fix.
>
> This applies to non-native XDP code only though, right?  IIRC that
> code was to make xdp easier to use but it's not used when xdp-like
> performance is needed.

Sure, this applies to non-native XDP code only. Thanks.

Thanks,
Liang

>
> Thanks
> /Ilias
> >
> > Thanks,
> > Liang
> >
> >
> > >
> > > I vote not to apply this.

Re: [PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

[Alexander Lobakin]

From: Liang Chen <liangchen.linux@gmail.com>
Date: Thu, 3 Aug 2023 16:25:49 +0800

> On Thu, Aug 3, 2023 at 1:11 AM Alexander Lobakin
> <aleksander.lobakin@intel.com> wrote:
>>
>> From: Liang Chen <liangchen.linux@gmail.com>
>> Date: Wed,  2 Aug 2023 15:04:54 +0800
>>
>>> In the generic XDP processing flow, if an skb with a page pool page
>>> (skb->pp_recycle == 1) fails to meet XDP packet requirements, it will
>>> undergo head expansion and linearization of fragment data. As a result,
>>> skb->head points to a reallocated buffer without any fragments. At this
>>> point, the skb will not contain any page pool pages. However, the
>>> skb->pp_recycle flag is still set to 1, which is inconsistent with the
>>> actual situation. Although it doesn't seem to cause much real harm at the
>>
>> This means it must be handled in the function which replaces the head,
>> i.e. pskb_expand_head(). Your change only suppresses one symptom of the
>> issue.
>>
> 
> I attempted to do so. But after pskb_expand_head, there may still be
> skb frags with pp pages left. It is after skb_linearize those frags
> are removed.

Ah, right.
Then you need to handle that in __pskb_pull_tail(). Check at the end of
the function whether the skb still has any frags, and if not, clear
skb->pp_recycle.

The most correct fix would be to do that in both pskb_expand_head() and
__pskb_pull_tail(): iterate over the frags and check if any page still
belongs to a page_pool. Then page_pool_return_skb_page() wouldn't hit
false-branch after the skb was re-layout.

[...]

Thanks,
Olek

Re: [PATCH net-next] xdp: Fixing skb->pp_recycle flag in generic XDP handling

[Liang Chen]

On Thu, Aug 3, 2023 at 11:17 PM Alexander Lobakin
<aleksander.lobakin@intel.com> wrote:
>
> From: Liang Chen <liangchen.linux@gmail.com>
> Date: Thu, 3 Aug 2023 16:25:49 +0800
>
> > On Thu, Aug 3, 2023 at 1:11 AM Alexander Lobakin
> > <aleksander.lobakin@intel.com> wrote:
> >>
> >> From: Liang Chen <liangchen.linux@gmail.com>
> >> Date: Wed,  2 Aug 2023 15:04:54 +0800
> >>
> >>> In the generic XDP processing flow, if an skb with a page pool page
> >>> (skb->pp_recycle == 1) fails to meet XDP packet requirements, it will
> >>> undergo head expansion and linearization of fragment data. As a result,
> >>> skb->head points to a reallocated buffer without any fragments. At this
> >>> point, the skb will not contain any page pool pages. However, the
> >>> skb->pp_recycle flag is still set to 1, which is inconsistent with the
> >>> actual situation. Although it doesn't seem to cause much real harm at the
> >>
> >> This means it must be handled in the function which replaces the head,
> >> i.e. pskb_expand_head(). Your change only suppresses one symptom of the
> >> issue.
> >>
> >
> > I attempted to do so. But after pskb_expand_head, there may still be
> > skb frags with pp pages left. It is after skb_linearize those frags
> > are removed.
>
> Ah, right.
> Then you need to handle that in __pskb_pull_tail(). Check at the end of
> the function whether the skb still has any frags, and if not, clear
> skb->pp_recycle.
>
> The most correct fix would be to do that in both pskb_expand_head() and
> __pskb_pull_tail(): iterate over the frags and check if any page still
> belongs to a page_pool. Then page_pool_return_skb_page() wouldn't hit
> false-branch after the skb was re-layout.
>

Yeah, I agree. netif_receive_generic_xdp may not be the only place the
flag can get wrong. To make the pp_recycle flag strictly reflect the
state of page pool usages, the check should be put in both functions.
But, since it's merely an indication that the skb is 'page pool
aware,' and considering that the performance impact we observed
doesn't affect the native XDP path, addressing this issue doesn't seem
worthwhile based on the feedback I've received.


Thanks,
Liang

> [...]
>
> Thanks,
> Olek