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
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.
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.
Le 01/28/17 à 07:55, Jiri Pirko a écrit :
Sat, Jan 28, 2017 at 03:19:04PM CET, idosch@mellanox.com wrote:
quoted
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:)
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.
quoted
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.
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