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

On 05/04/2011 10:41 AM, Alexander Duyck wrote:

On 5/4/2011 10:33 AM, Dimitris Michailidis wrote:

quoted

On 05/04/2011 10:24 AM, Ben Hutchings wrote:

quoted

On Wed, 2011-05-04 at 10:09 -0700, Dimitris Michailidis wrote:

quoted

On 05/03/2011 04:34 PM, Ben Hutchings wrote:

quoted

On Tue, 2011-05-03 at 16:23 -0700, Dimitris Michailidis wrote:

quoted

I think RX_CLS_LOC_UNSPEC should be passed to the driver, where 
there is
enough knowledge to pick an appropriate slot.  So I'd remove the

      if (loc == RX_CLS_LOC_UNSPEC)

block above, let the driver pick a slot, and then pass the 
selected location
back for ethtool to report.

But first we have to specify this in the ethtool API.  So please 
propose
a patch to ethtool.h.

In the past we discussed that being able to specify the first 
available slot or
the last available would be useful, so something like the below?

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 4194a20..909ef79 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h

@@ -442,7 +442,8 @@ struct ethtool_flow_ext {
    *    includes the %FLOW_EXT flag.
    * @ring_cookie: RX ring/queue index to deliver to, or 

%RX_CLS_FLOW_DISC
    *    if packets should be discarded
- * @location: Index of filter in hardware table
+ * @location: Index of filter in hardware table, or 
%RX_CLS_FLOW_FIRST_LOC for
+ *    first available index, or %RX_CLS_FLOW_LAST_LOC for last 
available

[...]

I think that's reasonable.  We should also explicitly state that
location determines priority, i.e. if a packet matches two filters then
the one with the lower location wins.

Easy and true for a TCAM.  For hashing would you use the location to 
decide how
to order filters that fall in the same bucket?

The problem is none of this is backwards compatible.  The niu driver has 
supported the network flow classifier rules since 2.6.30.  Adding this 
would cause all rule setups for niu to fail because these locations 
would have to exist outside of the current rule locations.

Looking at niu it already has a problem with its handling of location because 
it does

	u16 idx;

	idx = nfc->fs.location;

i.e., it disregards the upper 16 bits of location.  With the current code niu 
would return -EINVAL for the two new constants, which seems correct.

This is why I was suggesting that the best approach would be to update 
the kernel to add a separate ioctl for letting the driver setup the 
location.  We can just attempt to make that call and when we get the 
EOPNOTSUPP errno we know the device driver doesn't support it and can 
then let the rule manager take over.

The problem with this is the location is dependent on the type of filter being 
added.  I.e., the ioctl would need to get all the information the existing 
ioctl carries making the new ioctl a small superset of the current one.
Additionally, if the driver only allocates a location in a separate ioctl how 
does it know that the app is actually going to use it?