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 = &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
