| Message ID | 20210208113810.11118-1-hxseverything@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF |
| Headers | show |
| Series | bpf: in bpf_skb_adjust_room correct inner protocol for vxlan | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Guessed tree name to be net-next |
| netdev/subject_prefix | warning | Target tree name not specified in the subject |
| netdev/cc_maintainers | warning | 8 maintainers not CCed: yhs@fb.com kafai@fb.com ast@kernel.org songliubraving@fb.com john.fastabend@gmail.com kpsingh@kernel.org kuba@kernel.org andrii@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 45 this patch: 45 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 13 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 45 this patch: 45 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Mon, Feb 8, 2021 at 7:16 AM huangxuesen <hxseverything@gmail.com> wrote: > > From: huangxuesen <huangxuesen@kuaishou.com> > > When pushing vxlan tunnel header, set inner protocol as ETH_P_TEB in skb > to avoid HW device disabling udp tunnel segmentation offload, just like > vxlan_build_skb does. > > Drivers for NIC may invoke vxlan_features_check to check the > inner_protocol in skb for vxlan packets to decide whether to disable > NETIF_F_GSO_MASK. Currently it sets inner_protocol as the original > skb->protocol, that will make mlx5_core disable TSO and lead to huge > performance degradation. > > Signed-off-by: huangxuesen <huangxuesen@kuaishou.com> > Signed-off-by: chengzhiyong <chengzhiyong@kuaishou.com> > Signed-off-by: wangli <wangli09@kuaishou.com> > --- > net/core/filter.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 255aeee72402..f8d3ba3fe10f 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -3466,7 +3466,12 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, > skb->inner_mac_header = inner_net - inner_mac_len; > skb->inner_network_header = inner_net; > skb->inner_transport_header = inner_trans; > - skb_set_inner_protocol(skb, skb->protocol); > + > + if (flags & BPF_F_ADJ_ROOM_ENCAP_L4_UDP && > + inner_mac_len == ETH_HLEN) > + skb_set_inner_protocol(skb, htons(ETH_P_TEB)); This may be used by vxlan, but it does not imply it. Adding ETH_HLEN bytes likely means pushing an Ethernet header, but same point. Conversely, pushing an Ethernet header is not limited to UDP encap. This probably needs a new explicit BPF_F_ADJ_ROOM_.. flag, rather than trying to infer from imprecise heuristics.
Appreciate for your reply Willem! The original intention of this commit is that when we use bpf_skb_adjust_room to encapsulate Vxlan packets, we find some powerful device features disabled. Setting the inner_protocol directly as skb->protocol is the root cause. I understand that it’s not easy to handle all tunnel protocol in one bpf helper function. But for my immature idea, when pushing Ethernet header, setting the inner_protocol as ETH_P_TEB may be better. Now the flag BPF_F_ADJ_ROOM_ENCAP_L4_UDP includes many udp tunnel types( e.g. udp+mpls, geneve, vxlan). Adding an independent flag to represents Vxlan looks a little reduplicative. What’s your suggestion? Thanks again for your reply! > 2021年2月8日 下午9:06，Willem de Bruijn <willemdebruijn.kernel@gmail.com> 写道： > > On Mon, Feb 8, 2021 at 7:16 AM huangxuesen <hxseverything@gmail.com> wrote: >> >> From: huangxuesen <huangxuesen@kuaishou.com> >> >> When pushing vxlan tunnel header, set inner protocol as ETH_P_TEB in skb >> to avoid HW device disabling udp tunnel segmentation offload, just like >> vxlan_build_skb does. >> >> Drivers for NIC may invoke vxlan_features_check to check the >> inner_protocol in skb for vxlan packets to decide whether to disable >> NETIF_F_GSO_MASK. Currently it sets inner_protocol as the original >> skb->protocol, that will make mlx5_core disable TSO and lead to huge >> performance degradation. >> >> Signed-off-by: huangxuesen <huangxuesen@kuaishou.com> >> Signed-off-by: chengzhiyong <chengzhiyong@kuaishou.com> >> Signed-off-by: wangli <wangli09@kuaishou.com> >> --- >> net/core/filter.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/net/core/filter.c b/net/core/filter.c >> index 255aeee72402..f8d3ba3fe10f 100644 >> --- a/net/core/filter.c >> +++ b/net/core/filter.c >> @@ -3466,7 +3466,12 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, >> skb->inner_mac_header = inner_net - inner_mac_len; >> skb->inner_network_header = inner_net; >> skb->inner_transport_header = inner_trans; >> - skb_set_inner_protocol(skb, skb->protocol); >> + >> + if (flags & BPF_F_ADJ_ROOM_ENCAP_L4_UDP && >> + inner_mac_len == ETH_HLEN) >> + skb_set_inner_protocol(skb, htons(ETH_P_TEB)); > > This may be used by vxlan, but it does not imply it. > > Adding ETH_HLEN bytes likely means pushing an Ethernet header, but same point. > > Conversely, pushing an Ethernet header is not limited to UDP encap. > > This probably needs a new explicit BPF_F_ADJ_ROOM_.. flag, rather than > trying to infer from imprecise heuristics.
On Tue, Feb 9, 2021 at 5:41 AM 黄学森 <hxseverything@gmail.com> wrote: > > Appreciate for your reply Willem! > > The original intention of this commit is that when we use bpf_skb_adjust_room to encapsulate > Vxlan packets, we find some powerful device features disabled. > > Setting the inner_protocol directly as skb->protocol is the root cause. > > I understand that it’s not easy to handle all tunnel protocol in one bpf helper function. But for my > immature idea, when pushing Ethernet header, setting the inner_protocol as ETH_P_TEB may > be better. > > Now the flag BPF_F_ADJ_ROOM_ENCAP_L4_UDP includes many udp tunnel types( e.g. > udp+mpls, geneve, vxlan). Adding an independent flag to represents Vxlan looks a little > reduplicative. What’s your suggestion? Agreed. I don't mean to add a vxlan specific flag. Instead, a way to identify that the encapsulation includes a mac header. To a certain extent, that already exists as of commit 58dfc900faff ("bpf: add layer 2 encap support to bpf_skb_adjust_room"). That computes an inner_maclen. It makes sense that inner_protocol needs to be updated if inner_maclen indicates a mac header. I would only not infer it based on some imprecise measure, such as inner_maclen being 14. But add a new explicit flag BPF_F_ADJ_ROOM_ENCAP_L2_ETH. Update inner protocol if the flag is passed and inner_maclen >= ETH_HLEN. Fail the operation if the flag is passed and inner_maclen is too short. > Thanks again for your reply! > > > > > 2021年2月8日 下午9:06，Willem de Bruijn <willemdebruijn.kernel@gmail.com> 写道： > > > > On Mon, Feb 8, 2021 at 7:16 AM huangxuesen <hxseverything@gmail.com> wrote: > >> > >> From: huangxuesen <huangxuesen@kuaishou.com> > >> > >> When pushing vxlan tunnel header, set inner protocol as ETH_P_TEB in skb > >> to avoid HW device disabling udp tunnel segmentation offload, just like > >> vxlan_build_skb does. > >> > >> Drivers for NIC may invoke vxlan_features_check to check the > >> inner_protocol in skb for vxlan packets to decide whether to disable > >> NETIF_F_GSO_MASK. Currently it sets inner_protocol as the original > >> skb->protocol, that will make mlx5_core disable TSO and lead to huge > >> performance degradation. > >> > >> Signed-off-by: huangxuesen <huangxuesen@kuaishou.com> > >> Signed-off-by: chengzhiyong <chengzhiyong@kuaishou.com> > >> Signed-off-by: wangli <wangli09@kuaishou.com> > >> --- > >> net/core/filter.c | 7 ++++++- > >> 1 file changed, 6 insertions(+), 1 deletion(-) > >> > >> diff --git a/net/core/filter.c b/net/core/filter.c > >> index 255aeee72402..f8d3ba3fe10f 100644 > >> --- a/net/core/filter.c > >> +++ b/net/core/filter.c > >> @@ -3466,7 +3466,12 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, > >> skb->inner_mac_header = inner_net - inner_mac_len; > >> skb->inner_network_header = inner_net; > >> skb->inner_transport_header = inner_trans; > >> - skb_set_inner_protocol(skb, skb->protocol); > >> + > >> + if (flags & BPF_F_ADJ_ROOM_ENCAP_L4_UDP && > >> + inner_mac_len == ETH_HLEN) > >> + skb_set_inner_protocol(skb, htons(ETH_P_TEB)); > > > > This may be used by vxlan, but it does not imply it. > > > > Adding ETH_HLEN bytes likely means pushing an Ethernet header, but same point. > > > > Conversely, pushing an Ethernet header is not limited to UDP encap. > > > > This probably needs a new explicit BPF_F_ADJ_ROOM_.. flag, rather than > > trying to infer from imprecise heuristics. >
diff --git a/net/core/filter.c b/net/core/filter.c index 255aeee72402..f8d3ba3fe10f 100644 --- a/net/core/filter.c +++ b/net/core/filter.c @@ -3466,7 +3466,12 @@ static int bpf_skb_net_grow(struct sk_buff *skb, u32 off, u32 len_diff, skb->inner_mac_header = inner_net - inner_mac_len; skb->inner_network_header = inner_net; skb->inner_transport_header = inner_trans; - skb_set_inner_protocol(skb, skb->protocol); + + if (flags & BPF_F_ADJ_ROOM_ENCAP_L4_UDP && + inner_mac_len == ETH_HLEN) + skb_set_inner_protocol(skb, htons(ETH_P_TEB)); + else + skb_set_inner_protocol(skb, skb->protocol); skb->encapsulation = 1; skb_set_network_header(skb, mac_len);