| Message ID | CAGCJULOjetohzdyuMN=jfKPMx8_a4WUa2r=9whNvK22xNqhqNw@mail.gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: Fix missing meta data in skb with vlan packet | expand |
Hi, (CC: XDP maintainers) On 2019/04/13 21:16, Yuya Kusakabe wrote: > skb_reorder_vlan_header() should move XDP meta data with ethernet > header if XDP meta data exists. > > Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com> > Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com> > Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com> Missing fixes tag Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access") > --- > net/core/skbuff.c | 10 +++++++++- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > index ef2cd5712098..6bc663249c4c 100644 > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -5083,7 +5083,8 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len); > > static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb) > { > - int mac_len; > + int mac_len, meta_len; > + void *meta; > > if (skb_cow(skb, skb_headroom(skb)) < 0) { > kfree_skb(skb); > @@ -5095,6 +5096,13 @@ static struct sk_buff > *skb_reorder_vlan_header(struct sk_buff *skb) > memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb), > mac_len - VLAN_HLEN - ETH_TLEN); > } > + > + meta_len = skb_metadata_len(skb); > + if (meta_len) { Since this is not used by non-XDP skb and skb path is slow-path for XDP anyway, should add unlikely here in favor of non-XDP case? > + meta = skb_metadata_end(skb) - meta_len; > + memmove(meta + VLAN_HLEN, meta, meta_len); > + }; > + > skb->mac_header += VLAN_HLEN; > return skb; > } > -- > 2.20.1 > >
Hi, Thank you for the review. On Mon, Apr 15, 2019 at 3:06 PM Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote: > > Hi, > (CC: XDP maintainers) > > On 2019/04/13 21:16, Yuya Kusakabe wrote: > > skb_reorder_vlan_header() should move XDP meta data with ethernet > > header if XDP meta data exists. > > > > Signed-off-by: Yuya Kusakabe <yuya.kusakabe@gmail.com> > > Signed-off-by: Takeru Hayasaka <taketarou2@gmail.com> > > Co-developed-by: Takeru Hayasaka <taketarou2@gmail.com> > > Missing fixes tag > > Fixes: de8f3a83b0a0 ("bpf: add meta pointer for direct access") I will add this line in v2 patch. > > > > --- > > net/core/skbuff.c | 10 +++++++++- > > 1 file changed, 9 insertions(+), 1 deletion(-) > > > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index ef2cd5712098..6bc663249c4c 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c > > @@ -5083,7 +5083,8 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len); > > > > static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb) > > { > > - int mac_len; > > + int mac_len, meta_len; > > + void *meta; > > > > if (skb_cow(skb, skb_headroom(skb)) < 0) { > > kfree_skb(skb); > > @@ -5095,6 +5096,13 @@ static struct sk_buff > > *skb_reorder_vlan_header(struct sk_buff *skb) > > memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb), > > mac_len - VLAN_HLEN - ETH_TLEN); > > } > > + > > + meta_len = skb_metadata_len(skb); > > + if (meta_len) { > > Since this is not used by non-XDP skb and skb path is slow-path for XDP > anyway, should add unlikely here in favor of non-XDP case? I referred to meta data support of the ixgbe driver. It doesn't use "unlikely". https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/tree/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c#n2176 > > > > + meta = skb_metadata_end(skb) - meta_len; > > + memmove(meta + VLAN_HLEN, meta, meta_len); > > + }; > > + > > skb->mac_header += VLAN_HLEN; > > return skb; > > } > > -- > > 2.20.1 > > > > > > -- > Toshiaki Makita > Thank you.
On 2019/04/15 16:43, Yuya Kusakabe wrote: ... > > @@ -5095,6 +5096,13 @@ static struct sk_buff > > *skb_reorder_vlan_header(struct sk_buff *skb) > > memmove(skb_mac_header(skb) + VLAN_HLEN, > skb_mac_header(skb), > > mac_len - VLAN_HLEN - ETH_TLEN); > > } > > + > > + meta_len = skb_metadata_len(skb); > > + if (meta_len) { > > Since this is not used by non-XDP skb and skb path is slow-path for XDP > anyway, should add unlikely here in favor of non-XDP case? > > > I referred to meta data support of the ixgbe driver. It doesn't use > "unlikely". > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/tree/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c#n2176 No need to conform to existing logic when we can improve it... Anyway this is a minor point and I'm OK with either. Reviewed-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
On Mon, Apr 15, 2019 at 5:08 PM Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote: > > On 2019/04/15 16:43, Yuya Kusakabe wrote: > ... > > > @@ -5095,6 +5096,13 @@ static struct sk_buff > > > *skb_reorder_vlan_header(struct sk_buff *skb) > > > memmove(skb_mac_header(skb) + VLAN_HLEN, > > skb_mac_header(skb), > > > mac_len - VLAN_HLEN - ETH_TLEN); > > > } > > > + > > > + meta_len = skb_metadata_len(skb); > > > + if (meta_len) { > > > > Since this is not used by non-XDP skb and skb path is slow-path for XDP > > anyway, should add unlikely here in favor of non-XDP case? > > > > > > I referred to meta data support of the ixgbe driver. It doesn't use > > "unlikely". > > https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/tree/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c#n2176 > > No need to conform to existing logic when we can improve it... I understand. > Anyway this is a minor point and I'm OK with either. > > Reviewed-by: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> > > -- > Toshiaki Makita > Thank you!
On 2019/04/15 17:06, Yuya Kusakabe wrote: ... >>> @@ -5095,6 +5096,13 @@ static struct sk_buff >>> *skb_reorder_vlan_header(struct sk_buff *skb) ... >>> + meta = skb_metadata_end(skb) - meta_len; >>> + memmove(meta + VLAN_HLEN, meta, meta_len); >>> + }; I just noticed that there is an unnecessary semicolon here. Not sure if checkpatch.pl can detect this, but if you did not use it, please try it. Also it seems your patch is corrupted and tabs are converted into whitespaces. Please check if you can apply the patch by sending your patch to yourself, then send to the list. Thanks, Toshiaki Makita
On Mon, 15 Apr 2019 15:05:36 +0900 Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote: > > diff --git a/net/core/skbuff.c b/net/core/skbuff.c > > index ef2cd5712098..6bc663249c4c 100644 > > --- a/net/core/skbuff.c > > +++ b/net/core/skbuff.c [...] > > @@ -5095,6 +5096,13 @@ static struct sk_buff > > *skb_reorder_vlan_header(struct sk_buff *skb) > > memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb), > > mac_len - VLAN_HLEN - ETH_TLEN); > > } > > + > > + meta_len = skb_metadata_len(skb); > > + if (meta_len) { > > Since this is not used by non-XDP skb and skb path is slow-path for XDP > anyway, should add unlikely here in favor of non-XDP case? I want to stress, that XDP is meant to cooperate with network stack. The XDP metadata is meant as a communication channel between XDP and network-stack SKBs. Thus, there are use-cases where this is not considered a slow-path. That said I don't care if this is marked unlikely(), as not many people are using this metadata communication channel yet. In one customer use-case I have seen, they wanted to pop VLAN tags (Q-in-Q) at XDP layer, but keep some of the info in metadata for TC-bpf hook. I don't think they kept/left any VLAN headers in the SKB. > > + meta = skb_metadata_end(skb) - meta_len; > > + memmove(meta + VLAN_HLEN, meta, meta_len); > > + }; > > +
On 2019/04/15 18:57, Jesper Dangaard Brouer wrote: > On Mon, 15 Apr 2019 15:05:36 +0900 > Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp> wrote: > >>> diff --git a/net/core/skbuff.c b/net/core/skbuff.c >>> index ef2cd5712098..6bc663249c4c 100644 >>> --- a/net/core/skbuff.c >>> +++ b/net/core/skbuff.c > [...] >>> @@ -5095,6 +5096,13 @@ static struct sk_buff >>> *skb_reorder_vlan_header(struct sk_buff *skb) >>> memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb), >>> mac_len - VLAN_HLEN - ETH_TLEN); >>> } >>> + >>> + meta_len = skb_metadata_len(skb); >>> + if (meta_len) { >> >> Since this is not used by non-XDP skb and skb path is slow-path for XDP >> anyway, should add unlikely here in favor of non-XDP case? > > I want to stress, that XDP is meant to cooperate with network stack. > The XDP metadata is meant as a communication channel between XDP and > network-stack SKBs. > > Thus, there are use-cases where this is not considered a slow-path. Ah, that's true. Definitely there are cases where skb-path performance is also important with XDP enabled. Sorry about the confusion. > That said I don't care if this is marked unlikely(), as not many people > are using this metadata communication channel yet. > > In one customer use-case I have seen, they wanted to pop VLAN tags > (Q-in-Q) at XDP layer, but keep some of the info in metadata for TC-bpf > hook. I don't think they kept/left any VLAN headers in the SKB. > > >>> + meta = skb_metadata_end(skb) - meta_len; >>> + memmove(meta + VLAN_HLEN, meta, meta_len); >>> + }; >>> +
diff --git a/net/core/skbuff.c b/net/core/skbuff.c index ef2cd5712098..6bc663249c4c 100644 --- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -5083,7 +5083,8 @@ EXPORT_SYMBOL_GPL(skb_gso_validate_mac_len); static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb) { - int mac_len; + int mac_len, meta_len; + void *meta; if (skb_cow(skb, skb_headroom(skb)) < 0) { kfree_skb(skb); @@ -5095,6 +5096,13 @@ static struct sk_buff *skb_reorder_vlan_header(struct sk_buff *skb) memmove(skb_mac_header(skb) + VLAN_HLEN, skb_mac_header(skb), mac_len - VLAN_HLEN - ETH_TLEN); } + + meta_len = skb_metadata_len(skb); + if (meta_len) { + meta = skb_metadata_end(skb) - meta_len; + memmove(meta + VLAN_HLEN, meta, meta_len); + }; + skb->mac_header += VLAN_HLEN; return skb;