| Message ID | 1298682048.3555.18.camel@localhost |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, Feb 25, 2011 at 5:00 PM, Ben Hutchings <bhutchings@solarflare.com> wrote: > On Fri, 2011-02-25 at 15:32 -0800, Alexander Duyck wrote: >> This change is meant to add an ntuple define type to the rx network flow >> classification specifiers. The idea is to allow ntuple to be displayed and >> possibly configured via the network flow classification interface. To do >> this I added a ntuple_flow_spec_ext to the lsit of supported filters, and >> added a flow_type_ext value to the structure in an unused hole within the >> ethtool_rx_flow_spec structure. > > There's a hole there on 64-bit architectures. Unfortunately, on i386 > and other architectures where u64 is not 64-bit-aligned, there isn't. > We actually need to add compat handling for the commands that use it. > > Also, we don't want these flags to be ignored by older kernel versions > and drivers - they should reject specs that they don't understand. So > any extension flags need to be added to flow_type. > >> Due to the fact that the flow specifier structures are only 4 byte aligned >> instead of 8 I had to break the user data field into 2 sections. In >> addition I added the vlan ethertype field since this is what ixgbe was >> using the user-data for currently and it allows for the fields to stay 4 >> byte aligned while occupying space at the end of the flow_spec. >> >> In order to guarantee byte ordering I also thought it best to keep all >> fields in the flow_spec area a big endian value, as such I added vlan, vlan >> ethertype, and data as big endian values. > > It's not important that byte order is consistent across architectures. > >> Signed-off-by: Alexander Duyck <alexander.h.duyck@intel.com> >> --- >> >> include/linux/ethtool.h | 20 ++++++++++++++++++++ >> 1 files changed, 20 insertions(+), 0 deletions(-) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index aac3e2e..3d1f8e0 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -378,10 +378,25 @@ struct ethtool_usrip4_spec { >> }; >> >> /** >> + * struct ethtool_ntuple_spec_ext - flow spec extension for ntuple in nfc >> + * @unused: space unused by extension >> + * @vlan_etype: EtherType for vlan tagged packet to match >> + * @vlan_tci: VLAN tag to match >> + * @data: Driver-dependent data to match >> + */ >> +struct ethtool_ntuple_spec_ext { >> + __be32 unused[15]; >> + __be16 vlan_etype; >> + __be16 vlan_tci; >> + __be32 data[2]; >> +}; > [...] > > This is a really nasty way to reclaim space in the union. > > Let's name the union, shrink it and insert the extra fields that way: > > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -377,27 +377,43 @@ struct ethtool_usrip4_spec { > __u8 proto; > }; > > +union ethtool_flow_union { > + struct ethtool_tcpip4_spec tcp_ip4_spec; > + struct ethtool_tcpip4_spec udp_ip4_spec; > + struct ethtool_tcpip4_spec sctp_ip4_spec; > + struct ethtool_ah_espip4_spec ah_ip4_spec; > + struct ethtool_ah_espip4_spec esp_ip4_spec; > + struct ethtool_usrip4_spec usr_ip4_spec; > + struct ethhdr ether_spec; > + __u8 hdata[52]; > +}; > + > +struct ethtool_flow_ext { > + __be16 vlan_etype; > + __be16 vlan_tci; > + __be32 data[2]; > + __u32 reserved[2]; > +}; > + Any chance of getting the reserved fields moved to the top of the structure? My only concern is that we might end up with a flow spec larger than 52 bytes at some point and moving the reserved fields to the front might give us a little more wiggle room future compatibility. > /** > * struct ethtool_rx_flow_spec - specification for RX flow filter > * @flow_type: Type of match to perform, e.g. %TCP_V4_FLOW > * @h_u: Flow fields to match (dependent on @flow_type) > + * @h_ext: Additional fields to match > * @m_u: Masks for flow field bits to be ignored > + * @m_ext: Masks for additional field bits to be ignored. > + * Note, all additional fields must be ignored unless @flow_type > + * includes the %FLOW_EXT flag. > * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC > * if packets should be discarded > * @location: Index of filter in hardware table > */ > struct ethtool_rx_flow_spec { > __u32 flow_type; > - union { > - struct ethtool_tcpip4_spec tcp_ip4_spec; > - struct ethtool_tcpip4_spec udp_ip4_spec; > - struct ethtool_tcpip4_spec sctp_ip4_spec; > - struct ethtool_ah_espip4_spec ah_ip4_spec; > - struct ethtool_ah_espip4_spec esp_ip4_spec; > - struct ethtool_usrip4_spec usr_ip4_spec; > - struct ethhdr ether_spec; > - __u8 hdata[72]; > - } h_u, m_u; > + union ethtool_flow_union h_u; > + struct ethtool_flow_ext h_ext; > + union ethtool_flow_union m_u; > + struct ethtool_flow_ext m_ext; > __u64 ring_cookie; > __u32 location; > }; > @@ -954,6 +970,8 @@ struct ethtool_ops { > #define IPV4_FLOW 0x10 /* hash only */ > #define IPV6_FLOW 0x11 /* hash only */ > #define ETHER_FLOW 0x12 /* spec only (ether_spec) */ > +/* Flag to enable additional fields in struct ethtool_rx_flow_spec */ > +#define FLOW_EXT 0x80000000 > > /* L3-L4 network traffic flow hash options */ > #define RXH_L2DA (1 << 1) > --- > > Ben. This works for my purposes other than the one comment above. However if you are fine with it I am good with it since I can't think of any filters that we might need in the near future that would require more than 52 bytes. Acked-by: Alexander Duyck <alexander.h.duyck@intel.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2011-02-25 at 21:30 -0800, Alexander Duyck wrote: > On Fri, Feb 25, 2011 at 5:00 PM, Ben Hutchings > <bhutchings@solarflare.com> wrote: > > On Fri, 2011-02-25 at 15:32 -0800, Alexander Duyck wrote: [...] > >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > >> index aac3e2e..3d1f8e0 100644 > >> --- a/include/linux/ethtool.h > >> +++ b/include/linux/ethtool.h > >> @@ -378,10 +378,25 @@ struct ethtool_usrip4_spec { > >> }; > >> > >> /** > >> + * struct ethtool_ntuple_spec_ext - flow spec extension for ntuple in nfc > >> + * @unused: space unused by extension > >> + * @vlan_etype: EtherType for vlan tagged packet to match > >> + * @vlan_tci: VLAN tag to match > >> + * @data: Driver-dependent data to match > >> + */ > >> +struct ethtool_ntuple_spec_ext { > >> + __be32 unused[15]; > >> + __be16 vlan_etype; > >> + __be16 vlan_tci; > >> + __be32 data[2]; > >> +}; > > [...] > > > > This is a really nasty way to reclaim space in the union. > > > > Let's name the union, shrink it and insert the extra fields that way: > > > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -377,27 +377,43 @@ struct ethtool_usrip4_spec { > > __u8 proto; > > }; > > > > +union ethtool_flow_union { > > + struct ethtool_tcpip4_spec tcp_ip4_spec; > > + struct ethtool_tcpip4_spec udp_ip4_spec; > > + struct ethtool_tcpip4_spec sctp_ip4_spec; > > + struct ethtool_ah_espip4_spec ah_ip4_spec; > > + struct ethtool_ah_espip4_spec esp_ip4_spec; > > + struct ethtool_usrip4_spec usr_ip4_spec; > > + struct ethhdr ether_spec; > > + __u8 hdata[52]; > > +}; > > + > > +struct ethtool_flow_ext { > > + __be16 vlan_etype; > > + __be16 vlan_tci; > > + __be32 data[2]; > > + __u32 reserved[2]; > > +}; > > + > > Any chance of getting the reserved fields moved to the top of the > structure? My only concern is that we might end up with a flow spec > larger than 52 bytes at some point and moving the reserved fields to > the front might give us a little more wiggle room future > compatibility. [...] OK, so how about this: /** * union ethtool_flow_union - flow spec type-specific fields * @tcp_ip4_spec: TCP/IPv4 fields to match * @udp_ip4_spec: UDP/IPv4 fields to match * @sctp_ip4_spec: SCTP/IPv4 fields to match * @ah_ip4_spec: AH/IPv4 fields to match * @esp_ip4_spec: ESP/IPv4 fields to match * @usr_ip4_spec: User-defined IPv4 fields to match * @ether_spec: Ethernet fields to match * * Note: The size of this union may shrink in future to allow for * expansion in &struct ethtool_flow_ext. */ union ethtool_flow_union { struct ethtool_tcpip4_spec tcp_ip4_spec; struct ethtool_tcpip4_spec udp_ip4_spec; struct ethtool_tcpip4_spec sctp_ip4_spec; struct ethtool_ah_espip4_spec ah_ip4_spec; struct ethtool_ah_espip4_spec esp_ip4_spec; struct ethtool_usrip4_spec usr_ip4_spec; struct ethhdr ether_spec; __u8 hdata[60]; }; /** * struct ethtool_flow_ext - flow spec common extension fields * @vlan_etype: EtherType for vlan tagged packet to match * @vlan_tci: VLAN tag to match * @data: Driver-dependent data to match * * Note: Additional fields may be inserted before @vlan_etype in future, * but the offset of the existing fields within the containing structure * (&struct ethtool_rx_flow_spec) will be stable. */ struct ethtool_flow_ext { __be16 vlan_etype; __be16 vlan_tci; __be32 data[2]; }; Please can you check that these definitions won't affect the size of struct ethtool_rx_flow_spec on i386 or x86-64? Ben.
On Wed, 2011-03-02 at 11:11 -0800, Dimitrios Michailidis wrote: > Ben Hutchings wrote: > > /** > > * struct ethtool_flow_ext - flow spec common extension fields > > * @vlan_etype: EtherType for vlan tagged packet to match > > * @vlan_tci: VLAN tag to match > > * @data: Driver-dependent data to match > > * > > * Note: Additional fields may be inserted before @vlan_etype in future, > > * but the offset of the existing fields within the containing structure > > * (&struct ethtool_rx_flow_spec) will be stable. > > */ > > struct ethtool_flow_ext { > > __be16 vlan_etype; > > __be16 vlan_tci; > > __be32 data[2]; > > }; > > I am wondering about the semantics of these vlan_* fields. Is vlan_etype the > Ethertype in the VLAN header or the type after it? It would be the the type in the VLAN tag. The nested ethertype is normally implied by flow_type to be ETH_P_IP. This does leave the question of what this would mean: struct ethtool_rx_flow_spec fs = { .flow_type = ... | FLOW_EXT, ... .h_ext.vlan_tci = htons(0x1234), .m_ext.vlan_etype = 0xffff, }; This says the TCI must be == 0x1234 but the type can be anything. But the type surely has to be be one assigned for use in VLAN tags. Should we leave it to the driver/hardware to determine what those valid types are, or should we reject this as valid? > Specifically, I am wondering > whether these fields are limited to VLANs or can work with general Ethertypes and the > 2 bytes after them. If you set flow_type = ETHER_FLOW you can then specify any combination of Ethernet header fields to match in {h_u,m_u}.ether_spec. However, so far only sfc implements it and it only allows matching on the destination MAC address. Ben.
On 3/2/2011 12:03 PM, Dimitrios Michailidis wrote: > Ben Hutchings wrote: >> On Wed, 2011-03-02 at 11:11 -0800, Dimitrios Michailidis wrote: >>> Ben Hutchings wrote: >>>> /** >>>> * struct ethtool_flow_ext - flow spec common extension fields >>>> * @vlan_etype: EtherType for vlan tagged packet to match >>>> * @vlan_tci: VLAN tag to match >>>> * @data: Driver-dependent data to match >>>> * >>>> * Note: Additional fields may be inserted before @vlan_etype in future, >>>> * but the offset of the existing fields within the containing structure >>>> * (&struct ethtool_rx_flow_spec) will be stable. >>>> */ >>>> struct ethtool_flow_ext { >>>> __be16 vlan_etype; >>>> __be16 vlan_tci; >>>> __be32 data[2]; >>>> }; >>> >>> I am wondering about the semantics of these vlan_* fields. Is vlan_etype the >>> Ethertype in the VLAN header or the type after it? >> >> It would be the the type in the VLAN tag. The nested ethertype is >> normally implied by flow_type to be ETH_P_IP. >> >> This does leave the question of what this would mean: >> >> struct ethtool_rx_flow_spec fs = { >> .flow_type = ... | FLOW_EXT, >> ... >> .h_ext.vlan_tci = htons(0x1234), >> .m_ext.vlan_etype = 0xffff, >> }; >> >> This says the TCI must be == 0x1234 but the type can be anything. But >> the type surely has to be be one assigned for use in VLAN tags. Should >> we leave it to the driver/hardware to determine what those valid types >> are, or should we reject this as valid? > > Right. Devices have some internal rules for what qualifies as a VLAN frame. > If users are given the option to specify vlan_etype what do they get? > At least we need to specify what is expected so drivers can decide if they can support it. The basic idea I had is similar to what Ben described. Basically the vlan_etype can be used to determine the Ethertype for the VLAN to be compared. The reason for this is specifically the VLAN 0 case since without the VLAN Ethertype check VLAN 0 on ixgbe hardware will match an untagged frame and that may not be a desired result. As such we can specify the VLAN Ethertype and then we will only match VLAN tagged frames without any false hits. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3/2/2011 10:50 AM, Ben Hutchings wrote: > On Fri, 2011-02-25 at 21:30 -0800, Alexander Duyck wrote: >> On Fri, Feb 25, 2011 at 5:00 PM, Ben Hutchings >> <bhutchings@solarflare.com> wrote: >>> On Fri, 2011-02-25 at 15:32 -0800, Alexander Duyck wrote: > [...] >>>> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >>>> index aac3e2e..3d1f8e0 100644 >>>> --- a/include/linux/ethtool.h >>>> +++ b/include/linux/ethtool.h >>>> @@ -378,10 +378,25 @@ struct ethtool_usrip4_spec { >>>> }; >>>> >>>> /** >>>> + * struct ethtool_ntuple_spec_ext - flow spec extension for ntuple in nfc >>>> + * @unused: space unused by extension >>>> + * @vlan_etype: EtherType for vlan tagged packet to match >>>> + * @vlan_tci: VLAN tag to match >>>> + * @data: Driver-dependent data to match >>>> + */ >>>> +struct ethtool_ntuple_spec_ext { >>>> + __be32 unused[15]; >>>> + __be16 vlan_etype; >>>> + __be16 vlan_tci; >>>> + __be32 data[2]; >>>> +}; >>> [...] >>> >>> This is a really nasty way to reclaim space in the union. >>> >>> Let's name the union, shrink it and insert the extra fields that way: >>> >>> --- a/include/linux/ethtool.h >>> +++ b/include/linux/ethtool.h >>> @@ -377,27 +377,43 @@ struct ethtool_usrip4_spec { >>> __u8 proto; >>> }; >>> >>> +union ethtool_flow_union { >>> + struct ethtool_tcpip4_spec tcp_ip4_spec; >>> + struct ethtool_tcpip4_spec udp_ip4_spec; >>> + struct ethtool_tcpip4_spec sctp_ip4_spec; >>> + struct ethtool_ah_espip4_spec ah_ip4_spec; >>> + struct ethtool_ah_espip4_spec esp_ip4_spec; >>> + struct ethtool_usrip4_spec usr_ip4_spec; >>> + struct ethhdr ether_spec; >>> + __u8 hdata[52]; >>> +}; >>> + >>> +struct ethtool_flow_ext { >>> + __be16 vlan_etype; >>> + __be16 vlan_tci; >>> + __be32 data[2]; >>> + __u32 reserved[2]; >>> +}; >>> + >> >> Any chance of getting the reserved fields moved to the top of the >> structure? My only concern is that we might end up with a flow spec >> larger than 52 bytes at some point and moving the reserved fields to >> the front might give us a little more wiggle room future >> compatibility. > [...] > > OK, so how about this: > > /** > * union ethtool_flow_union - flow spec type-specific fields > * @tcp_ip4_spec: TCP/IPv4 fields to match > * @udp_ip4_spec: UDP/IPv4 fields to match > * @sctp_ip4_spec: SCTP/IPv4 fields to match > * @ah_ip4_spec: AH/IPv4 fields to match > * @esp_ip4_spec: ESP/IPv4 fields to match > * @usr_ip4_spec: User-defined IPv4 fields to match > * @ether_spec: Ethernet fields to match > * > * Note: The size of this union may shrink in future to allow for > * expansion in&struct ethtool_flow_ext. > */ > union ethtool_flow_union { > struct ethtool_tcpip4_spec tcp_ip4_spec; > struct ethtool_tcpip4_spec udp_ip4_spec; > struct ethtool_tcpip4_spec sctp_ip4_spec; > struct ethtool_ah_espip4_spec ah_ip4_spec; > struct ethtool_ah_espip4_spec esp_ip4_spec; > struct ethtool_usrip4_spec usr_ip4_spec; > struct ethhdr ether_spec; > __u8 hdata[60]; > }; > > /** > * struct ethtool_flow_ext - flow spec common extension fields > * @vlan_etype: EtherType for vlan tagged packet to match > * @vlan_tci: VLAN tag to match > * @data: Driver-dependent data to match > * > * Note: Additional fields may be inserted before @vlan_etype in future, > * but the offset of the existing fields within the containing structure > * (&struct ethtool_rx_flow_spec) will be stable. > */ > struct ethtool_flow_ext { > __be16 vlan_etype; > __be16 vlan_tci; > __be32 data[2]; > }; > > Please can you check that these definitions won't affect the size of > struct ethtool_rx_flow_spec on i386 or x86-64? > > Ben. > I'll try to look into it next week since I am just getting caught up from being out on vacation. As I recall when I had made my original changes they didn't have an effect on the size so this should be fine since all of the fields have a maximum alignment of 32 bits. Thanks, Alex -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -377,27 +377,43 @@ struct ethtool_usrip4_spec { __u8 proto; }; +union ethtool_flow_union { + struct ethtool_tcpip4_spec tcp_ip4_spec; + struct ethtool_tcpip4_spec udp_ip4_spec; + struct ethtool_tcpip4_spec sctp_ip4_spec; + struct ethtool_ah_espip4_spec ah_ip4_spec; + struct ethtool_ah_espip4_spec esp_ip4_spec; + struct ethtool_usrip4_spec usr_ip4_spec; + struct ethhdr ether_spec; + __u8 hdata[52]; +}; + +struct ethtool_flow_ext { + __be16 vlan_etype; + __be16 vlan_tci; + __be32 data[2]; + __u32 reserved[2]; +}; + /** * struct ethtool_rx_flow_spec - specification for RX flow filter * @flow_type: Type of match to perform, e.g. %TCP_V4_FLOW * @h_u: Flow fields to match (dependent on @flow_type) + * @h_ext: Additional fields to match * @m_u: Masks for flow field bits to be ignored + * @m_ext: Masks for additional field bits to be ignored. + * Note, all additional fields must be ignored unless @flow_type + * includes the %FLOW_EXT flag. * @ring_cookie: RX ring/queue index to deliver to, or %RX_CLS_FLOW_DISC * if packets should be discarded * @location: Index of filter in hardware table */ struct ethtool_rx_flow_spec { __u32 flow_type; - union { - struct ethtool_tcpip4_spec tcp_ip4_spec; - struct ethtool_tcpip4_spec udp_ip4_spec; - struct ethtool_tcpip4_spec sctp_ip4_spec; - struct ethtool_ah_espip4_spec ah_ip4_spec; - struct ethtool_ah_espip4_spec esp_ip4_spec; - struct ethtool_usrip4_spec usr_ip4_spec; - struct ethhdr ether_spec; - __u8 hdata[72]; - } h_u, m_u; + union ethtool_flow_union h_u; + struct ethtool_flow_ext h_ext; + union ethtool_flow_union m_u; + struct ethtool_flow_ext m_ext; __u64 ring_cookie; __u32 location; }; @@ -954,6 +970,8 @@ struct ethtool_ops { #define IPV4_FLOW 0x10 /* hash only */ #define IPV6_FLOW 0x11 /* hash only */ #define ETHER_FLOW 0x12 /* spec only (ether_spec) */ +/* Flag to enable additional fields in struct ethtool_rx_flow_spec */ +#define FLOW_EXT 0x80000000 /* L3-L4 network traffic flow hash options */ #define RXH_L2DA (1 << 1)