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:

quoted

On Thu, Nov 29, 2018 at 03:22:28AM +0100, Pablo Neira Ayuso wrote:
...

quoted

+	act = &flow->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