Re: [PATCH net-next,v4 09/12] ethtool: add basic ethtool_rx_flow_spec to flow_rule structure translator
On Wed, Dec 05, 2018 at 02:10:37PM +0100, Pablo Neira Ayuso wrote:
> On Thu, Nov 29, 2018 at 02:12:14PM +0100, Michal Kubecek wrote:
> > On Thu, Nov 29, 2018 at 03:22:28AM +0100, Pablo Neira Ayuso wrote:
> > ...
> > > + act = >rule->action.entries[0];
> > > + switch (fs->ring_cookie) {
> > > + case RX_CLS_FLOW_DISC:
> > > + act->id = FLOW_ACTION_DROP;
> > > + break;
> > > + case RX_CLS_FLOW_WAKE:
> > > + act->id = FLOW_ACTION_WAKE;
> > > + break;
> > > + default:
> > > + act->id = FLOW_ACTION_QUEUE;
> > > + act->queue_index = fs->ring_cookie;
> > > + break;
> > > + }
> >
> > IMHO it would be cleaner if rules passing packets to RSS contexts were
> > also implemented using action, e.g. by using FLOW_ACTION_CONTEXT for
> > act->id and reusing act->queue_index for context id (using either an
> > anonymous union or more generic name).
>
> Are refering to FLOW_RSS specifically? Or just rename in this action
> and field?
My idea is that currently the ethtool interface can apply four types of
action to matching packets
- put into a specific queue
- discard
- distribute accross the queues according to a RSS context
- use the rule as a wake-on-lan filter
What is confusing is that due to lack of extensibility of the ethtool
ioctl interface, third option is implemented by setting FLOW_RSS flag in
ethtool_rxnfc::fs.flow_type and passing the RSS context id in
ethtool_rxnfc::rss_context, i.e. outside struct ethtool_rx_flow_spec.
The context id would need to be passed to the conversion function in
addition to the struct ethtool_rx_flow_spec pointer.
I believe we should use this opportunity to handle this case in a way
which would match the other three. In particular by introducing another
value for act->id (e.g. FLOW_ACTION_CONTEXT) and passing the target
context id in the same structure (e.g. by renaming queue_index to
something more generic or by using an anonymous union with it).
Michal Kubecek
Re: [PATCH net-next,v4 09/12] ethtool: add basic ethtool_rx_flow_spec to flow_rule structure translator
On Thu, Nov 29, 2018 at 03:22:28AM +0100, 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.
>
> This patch also includes support for FLOW_EXT and FLOW_MAC_EXT.
>
> Signed-off-by: Pablo Neira Ayuso
...
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index afd9596ce636..c76d1b34c9a2 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
...
> +
> +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;
> + struct flow_dissector_key_vlan vlan;
> + struct flow_dissector_key_eth_addrs eth_addrs;
> +} __aligned(BITS_PER_LONG / 8); /* Ensure that we can do comparisons as
> longs. */
How about __aligned(sizeof(long)) ?
...
> + act = >rule->action.entries[0];
> + switch (fs->ring_cookie) {
> + case RX_CLS_FLOW_DISC:
> + act->id = FLOW_ACTION_DROP;
> + break;
> + case RX_CLS_FLOW_WAKE:
> + act->id = FLOW_ACTION_WAKE;
> + break;
> + default:
> + act->id = FLOW_ACTION_QUEUE;
> + act->queue_index = fs->ring_cookie;
> + break;
> + }
IMHO it would be cleaner if rules passing packets to RSS contexts were
also implemented using action, e.g. by using FLOW_ACTION_CONTEXT for
act->id and reusing act->queue_index for context id (using either an
anonymous union or more generic name).
Another idea: I would prefer moving this translation from NIC drivers to
the generic ethtool code. That would mean providing new ethtool_ops
callbacks (with a plan to deprecate the old ones). But it's probably
something something to leave for later as there would be a lot of pain
for no actual gain right now. (The reason I'm thinking about this is
that it wouldn't feel right to translate netlink messages to the ethtool
structures only to translate those once again in the NIC driver.)
Michal Kubecek
[PATCH net-next,v4 09/12] ethtool: 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.
This patch also includes support for FLOW_EXT and FLOW_MAC_EXT.
Signed-off-by: Pablo Neira Ayuso
---
v4: Rename to ethtool_rx_flow_rule_create() and ethtool_rx_flow_rule_destroy(),
requested by Jiri Pirko.
Add support for FLOW_EXT and FLOW_MAC_EXT, by Florian Fainelli.
Use BIT() to set .use_keys, also by Florian Fainelli.
Double-check mask field is set when value field is present in
ethtool_rx_flow_rule, requested by Florian Fainelli.
include/linux/ethtool.h | 10 ++
net/core/ethtool.c | 237
2 files changed, 247 insertions(+)
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index afd9596ce636..c76d1b34c9a2 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_create(const struct ethtool_rx_flow_spec *fs);
+void ethtool_rx_flow_rule_destroy(struct ethtool_rx_flow_rule *rule);
+
#endif /* _LINUX_ETHTOOL_H */
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index d05402868575..d25f48acac57 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,239 @@ 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;
+ struct flow_dissector_key_vlan vlan;
+ struct flow_dissector_key_eth_addrs eth_addrs;
+} __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_create(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 ERR_PTR(-ENOMEM);
+
+ /* ethtool_rx supports only one single action per rule. */
+ flow->rule = flow_rule_alloc(1);
+ if (!flow->rule) {
+ kfree(flow);
+ return ERR_PTR(-ENOMEM);
+ }
+
+ match = (struct ethtool_rx_flow_match *)flow->priv;
+ flow->rule->match.dissector = >dissector;
+ flow->rule->match.mask = >mask;
+ flow->rule->match.key = >key;
+
+ match->mask.basic.n_proto = htons(0x);
+
+ switch (fs->flow_type & ~(FLOW_EXT | FLOW_MAC_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 = >h_u.tcp_ip4_spec;
+ v4_m_spec = >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 |=
+ BIT(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;
+
