cls_matchall and port mirroring questions

cls_matchall and port mirroring questions

[Florian Fainelli]

Hi,

As I am adding support for cls_matchall in the b53/bcm_sf2 drivers, I
was looking into several, yet unrelated things:

- mlxsw does not seem to specify whether the port used for capture
remains usable, or blocks non-mirror traffic ingressing/egressing it, do
we want a control knob for that? If not, what is a sensible default,
block all non capture traffic?

- do we have an updated man page for tc-matchall.8 that features how to
use the statistical sampler too? b53 switches have a divider that allows
us to select how many frames we want to receive (10 bit value).

- b53 supports capture against a particular MAC SA or DA (or both), do
we want to be able to control that somehow? What about Marvell switches,
what can they do?

-  a fair amount of code dealing with the cls_matchall mirroring entry
is not switch driver specific, in fact, the only things that are switch
driver specific are:
	- list pointer where to store this entry (typically in the private
network device context)
	- operation to check whether the device belongs to us (identical
netdev_ops)
	- retrieval of the destination port number (to_port) which is also
typically available in network device private context

Do we want to move a fair amount of code into switchdev, treat
cls_matchall entries as a specific switchdev object, and have drivers
take over at the same level that mlxsw_sp_port_add_cls_matchall_mirror()
currently starts?

Thanks!
-- 
Florian

Re: cls_matchall and port mirroring questions

[Ido Schimmel]

On Thu, Jan 26, 2017 at 07:00:50PM -0800, Florian Fainelli wrote:
> Hi,
> 
> As I am adding support for cls_matchall in the b53/bcm_sf2 drivers, I
> was looking into several, yet unrelated things:
> 
> - mlxsw does not seem to specify whether the port used for capture
> remains usable, or blocks non-mirror traffic ingressing/egressing it, do
> we want a control knob for that? If not, what is a sensible default,
> block all non capture traffic?

Doesn't make sense to me to add such a default. It's up to the user.

> - do we have an updated man page for tc-matchall.8 that features how to
> use the statistical sampler too? b53 switches have a divider that allows
> us to select how many frames we want to receive (10 bit value).

https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=5c5670fae43027778e84b9d9ff3b9d91a10a8131

Yotam (Cced) already commented that he intends to send iproute patches.

> - b53 supports capture against a particular MAC SA or DA (or both), do
> we want to be able to control that somehow?

Can't you just use flower (for example) instead of matchall in that
case?

> What about Marvell switches, what can they do?

No idea :)

> -  a fair amount of code dealing with the cls_matchall mirroring entry
> is not switch driver specific, in fact, the only things that are switch
> driver specific are:
> 	- list pointer where to store this entry (typically in the private
> network device context)
> 	- operation to check whether the device belongs to us (identical
> netdev_ops)
> 	- retrieval of the destination port number (to_port) which is also
> typically available in network device private context
> 
> Do we want to move a fair amount of code into switchdev, treat
> cls_matchall entries as a specific switchdev object, and have drivers
> take over at the same level that mlxsw_sp_port_add_cls_matchall_mirror()
> currently starts?

I prefer the current way in which we re-use as many as possible core
APIs without adding switchdev-specific code. I don't have a concrete
argument against your proposal, though.

Re: cls_matchall and port mirroring questions

[Jiri Pirko]

Sat, Jan 28, 2017 at 03:19:04PM CET, idosch@mellanox.com wrote:
>On Thu, Jan 26, 2017 at 07:00:50PM -0800, Florian Fainelli wrote:
>> Hi,
>> 
>> As I am adding support for cls_matchall in the b53/bcm_sf2 drivers, I
>> was looking into several, yet unrelated things:
>> 
>> - mlxsw does not seem to specify whether the port used for capture
>> remains usable, or blocks non-mirror traffic ingressing/egressing it, do
>> we want a control knob for that? If not, what is a sensible default,
>> block all non capture traffic?
>
>Doesn't make sense to me to add such a default. It's up to the user.
>
>> - do we have an updated man page for tc-matchall.8 that features how to
>> use the statistical sampler too? b53 switches have a divider that allows
>> us to select how many frames we want to receive (10 bit value).
>
>https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=5c5670fae43027778e84b9d9ff3b9d91a10a8131
>
>Yotam (Cced) already commented that he intends to send iproute patches.
>
>> - b53 supports capture against a particular MAC SA or DA (or both), do
>> we want to be able to control that somehow?
>
>Can't you just use flower (for example) instead of matchall in that
>case?

Definitelly. No extensions to matchall, as otherwise it could not be
called like that anylonger:)


>
>> What about Marvell switches, what can they do?
>
>No idea :)
>
>> -  a fair amount of code dealing with the cls_matchall mirroring entry
>> is not switch driver specific, in fact, the only things that are switch
>> driver specific are:
>> 	- list pointer where to store this entry (typically in the private
>> network device context)
>> 	- operation to check whether the device belongs to us (identical
>> netdev_ops)
>> 	- retrieval of the destination port number (to_port) which is also
>> typically available in network device private context
>> 
>> Do we want to move a fair amount of code into switchdev, treat
>> cls_matchall entries as a specific switchdev object, and have drivers
>> take over at the same level that mlxsw_sp_port_add_cls_matchall_mirror()
>> currently starts?
>
>I prefer the current way in which we re-use as many as possible core
>APIs without adding switchdev-specific code. I don't have a concrete
>argument against your proposal, though.

This (tc-offload) is completely unrelated to switchdev. So it would make
no sense.

Re: cls_matchall and port mirroring questions

[Florian Fainelli]

Le 01/28/17 à 07:55, Jiri Pirko a écrit :
> Sat, Jan 28, 2017 at 03:19:04PM CET, idosch@mellanox.com wrote:
>> On Thu, Jan 26, 2017 at 07:00:50PM -0800, Florian Fainelli wrote:
>>> Hi,
>>>
>>> As I am adding support for cls_matchall in the b53/bcm_sf2 drivers, I
>>> was looking into several, yet unrelated things:
>>>
>>> - mlxsw does not seem to specify whether the port used for capture
>>> remains usable, or blocks non-mirror traffic ingressing/egressing it, do
>>> we want a control knob for that? If not, what is a sensible default,
>>> block all non capture traffic?
>>
>> Doesn't make sense to me to add such a default. It's up to the user.
>>
>>> - do we have an updated man page for tc-matchall.8 that features how to
>>> use the statistical sampler too? b53 switches have a divider that allows
>>> us to select how many frames we want to receive (10 bit value).
>>
>> https://git.kernel.org/cgit/linux/kernel/git/davem/net-next.git/commit/?id=5c5670fae43027778e84b9d9ff3b9d91a10a8131
>>
>> Yotam (Cced) already commented that he intends to send iproute patches.
>>
>>> - b53 supports capture against a particular MAC SA or DA (or both), do
>>> we want to be able to control that somehow?
>>
>> Can't you just use flower (for example) instead of matchall in that
>> case?
> 
> Definitelly. No extensions to matchall, as otherwise it could not be
> called like that anylonger:)

OK, that make sense. I thought of matchall as action that would solely
deal with all port mirroring features, but using flower on top of an
ingress or egress qdisc + src/dst MAC would definitively and I guess
make senses too.

> 
> 
>>
>>> What about Marvell switches, what can they do?
>>
>> No idea :)
>>
>>> -  a fair amount of code dealing with the cls_matchall mirroring entry
>>> is not switch driver specific, in fact, the only things that are switch
>>> driver specific are:
>>> 	- list pointer where to store this entry (typically in the private
>>> network device context)
>>> 	- operation to check whether the device belongs to us (identical
>>> netdev_ops)
>>> 	- retrieval of the destination port number (to_port) which is also
>>> typically available in network device private context
>>>
>>> Do we want to move a fair amount of code into switchdev, treat
>>> cls_matchall entries as a specific switchdev object, and have drivers
>>> take over at the same level that mlxsw_sp_port_add_cls_matchall_mirror()
>>> currently starts?
>>
>> I prefer the current way in which we re-use as many as possible core
>> APIs without adding switchdev-specific code. I don't have a concrete
>> argument against your proposal, though.
> 
> This (tc-offload) is completely unrelated to switchdev. So it would make
> no sense.

That is true, there is a bit of code (allocation of tc entries, parsing
of actions list etc.) that could be made generic; and one could argue
that each tc action programming request could look like some kind of
special switchdev object. I am totally fine keeping things the way they
are though.

Thanks!
-- 
Florian