Re: cls_matchall and port mirroring questions

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:

quoted

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.

quoted

- 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.

quoted

- 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:)

quoted

What about Marvell switches, what can they do?

No idea :)

quoted

-  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.