Re: [PATCH net-next,v3 09/12] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator
Thu, Nov 22, 2018 at 05:59:27AM CET, f.faine...@gmail.com wrote: > > >On 11/20/2018 6:51 PM, Pablo Neira Ayuso wrote: >> This patch adds a function to translate the ethtool_rx_flow_spec >> structure to the flow_rule representation. >> >> This allows us to reuse code from the driver side given that both flower >> and ethtool_rx_flow interfaces use the same representation. >> >> Signed-off-by: Pablo Neira Ayuso >> --- >> v3: Suggested by Jiri Pirko: >> - Add struct ethtool_rx_flow_rule, keep placeholder to private >> dissector information. >> Reported by Manish Chopra: >> - Fix incorrect dissector user_keys flags. >> >> include/linux/ethtool.h | 10 +++ >> net/core/ethtool.c | 189 >> >> 2 files changed, 199 insertions(+) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index afd9596ce636..99849e0858b2 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -400,4 +400,14 @@ struct ethtool_ops { >> void(*get_ethtool_phy_stats)(struct net_device *, >> struct ethtool_stats *, u64 *); >> }; >> + >> +struct ethtool_rx_flow_rule { >> +struct flow_rule*rule; >> +unsigned long priv[0]; > >This forces you to cast to/from that member, any reason this is just not >a void *priv here? The reason is single alloc call for rule and its priv. Quite usual along the code. >-- >Florian
Re: [PATCH net-next,v3 09/12] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator
Thu, Nov 22, 2018 at 05:57:31AM CET, f.faine...@gmail.com wrote: > > >On 11/20/2018 6:51 PM, Pablo Neira Ayuso wrote: >> This patch adds a function to translate the ethtool_rx_flow_spec >> structure to the flow_rule representation. >> >> This allows us to reuse code from the driver side given that both flower >> and ethtool_rx_flow interfaces use the same representation. >> >> Signed-off-by: Pablo Neira Ayuso >> --- >> v3: Suggested by Jiri Pirko: >> - Add struct ethtool_rx_flow_rule, keep placeholder to private >> dissector information. >> Reported by Manish Chopra: >> - Fix incorrect dissector user_keys flags. >> >> include/linux/ethtool.h | 10 +++ >> net/core/ethtool.c | 189 >> >> 2 files changed, 199 insertions(+) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index afd9596ce636..99849e0858b2 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -400,4 +400,14 @@ struct ethtool_ops { >> void(*get_ethtool_phy_stats)(struct net_device *, >> struct ethtool_stats *, u64 *); >> }; >> + >> +struct ethtool_rx_flow_rule { >> +struct flow_rule*rule; >> +unsigned long priv[0]; >> +}; >> + >> +struct ethtool_rx_flow_rule * >> +ethtool_rx_flow_rule_alloc(const struct ethtool_rx_flow_spec *fs); >> +void ethtool_rx_flow_rule_free(struct ethtool_rx_flow_rule *rule); >> + >> #endif /* _LINUX_ETHTOOL_H */ >> diff --git a/net/core/ethtool.c b/net/core/ethtool.c >> index d05402868575..e679d6478371 100644 >> --- a/net/core/ethtool.c >> +++ b/net/core/ethtool.c >> @@ -28,6 +28,7 @@ >> #include >> #include >> #include >> +#include >> >> /* >> * Some useful ethtool_ops methods that're device independent. >> @@ -2808,3 +2809,191 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) >> >> return rc; >> } >> + >> +struct ethtool_rx_flow_key { >> +struct flow_dissector_key_basic basic; >> +union { >> +struct flow_dissector_key_ipv4_addrsipv4; >> +struct flow_dissector_key_ipv6_addrsipv6; >> +}; >> +struct flow_dissector_key_ports tp; >> +struct flow_dissector_key_ipip; >> +} __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as >> longs. */ >> + >> +struct ethtool_rx_flow_match { >> +struct flow_dissector dissector; >> +struct ethtool_rx_flow_key key; >> +struct ethtool_rx_flow_key mask; >> +}; >> + >> +struct ethtool_rx_flow_rule * >> +ethtool_rx_flow_rule_alloc(const struct ethtool_rx_flow_spec *fs) > >This is more than alloc, it's allocate and map, no reason to split the >two operations AFAICT, but the name could be improved, how about >alloc_from()? Or ethtool_rx_flow_rule_create() and ethtool_rx_flow_rule_destroy() > >> +{ >> +static struct in6_addr zero_addr = {}; >> +struct ethtool_rx_flow_match *match; >> +struct ethtool_rx_flow_rule *flow; >> +struct flow_action_entry *act; >> + >> +flow = kzalloc(sizeof(struct ethtool_rx_flow_rule) + >> + sizeof(struct ethtool_rx_flow_match), GFP_KERNEL); >> +if (!flow) >> +return NULL; >> + >> +/* ethtool_rx supports only one single action per rule. */ >> +flow->rule = flow_rule_alloc(1); >> +if (!flow->rule) { >> +kfree(flow); >> +return NULL; >> +} >> + >> +match = (struct ethtool_rx_flow_match *)flow->priv; >> +flow->rule->match.dissector = &match->dissector; >> +flow->rule->match.mask = &match->mask; >> +flow->rule->match.key = &match->key; >> + >> +match->mask.basic.n_proto = 0x; >> + >> +switch (fs->flow_type & ~FLOW_EXT) { >> +case TCP_V4_FLOW: >> +case UDP_V4_FLOW: { >> +const struct ethtool_tcpip4_spec *v4_spec, *v4_m_spec; >> + >> +match->key.basic.n_proto = htons(ETH_P_IP); >> + >> +v4_spec = &fs->h_u.tcp_ip4_spec; >> +v4_m_spec = &fs->m_u.tcp_ip4_spec; >> + >> +if (v4_m_spec->ip4src) { >> +match->key.ipv4.src = v4_spec->ip4src; >> +match->mask.ipv4.src = v4_m_spec->ip4src; >> +} >> +if (v4_m_spec->ip4dst) { >> +match->key.ipv4.dst = v4_spec->ip4dst; >> +match->mask.ipv4.dst = v4_m_spec->ip4dst; >> +} > >I got confused a while ago between the ethtool ntuple and nfc semantics, >and I can't remember if the following is true: > >- bits set to 1 indicate a match and bit set to 0 indicate a don't care >for nfc >- bits set to 0 indicate a match and bit set to 1 indicate a don't care >for ntuple > >Depending on the answer that could mean that this check on a zero >address may have to change. > >> +if (v4_m_spec->ip4src || >> +v4_m_spec->ip4dst)
Re: [PATCH net-next,v3 09/12] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator
On 11/20/2018 6:51 PM, Pablo Neira Ayuso wrote: > This patch adds a function to translate the ethtool_rx_flow_spec > structure to the flow_rule representation. > > This allows us to reuse code from the driver side given that both flower > and ethtool_rx_flow interfaces use the same representation. > > Signed-off-by: Pablo Neira Ayuso > --- > v3: Suggested by Jiri Pirko: > - Add struct ethtool_rx_flow_rule, keep placeholder to private > dissector information. > Reported by Manish Chopra: > - Fix incorrect dissector user_keys flags. > > include/linux/ethtool.h | 10 +++ > net/core/ethtool.c | 189 > > 2 files changed, 199 insertions(+) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index afd9596ce636..99849e0858b2 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -400,4 +400,14 @@ struct ethtool_ops { > void(*get_ethtool_phy_stats)(struct net_device *, >struct ethtool_stats *, u64 *); > }; > + > +struct ethtool_rx_flow_rule { > + struct flow_rule*rule; > + unsigned long priv[0]; This forces you to cast to/from that member, any reason this is just not a void *priv here? -- Florian
Re: [PATCH net-next,v3 09/12] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator
On 11/20/2018 6:51 PM, Pablo Neira Ayuso wrote: > This patch adds a function to translate the ethtool_rx_flow_spec > structure to the flow_rule representation. > > This allows us to reuse code from the driver side given that both flower > and ethtool_rx_flow interfaces use the same representation. > > Signed-off-by: Pablo Neira Ayuso > --- > v3: Suggested by Jiri Pirko: > - Add struct ethtool_rx_flow_rule, keep placeholder to private > dissector information. > Reported by Manish Chopra: > - Fix incorrect dissector user_keys flags. > > include/linux/ethtool.h | 10 +++ > net/core/ethtool.c | 189 > > 2 files changed, 199 insertions(+) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index afd9596ce636..99849e0858b2 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -400,4 +400,14 @@ struct ethtool_ops { > void(*get_ethtool_phy_stats)(struct net_device *, >struct ethtool_stats *, u64 *); > }; > + > +struct ethtool_rx_flow_rule { > + struct flow_rule*rule; > + unsigned long priv[0]; > +}; > + > +struct ethtool_rx_flow_rule * > +ethtool_rx_flow_rule_alloc(const struct ethtool_rx_flow_spec *fs); > +void ethtool_rx_flow_rule_free(struct ethtool_rx_flow_rule *rule); > + > #endif /* _LINUX_ETHTOOL_H */ > diff --git a/net/core/ethtool.c b/net/core/ethtool.c > index d05402868575..e679d6478371 100644 > --- a/net/core/ethtool.c > +++ b/net/core/ethtool.c > @@ -28,6 +28,7 @@ > #include > #include > #include > +#include > > /* > * Some useful ethtool_ops methods that're device independent. > @@ -2808,3 +2809,191 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) > > return rc; > } > + > +struct ethtool_rx_flow_key { > + struct flow_dissector_key_basic basic; > + union { > + struct flow_dissector_key_ipv4_addrsipv4; > + struct flow_dissector_key_ipv6_addrsipv6; > + }; > + struct flow_dissector_key_ports tp; > + struct flow_dissector_key_ipip; > +} __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as > longs. */ > + > +struct ethtool_rx_flow_match { > + struct flow_dissector dissector; > + struct ethtool_rx_flow_key key; > + struct ethtool_rx_flow_key mask; > +}; > + > +struct ethtool_rx_flow_rule * > +ethtool_rx_flow_rule_alloc(const struct ethtool_rx_flow_spec *fs) This is more than alloc, it's allocate and map, no reason to split the two operations AFAICT, but the name could be improved, how about alloc_from()? > +{ > + static struct in6_addr zero_addr = {}; > + struct ethtool_rx_flow_match *match; > + struct ethtool_rx_flow_rule *flow; > + struct flow_action_entry *act; > + > + flow = kzalloc(sizeof(struct ethtool_rx_flow_rule) + > +sizeof(struct ethtool_rx_flow_match), GFP_KERNEL); > + if (!flow) > + return NULL; > + > + /* ethtool_rx supports only one single action per rule. */ > + flow->rule = flow_rule_alloc(1); > + if (!flow->rule) { > + kfree(flow); > + return NULL; > + } > + > + match = (struct ethtool_rx_flow_match *)flow->priv; > + flow->rule->match.dissector = &match->dissector; > + flow->rule->match.mask = &match->mask; > + flow->rule->match.key = &match->key; > + > + match->mask.basic.n_proto = 0x; > + > + switch (fs->flow_type & ~FLOW_EXT) { > + case TCP_V4_FLOW: > + case UDP_V4_FLOW: { > + const struct ethtool_tcpip4_spec *v4_spec, *v4_m_spec; > + > + match->key.basic.n_proto = htons(ETH_P_IP); > + > + v4_spec = &fs->h_u.tcp_ip4_spec; > + v4_m_spec = &fs->m_u.tcp_ip4_spec; > + > + if (v4_m_spec->ip4src) { > + match->key.ipv4.src = v4_spec->ip4src; > + match->mask.ipv4.src = v4_m_spec->ip4src; > + } > + if (v4_m_spec->ip4dst) { > + match->key.ipv4.dst = v4_spec->ip4dst; > + match->mask.ipv4.dst = v4_m_spec->ip4dst; > + } I got confused a while ago between the ethtool ntuple and nfc semantics, and I can't remember if the following is true: - bits set to 1 indicate a match and bit set to 0 indicate a don't care for nfc - bits set to 0 indicate a match and bit set to 1 indicate a don't care for ntuple Depending on the answer that could mean that this check on a zero address may have to change. > + if (v4_m_spec->ip4src || > + v4_m_spec->ip4dst) { > + match->dissector.used_keys |= > + (1 << FLOW_DISSECTOR_KEY_IPV4_ADDRS); Can you use BIT() here (and likewise for every one below). [snip] > + > + return flow; Wh
[PATCH net-next,v3 09/12] flow_dissector: add basic ethtool_rx_flow_spec to flow_rule structure translator
This patch adds a function to translate the ethtool_rx_flow_spec structure to the flow_rule representation. This allows us to reuse code from the driver side given that both flower and ethtool_rx_flow interfaces use the same representation. Signed-off-by: Pablo Neira Ayuso --- v3: Suggested by Jiri Pirko: - Add struct ethtool_rx_flow_rule, keep placeholder to private dissector information. Reported by Manish Chopra: - Fix incorrect dissector user_keys flags. include/linux/ethtool.h | 10 +++ net/core/ethtool.c | 189 2 files changed, 199 insertions(+) diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index afd9596ce636..99849e0858b2 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -400,4 +400,14 @@ struct ethtool_ops { void(*get_ethtool_phy_stats)(struct net_device *, struct ethtool_stats *, u64 *); }; + +struct ethtool_rx_flow_rule { + struct flow_rule*rule; + unsigned long priv[0]; +}; + +struct ethtool_rx_flow_rule * +ethtool_rx_flow_rule_alloc(const struct ethtool_rx_flow_spec *fs); +void ethtool_rx_flow_rule_free(struct ethtool_rx_flow_rule *rule); + #endif /* _LINUX_ETHTOOL_H */ diff --git a/net/core/ethtool.c b/net/core/ethtool.c index d05402868575..e679d6478371 100644 --- a/net/core/ethtool.c +++ b/net/core/ethtool.c @@ -28,6 +28,7 @@ #include #include #include +#include /* * Some useful ethtool_ops methods that're device independent. @@ -2808,3 +2809,191 @@ int dev_ethtool(struct net *net, struct ifreq *ifr) return rc; } + +struct ethtool_rx_flow_key { + struct flow_dissector_key_basic basic; + union { + struct flow_dissector_key_ipv4_addrsipv4; + struct flow_dissector_key_ipv6_addrsipv6; + }; + struct flow_dissector_key_ports tp; + struct flow_dissector_key_ipip; +} __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as longs. */ + +struct ethtool_rx_flow_match { + struct flow_dissector dissector; + struct ethtool_rx_flow_key key; + struct ethtool_rx_flow_key mask; +}; + +struct ethtool_rx_flow_rule * +ethtool_rx_flow_rule_alloc(const struct ethtool_rx_flow_spec *fs) +{ + static struct in6_addr zero_addr = {}; + struct ethtool_rx_flow_match *match; + struct ethtool_rx_flow_rule *flow; + struct flow_action_entry *act; + + flow = kzalloc(sizeof(struct ethtool_rx_flow_rule) + + sizeof(struct ethtool_rx_flow_match), GFP_KERNEL); + if (!flow) + return NULL; + + /* ethtool_rx supports only one single action per rule. */ + flow->rule = flow_rule_alloc(1); + if (!flow->rule) { + kfree(flow); + return NULL; + } + + match = (struct ethtool_rx_flow_match *)flow->priv; + flow->rule->match.dissector = &match->dissector; + flow->rule->match.mask = &match->mask; + flow->rule->match.key = &match->key; + + match->mask.basic.n_proto = 0x; + + switch (fs->flow_type & ~FLOW_EXT) { + case TCP_V4_FLOW: + case UDP_V4_FLOW: { + const struct ethtool_tcpip4_spec *v4_spec, *v4_m_spec; + + match->key.basic.n_proto = htons(ETH_P_IP); + + v4_spec = &fs->h_u.tcp_ip4_spec; + v4_m_spec = &fs->m_u.tcp_ip4_spec; + + if (v4_m_spec->ip4src) { + match->key.ipv4.src = v4_spec->ip4src; + match->mask.ipv4.src = v4_m_spec->ip4src; + } + if (v4_m_spec->ip4dst) { + match->key.ipv4.dst = v4_spec->ip4dst; + match->mask.ipv4.dst = v4_m_spec->ip4dst; + } + if (v4_m_spec->ip4src || + v4_m_spec->ip4dst) { + match->dissector.used_keys |= + (1 << FLOW_DISSECTOR_KEY_IPV4_ADDRS); + match->dissector.offset[FLOW_DISSECTOR_KEY_IPV4_ADDRS] = + offsetof(struct ethtool_rx_flow_key, ipv4); + } + if (v4_m_spec->psrc) { + match->key.tp.src = v4_spec->psrc; + match->mask.tp.src = v4_m_spec->psrc; + } + if (v4_m_spec->pdst) { + match->key.tp.dst = v4_spec->pdst; + match->mask.tp.dst = v4_m_spec->pdst; + } + if (v4_m_spec->psrc || + v4_m_spec->pdst) { + match->dissector.used_keys |= + (1 << FLOW_DISSECTOR_KEY_PORTS); + match->dissector.offset[FLOW_DISSECTOR_KEY_PORTS] = +