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

On Mon, 2011-03-07 at 09:04 -0800, Alexander Duyck wrote:

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

quoted

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.

I added that documentation.  Since I missed the original rxnfc patch for
ethtool, from which I could have inferred the correct semantics of the
masks, I assumed that they were interrpeted the same as in
ethtool_rx_ntuple_flow_spec.

[...]

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.

As I understand it, the location also determines the *priority* for the
rule.  Which is why I wrote that "@fs.@location specifies the index to
use and must not be ignored."

To support hardware where the filter table is hash-based rather than a
TCAM, we would need some kind of flag or special value of location that
means 'wherever'.

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.

Right.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.