Message-ID: <20191118083624.GA2149@splinter>
Date:   Mon, 18 Nov 2019 10:36:24 +0200
From:   Ido Schimmel <idosch@...sch.org>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     andrew@...n.ch, f.fainelli@...il.com, vivien.didelot@...il.com,
        davem@...emloft.net, jiri@...lanox.com,
        jakub.kicinski@...ronome.com, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net-next] net: dsa: sja1105: Make HOSTPRIO a devlink
 param

On Sat, Nov 16, 2019 at 07:23:25PM +0200, Vladimir Oltean wrote:
> Unfortunately with this hardware, there is no way to transmit in-band
> QoS hints with management frames (i.e. VLAN PCP is ignored). The traffic
> class for these is fixed in the static config (which in turn requires a
> reset to change).
> 
> With the new ability to add time gates for individual traffic classes,
> there is a real danger that the user might unknowingly turn off the
> traffic class for PTP, BPDUs, LLDP etc.
> 
> So we need to manage this situation the best we can. There isn't any
> knob in Linux for this, so create a driver-specific devlink param which
> is a runtime u8. The default value is 7 (the highest priority traffic
> class).
> 
> Patch is largely inspired by the mv88e6xxx ATU_hash devlink param
> implementation.
> 
> Signed-off-by: Vladimir Oltean <olteanv@...il.com>
> ---
> Changes in v2:
> Turned the NET_DSA_SJA1105_HOSTPRIO kernel config into a "hostprio"
> runtime devlink param.
> 
>  .../networking/devlink-params-sja1105.txt     |  9 ++
>  Documentation/networking/dsa/sja1105.rst      | 19 +++-
>  MAINTAINERS                                   |  1 +
>  drivers/net/dsa/sja1105/sja1105.h             |  1 +
>  drivers/net/dsa/sja1105/sja1105_main.c        | 94 +++++++++++++++++++
>  5 files changed, 122 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/networking/devlink-params-sja1105.txt
> 
> diff --git a/Documentation/networking/devlink-params-sja1105.txt b/Documentation/networking/devlink-params-sja1105.txt
> new file mode 100644
> index 000000000000..5096a4cf923c
> --- /dev/null
> +++ b/Documentation/networking/devlink-params-sja1105.txt
> @@ -0,0 +1,9 @@
> +hostprio		[DEVICE, DRIVER-SPECIFIC]
> +			Configure the traffic class which will be used for
> +			management (link-local) traffic injected and trapped
> +			to/from the CPU. This includes STP, PTP, LLDP etc, as
> +			well as hardware-specific meta frames with RX
> +			timestamps.  Higher is better as long as you care about
> +			your PTP frames.

Vladimir,

I have some concerns about this. Firstly, I'm not sure why you need to
expose this and who do you expect to be able to configure this? I'm
asking because once you expose it to users there might not be a way
back. mlxsw is upstream for over four years and the traffic classes for
the different packet types towards the CPU are hard coded in the driver
and based on "sane" defaults. It is therefore surprising to me that you
already see the need to expose this.

Secondly, I find the name confusing. You call it "hostprio", but the
description says "traffic class". These are two different things.
Priority is a packet attribute based on which you can classify the
packet to a transmission queue (traffic class). And if you have multiple
transmission queues towards the CPU, how do you configure their
scheduling and AQM? This relates to my next point.

Thirdly, the fact that "there isn't any knob in Linux for this" does not
mean that we should not create a proper one. As I see it, the CPU port
(switch side, not DSA master) is a port like any other and therefore
should have a netdev. With a netdev you can properly configure its
different QoS attributes using tc instead of introducing driver-specific
devlink-params.

Yes, I understand that this is a large task compared to just adding this
devlink-param, but adding a new user interface always scares me and I
think we should do the right thing here.

Thanks

Powered by blists - more mailing lists