Message-ID: <1298939712.2569.43.camel@bwh-desktop>
Date:	Tue, 01 Mar 2011 00:35:12 +0000
From:	Ben Hutchings <bhutchings@...arflare.com>
To:	Alexander Duyck <alexander.h.duyck@...el.com>
Cc:	Santwona Behera <santwona.behera@....com>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: [ethtool PATCH 2/2] Add RX packet classification interface

On Tue, 2011-02-22 at 12:52 -0800, Alexander Duyck wrote:
[...]
> >> @@ -408,6 +425,14 @@ static int msglvl_changed;
> >>   static u32 msglvl_wanted = 0;
> >>   static u32 msglvl_mask = 0;
> >>
> >> +static int rx_rings_get = 0;
> >> +static int rx_class_rule_get = -1;
> >> +static int rx_class_rule_getall = 0;
> >> +static int rx_class_rule_del = -1;
> >> +static int rx_class_rule_added = 0;
> >> +static struct ethtool_rx_flow_spec rx_rule_fs;
> >> +static u8 rxclass_loc_valid = 0;
> >> +
> >>   static enum {
> >>        ONLINE=0,
> >>        OFFLINE,
> > [...]
> >> @@ -945,6 +974,23 @@ static void parse_cmdline(int argc, char **argp)
> >>                                                rxflow_str_to_type(argp[i]);
> >>                                        if (!rx_fhash_get)
> >>                                                show_usage(1);
> >> +                             } else if (!strcmp(argp[i], "rx-rings")) {
> >> +                                     i += 1;
> >> +                                     rx_rings_get = 1;
> >
> > I'm not convinced of the value of a separate rx-rings option/keyword.
> > However it's probably worth displaying the number of rings/queues when
> > showing other flow hashing and steering/filtering information (the -x
> > option does this).
> 
> My thought was that it would be useful for determining the number of 
> rings prior to adding a rule.  Especially if we have any kind of scripts 
> running on top of ethtool so that we can avoid rules that will fail due 
> to ring values being greater than the actual number of rings.  I might 
> try looking into adding it to the display options for the filters.

I think it would also be appropriate to add this to the output of the
-g/--show-ring option.

[...]
> >>                                } 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?

[...]
> >> +     for (i = 2; i<  opt_cnt;) {
> >> +             if (!strcmp(optstr[i], "tos")) {
> >> +                     tos = (u_int8_t)strtoul(optstr[i+1], (char **)NULL,
> >> +                                              0);
> >> +                     tm = 0xff;
> >> +                     fsp->h_u.tcp_ip4_spec.tos = tos;
> >> +
> >> +                     i += 2;
> >> +                     if (opt_cnt>  (i+1)) {
> >> +                             if (!strcmp(optstr[i], "m")) {
> >> +                                     tm = (u_int8_t)strtoul(optstr[i+1],
> >> +                                                            (char **)NULL,
> >> +                                                            16);
> >> +                                     i += 2;
> >> +                             }
> >> +                     }
> >> +                     fsp->m_u.tcp_ip4_spec.tos = tm;
> >> +             } else if (!strcmp(optstr[i], "sip")) {
> > [...]
> >
> > These keyword names must be made consistent with those used for the -U
> > (--config-ntuple) option.
> >
> 
> I will update the names to be consistent with the ntuple options, 
> however I would prefer to keep the option of short-cutting the mask via 
> the "m" value.  It will not be hard to make it support both since the 
> pattern would be to test for either "m" or "%s-mask".
[...]

Agreed, that would be a useful shortcut.

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.

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists