Messages in this thread | | | Date | Tue, 8 Mar 2022 08:17:31 -0800 | From | Jakub Kicinski <> | Subject | Re: [ovs-dev] [PATCH net-next v8] net: openvswitch: IPv6: Add IPv6 extension header support |
| |
On Tue, 8 Mar 2022 15:12:45 +0100 Ilya Maximets wrote: > >> diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h > >> index 9d1710f20505..ab6755621e02 100644 > >> --- a/include/uapi/linux/openvswitch.h > >> +++ b/include/uapi/linux/openvswitch.h > >> @@ -351,11 +351,16 @@ enum ovs_key_attr { > >> OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV4, /* struct ovs_key_ct_tuple_ipv4 */ > >> OVS_KEY_ATTR_CT_ORIG_TUPLE_IPV6, /* struct ovs_key_ct_tuple_ipv6 */ > >> OVS_KEY_ATTR_NSH, /* Nested set of ovs_nsh_key_* */ > >> - OVS_KEY_ATTR_IPV6_EXTHDRS, /* struct ovs_key_ipv6_exthdr */ > >> > >> #ifdef __KERNEL__ > >> OVS_KEY_ATTR_TUNNEL_INFO, /* struct ip_tunnel_info */ > >> #endif > >> + /* User space decided to squat on types 30 and 31 */ > >> + OVS_KEY_ATTR_IPV6_EXTHDRS = 32, /* struct ovs_key_ipv6_exthdr */ > >> + /* WARNING: <scary warning to avoid the problem coming back> */ > > Yes, that is something that I had in mind too. The only thing that makes > me uncomfortable is OVS_KEY_ATTR_TUNNEL_INFO = 30 here. Even though it > doesn't make a lot of difference, I'd better keep the kernel-only attributes > at the end of the enumeration. Is there a better way to handle kernel-only > attribute?
My thought was to leave the kernel/userspace only types "behind" to avoid perpetuating the same constructs.
Johannes's point about userspace to userspace messages makes the whole thing a little less of an aberration.
Is there a reason for the types to be hidden under __KERNEL__? Or someone did that in a misguided attempt to save space in attr arrays when parsing?
> Also, the OVS_KEY_ATTR_ND_EXTENSIONS (31) attribute used to store IPv6 Neighbor > Discovery extensions is currently implemented only for userspace, but nothing > actually prevents us having the kernel implementation. So, we need a way to > make it usable by the kernel in the future.
The "= 32" leaves the earlier attr types as reserved so nothing prevents us from defining them later. But..
> > It might be nicer to actually document here in what's at least supposed > > to be the canonical documentation of the API what those types were used > > for. > > I agree with that.
Should we add the user space types to the kernel header and remove the ifdef __KERNEL__ around TUNNEL_INFO, then?
| |