Message-ID: <87371n9p4h.fsf@intel.com>
Date:   Mon, 26 Feb 2018 17:51:10 -0800
From:   Vinicius Costa Gomes <vinicius.gomes@...el.com>
To:     Florian Fainelli <f.fainelli@...il.com>,
        intel-wired-lan@...ts.osuosl.org
Cc:     jeffrey.t.kirsher@...el.com, netdev@...r.kernel.org,
        jesus.sanchez-palencia@...el.com
Subject: Re: [next-queue PATCH 7/8] igb: Add support for adding offloaded clsflower filters

Hi,

Florian Fainelli <f.fainelli@...il.com> writes:

> On 02/26/2018 04:40 PM, Vinicius Costa Gomes wrote:
>> Hi,
>> 
>> Florian Fainelli <f.fainelli@...il.com> writes:
>> 
>>> On February 23, 2018 5:20:35 PM PST, Vinicius Costa Gomes <vinicius.gomes@...el.com> wrote:
>>>> This allows filters added by tc-flower and specifying MAC addresses,
>>>> Ethernet types, and the VLAN priority field, to be offloaded to the
>>>> controller.
>>>>
>>>> This reuses most of the infrastructure used by ethtool, ethtool can be
>>>> used to read these filters, but modification and deletion can only be
>>>> done via tc-flower.
>>>
>>> You would want to check what other drivers supporting both
>>> ethtool::rxnfc and cls_flower do, but this can be seriously confusing
>>> to an user. As an user I would be more comfortable with seeing only
>>> rules added through ethtool via ethtool and those added by cls_flower
>>> via cls_flower. They will both access a shared set of resources but it
>>> seems easier for me to dump rules with both tools to figure out why
>>> -ENOSPC was returned rather than seeing something I did not add.
>>> Others might see it entirely differently.
>> 
>> I took a closer look at mlx5 and i40e, and they seem to use different
>> hardware capabilities (even incompatible in the case of i40e, which
>> returns an error when tring to add cls_flower filter when an ethtool
>> based filter exists) for ethtool and cls_flower. So I don't think the
>> model applies exactly here.
>> 
>> As they keep the filters separated for the user perspective, I could do
>> the same, in the name of convention, it's just that the separation is
>> more "artificial". But I have no strong opinions either way.
>
> True, I would still conform to what these two drivers do since they have
> a large user base (so does igb, but not yet for cls_flower yet since you
> are obviously working on it).

Awesome. Will present them as separate to the user, then.

>
>> 
>>>
>>> If you added the ability for cls_flower to indicate a queue number and
>>> either a fixed rule index or auto-placement (RX_CLS_LOC_ANY), could
>>> that eliminate entirely the need for adding MAC address steering in
>>> earlier patches?
>> 
>> I am not sure that I understand. 'cls_flower' already has support for
>> indicating a queue number (expressed via the 'hw_tc' parameter to tc)
>> (commit 384c181e3780 ("net: sched: Identify hardware traffic classes
>> using classid").
>
> I had missed that cls_flower gained the capability to specify a queue
> number, that's good. What it still does not support AFAICT that ethtool
> does though is either automatically allocating a rule location (Rule ID
> shown by ethtool) or allowing placement at a specific location. This can
> be important when the rule location can be carried by the hardware on
> e.g: a per-packet basis, the hardware that I work with (bcm_sf2_cfp.c)
> makes use of that for instance, maybe this is such an isolated case that
> I should take care of it at some point if I was remotely serious into
> providing tc/cls_flower support for that driver...

Now I am starting to see what you meant about the rules location. In the
igb case of the igb-based controller, only the rule 0 is special (it's
reserved for the local address) which the user wouldn't have no control
over. From what I can see, for the igb driver, the location of rules only
controls the order they are displayed to the user.

>
>> 
>> And adding more control for the allocation of indexes for the rules seem
>> not to help much in reducing the size/complexity of this series. I.e.
>> this series has 4 parts: bug fixes, adding support for source addresses
>> for MAC filters, adding ethtool support MAC address filters (as it was
>> only missing some glue code), and adding offloading for some types of
>> cls_flower filters. More control for the allocation of rule indexes would
>> only affect the cls_flower part.
>> 
>> But perhaps I could be missing something here.
>
> You are absolutely right, it was not so much about trying to reduce the
> complexity rather than avoiding having two user interface facilities:
> ethtool and tc/cls_flower to do essentailly the same thing, yet, having
> some small differences in the offered capabilities, in the case of
> tc/cls_flower, lack of specification of rule location.
> -- 
> Florian


Cheers,
--
Vinicius

Powered by blists - more mailing lists