Re: [ethtool PATCH 2/2] Add RX packet classification interface

On 3/7/2011 7:57 AM, Ben Hutchings wrote:

On Fri, 2011-03-04 at 11:09 -0800, Alexander Duyck wrote:

quoted

On 2/28/2011 4:35 PM, Ben Hutchings wrote:

quoted

On Tue, 2011-02-22 at 12:52 -0800, Alexander Duyck wrote:

[...]

quoted

quoted

quoted

quoted

                                 } else
                                         show_usage(1);
                                 break;

I don't think the same options (-n, -N) should be used both for flow
hashing and n-tuple flow steering/filtering.  This command-line
interface and the structure used in the ethtool API just seem to reflect
the implementation in the niu driver.

(In fact I would much prefer it if the -u and -U options could be used
for both the rxnfc and rxntuple interfaces.  But I haven't thought about
how the differences in functionality would be exposed to or hidden from
the user.)

I was kind of thinking about merging the two interfaces too, but I was
looking at it more from the perspective of moving away from ntuple more
towards this newer interface.  My main motivation being that the filter
display option is so badly broken for ntuple that it would be easier to
make ntuple a subset of the flow classifier instead of the other way around.

What would you think of using the "flow-type" keyword to indicate legacy
ntuple support, and then adding something like "class-rule-add", and
"class-rule-del" to add support for the network flow classifier calls?

I really don't want to introduce different syntax for functionality that
is common between the two command sets.  The user should not have to
know that driver A implements interface I and driver B implements
interface J, except that since version 2.6.y driver A implements
interface J too.

Surely it is possible to try one interface, then the other, when the
requested filter can be implemented either way?

The problem is that the interfaces are different in the way they
implement their masks.  N-tuple defines the mask as 0s mean inclusion,
1s, mean exclusion.

You have got to be kidding me!

If this is the case, then the current kernel-doc for
ethtool_rx_flow_spec::m_u is incorrect.

That would be the case.  The m_u for ethtool_rx_flow_spec is 0 for bits 
to be ignored.  It is one of the things I really liked about that since 
in combination with the way the original patch generated the masks it 
would mean no goofy bit setting workarounds.

I think the documentation was added after the ethtool_rx_flow_spec and 
ethtool_rx_ntuple_flow_spec were and it looks like whoever added it 
probably assumed it worked the same way as ntuple.  I can probably 
submit an updated patch to correct the kernel-doc for that.

quoted

The network flow classifier filters are the exact
opposite.  As such we really need to know which type of filter we are
dealing with before we start setting up values.

This is nonsense; it's not hard to flip the bits later.

Sorry about that I was probably thinking a bit too literally.  So what 
you are suggesting is that I store the values as a 1's compliment of 
what the user set as input for ethtool_rx_flow_spec.  I think I can 
manage that.

quoted

In addition there are
options such as location which exist in network flow classifier rules,
but not in ntuple rules.

Sure, and when those are specified then there can be no fallback.

Ben.

Actually now that I am thinking about it I could probably just ignore 
location for rules that end up being processed via ntuple.

The only time where location really matters is if you are attempting to 
overwrite an existing rule and I am not sure how that would be handled 
in ntuple anyway since right now adding additional rules via ntuple for 
ixgbe just results in duplicate rules being defined.

The idea I have for this now is to just record everything using the 
ethtool_rx_flow_spec.  With it extended to support VLAN and 64 bytes of 
data we should be able to just store everything in the one structure, 
try recording it to the hardware via the nfc interface, if that fails 
then copy the values except for location into a ntuple structure, and 
attempt to write it via the ntuple interface.

Thanks,

Alex