On Mon, Jan 11, 2021 at 4:45 AM Xin Long <lucien.xin@xxxxxxxxx> wrote:
>
> After commit 89319d3801d1 ("net: Add frag_list support to skb_segment"),
> it goes to process frag_list when !hsize in skb_segment(). However, when
> using skb frag_list, sg normally should not be set. In this case, hsize
> will be set with len right before !hsize check, then it won't go to
> frag_list processing code.
>
> So the right thing to do is move the hsize check to the else block, so
> that it won't affect the !hsize check for frag_list processing.
>
> Signed-off-by: Xin Long <lucien.xin@xxxxxxxxx>
> ---
> net/core/skbuff.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 7626a33..ea79359 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -3855,8 +3855,6 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
> hsize = skb_headlen(head_skb) - offset;
> if (hsize < 0)
> hsize = 0;
> - if (hsize > len || !sg)
> - hsize = len;
>
> if (!hsize && i >= nfrags && skb_headlen(list_skb) &&
> (skb_headlen(list_skb) == len || sg)) {
So looking at the function it seems like the only spot where the
standard path actually reads the hsize value is right here, and it is
overwritten before we exit the non-error portion of the if statement.
I wonder if we couldn't save ourselves a few cycles and avoid an
unnecessary assignment by replacing the "!hsize" with a check for
"hsize <= 0" and just move the entire set of checks above down into
the lower block.
> @@ -3901,6 +3899,9 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
> skb_release_head_state(nskb);
> __skb_push(nskb, doffset);
> } else {
> + if (hsize > len || !sg)
> + hsize = len;
> +
Then you could essentially just add the "if (hsize < 0)" piece here as
an "else if" check and avoid the check if it isn't needed.
> nskb = __alloc_skb(hsize + doffset + headroom,
> GFP_ATOMIC, skb_alloc_rx_flag(head_skb),
> NUMA_NO_NODE);
> --
> 2.1.0
>
