Re: [PATCH net-next 1/4] flow_dissector: Get vlan info from skb->vlan_tci instead of skb->data
Mon, Aug 15, 2016 at 05:51:38PM CEST, had...@dev.mellanox.co.il wrote: >On Mon, Aug 15, 2016 at 5:38 AM, Toshiaki Makita >wrote: >> On 16/08/14 (日) 23:58, Hadar Hen Zion wrote: >>> >>> On Fri, Aug 12, 2016 at 9:36 AM, Toshiaki Makita >>> wrote: On 2016/08/10 22:32, Hadar Hen Zion wrote: > > Early in the datapath skb_vlan_untag function is called, stripped > the vlan from the skb and set skb->vlan_tci and skb->vlan_proto fields. > > The current dissection doesn't handle vlan packets correctly. Vlan > doesn't exist in skb->data anymore when applying flow dissection on the > skb, fix that. RPS (and flow-dissector called in RPS) is performed before vlan-strip in __netif_receive_skb_core(). >>> >>> >>> right, I'll fix it to v2. >>> Also, in cases skb is tagged with multiple vlan headers (typical when using 802.1ad), the second level vlan tag is in skb->data. >>> >>> >>> Currently, flow_dissector doesn't support multiple vlan headers, only >>> one vlan_id field is present. >>> There aren't any flow_dissector "customers" yet for multiple vlan support. >> >> >> Sure, no need to store second level vlan tag information for now. >> The point is that current flow-dissector correctly skips any number of vlan >> tags and get hash value from IP/TCP/UDP headers, so RPS works for multiple >> vlan tagged packets. >> >> Thanks, >> Toshiaki Makita > >ok, so we are on the same page. >The flow dissector will correctly skip any number of vlans regardless >if the first vlan is stripped or not. On RX the first vlan is always stripped either by hw or by skb_vlan_untag. On TX the first vlan is also stripped as validate_xmit_skb_list which pushes vlan header is called just before dev_hard_start_xmit. So I believe you can safely work just with skb->vlan_*
Re: [PATCH net-next 1/4] flow_dissector: Get vlan info from skb->vlan_tci instead of skb->data
On Mon, Aug 15, 2016 at 5:38 AM, Toshiaki Makitawrote: > On 16/08/14 (日) 23:58, Hadar Hen Zion wrote: >> >> On Fri, Aug 12, 2016 at 9:36 AM, Toshiaki Makita >> wrote: >>> >>> On 2016/08/10 22:32, Hadar Hen Zion wrote: Early in the datapath skb_vlan_untag function is called, stripped the vlan from the skb and set skb->vlan_tci and skb->vlan_proto fields. The current dissection doesn't handle vlan packets correctly. Vlan doesn't exist in skb->data anymore when applying flow dissection on the skb, fix that. >>> >>> >>> RPS (and flow-dissector called in RPS) is performed before vlan-strip in >>> __netif_receive_skb_core(). >> >> >> right, I'll fix it to v2. >> >>> Also, in cases skb is tagged with multiple vlan headers (typical when >>> using 802.1ad), the second level vlan tag is in skb->data. >> >> >> Currently, flow_dissector doesn't support multiple vlan headers, only >> one vlan_id field is present. >> There aren't any flow_dissector "customers" yet for multiple vlan support. > > > Sure, no need to store second level vlan tag information for now. > The point is that current flow-dissector correctly skips any number of vlan > tags and get hash value from IP/TCP/UDP headers, so RPS works for multiple > vlan tagged packets. > > Thanks, > Toshiaki Makita ok, so we are on the same page. The flow dissector will correctly skip any number of vlans regardless if the first vlan is stripped or not. I also found a dependency between my vlan addition to flower and mlx5 tc offload support so I'm working to fix it for V2. Thanks, Hadar
Re: [PATCH net-next 1/4] flow_dissector: Get vlan info from skb->vlan_tci instead of skb->data
On 16/08/14 (日) 23:58, Hadar Hen Zion wrote: On Fri, Aug 12, 2016 at 9:36 AM, Toshiaki Makitawrote: On 2016/08/10 22:32, Hadar Hen Zion wrote: Early in the datapath skb_vlan_untag function is called, stripped the vlan from the skb and set skb->vlan_tci and skb->vlan_proto fields. The current dissection doesn't handle vlan packets correctly. Vlan doesn't exist in skb->data anymore when applying flow dissection on the skb, fix that. RPS (and flow-dissector called in RPS) is performed before vlan-strip in __netif_receive_skb_core(). right, I'll fix it to v2. Also, in cases skb is tagged with multiple vlan headers (typical when using 802.1ad), the second level vlan tag is in skb->data. Currently, flow_dissector doesn't support multiple vlan headers, only one vlan_id field is present. There aren't any flow_dissector "customers" yet for multiple vlan support. Sure, no need to store second level vlan tag information for now. The point is that current flow-dissector correctly skips any number of vlan tags and get hash value from IP/TCP/UDP headers, so RPS works for multiple vlan tagged packets. Thanks, Toshiaki Makita
Re: [PATCH net-next 1/4] flow_dissector: Get vlan info from skb->vlan_tci instead of skb->data
On Fri, Aug 12, 2016 at 9:36 AM, Toshiaki Makitawrote: > On 2016/08/10 22:32, Hadar Hen Zion wrote: >> Early in the datapath skb_vlan_untag function is called, stripped >> the vlan from the skb and set skb->vlan_tci and skb->vlan_proto fields. >> >> The current dissection doesn't handle vlan packets correctly. Vlan >> doesn't exist in skb->data anymore when applying flow dissection on the >> skb, fix that. > > RPS (and flow-dissector called in RPS) is performed before vlan-strip in > __netif_receive_skb_core(). right, I'll fix it to v2. > Also, in cases skb is tagged with multiple vlan headers (typical when > using 802.1ad), the second level vlan tag is in skb->data. Currently, flow_dissector doesn't support multiple vlan headers, only one vlan_id field is present. There aren't any flow_dissector "customers" yet for multiple vlan support. > So I think you should handle both of skb->vlan_tci and skb->data cases. Sure, will do it. > > Thanks, > Toshiaki Makita > >
Re: [PATCH net-next 1/4] flow_dissector: Get vlan info from skb->vlan_tci instead of skb->data
On 2016/08/10 22:32, Hadar Hen Zion wrote: > Early in the datapath skb_vlan_untag function is called, stripped > the vlan from the skb and set skb->vlan_tci and skb->vlan_proto fields. > > The current dissection doesn't handle vlan packets correctly. Vlan > doesn't exist in skb->data anymore when applying flow dissection on the > skb, fix that. RPS (and flow-dissector called in RPS) is performed before vlan-strip in __netif_receive_skb_core(). Also, in cases skb is tagged with multiple vlan headers (typical when using 802.1ad), the second level vlan tag is in skb->data. So I think you should handle both of skb->vlan_tci and skb->data cases. Thanks, Toshiaki Makita
Re: [PATCH net-next 1/4] flow_dissector: Get vlan info from skb->vlan_tci instead of skb->data
Wed, Aug 10, 2016 at 03:32:20PM CEST, had...@mellanox.com wrote: >Early in the datapath skb_vlan_untag function is called, stripped >the vlan from the skb and set skb->vlan_tci and skb->vlan_proto fields. > >The current dissection doesn't handle vlan packets correctly. Vlan >doesn't exist in skb->data anymore when applying flow dissection on the >skb, fix that. > >Fixes: 0744dd00c1b1 ('net: introduce skb_flow_dissect()') >Signed-off-by: Hadar Hen ZionAcked-by: Jiri Pirko
[PATCH net-next 1/4] flow_dissector: Get vlan info from skb->vlan_tci instead of skb->data
Early in the datapath skb_vlan_untag function is called, stripped the vlan from the skb and set skb->vlan_tci and skb->vlan_proto fields. The current dissection doesn't handle vlan packets correctly. Vlan doesn't exist in skb->data anymore when applying flow dissection on the skb, fix that. Fixes: 0744dd00c1b1 ('net: introduce skb_flow_dissect()') Signed-off-by: Hadar Hen Zion--- net/core/flow_dissector.c | 13 +++-- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c index 61ad43f..6060fc2 100644 --- a/net/core/flow_dissector.c +++ b/net/core/flow_dissector.c @@ -122,7 +122,8 @@ bool __skb_flow_dissect(const struct sk_buff *skb, if (!data) { data = skb->data; - proto = skb->protocol; + proto = skb_vlan_tag_present(skb) ? +skb->vlan_proto : skb->protocol; nhoff = skb_network_offset(skb); hlen = skb_headlen(skb); } @@ -240,13 +241,6 @@ ipv6: } case htons(ETH_P_8021AD): case htons(ETH_P_8021Q): { - const struct vlan_hdr *vlan; - struct vlan_hdr _vlan; - - vlan = __skb_header_pointer(skb, nhoff, sizeof(_vlan), data, hlen, &_vlan); - if (!vlan) - goto out_bad; - if (dissector_uses_key(flow_dissector, FLOW_DISSECTOR_KEY_VLANID)) { key_tags = skb_flow_dissector_target(flow_dissector, @@ -256,8 +250,7 @@ ipv6: key_tags->vlan_id = skb_vlan_tag_get_id(skb); } - proto = vlan->h_vlan_encapsulated_proto; - nhoff += sizeof(*vlan); + proto = skb->protocol; goto again; } case htons(ETH_P_PPP_SES): { -- 1.8.3.1