Message-ID: <DB8PR04MB57855557502DDC669A470E6FF0019@DB8PR04MB5785.eurprd04.prod.outlook.com>
Date:   Thu, 10 Nov 2022 10:33:50 +0000
From:   Xiaoliang Yang <xiaoliang.yang_1@....com>
To:     Vladimir Oltean <vladimir.oltean@....com>,
        Vinicius Costa Gomes <vinicius.gomes@...el.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Michal Kubecek <mkubecek@...e.cz>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Kurt Kanzenbach <kurt@...utronix.de>,
        Rui Sousa <rui.sousa@....com>,
        Ferenc Fejes <ferenc.fejes@...csson.com>,
        Richie Pearn <richard.pearn@....com>
Subject: 回复: [RFC PATCH net-next 2/7] net: ethtool: add support for Frame Preemption and MAC Merge layer

On Sep 15, 2022, Vladimir wrote:
> > That's a good point (your understanding of the flow is similar to mine).
> > This seems a good idea. But we may have to wait for a bit until we
> > have a LLDP implementation that supports this.
> 
> This is circular; we have a downstream patch to openlldp that adds support for
> this TLV, but it can't go anywhere until there is mainline kernel support.
> 
> What about splitting out MAC merge support from FP support, and me
> concentrating on the MAC merge layer for now? They're independent up to a
> pretty high level. The MAC merge layer is supposed to be controlled by the
> LLDP daemon and be pretty much plug-and-play, while the FP adminStatus is
> supposed to be set by some high level administrator, like a NETCONF client.
I agree that splitting out MAC merge support from FP support, they are defined
by different spec. I'm trying to add LLDP exchange verification support. The
procedure is like following:
 1. enable preemption on local port by using "ethtool", do not active preemption.
 2. Decode the LLDP TLV of remote port and ensure the remote port supports
and enables preemption.
 3. Run SMD-v/r verify process on local port or active the preemption directly.
 4. Send updated LLDP TLV to remote port.
 5. Disable preemption on local port and repeat step 1-4 when link down/up.
The struct "ethtool_mm_cfg" seems not fit this procedure. I update it:
struct ethtool_mm_cfg {
	u32 verify_time;
	bool verify_disable;
	bool enabled;
+	bool active; // Used to active or start verify preemption by LLDP daemon.
	u8 add_frag_size;
};
If we want to enable/disable the LLDP exchange verification, maybe we need
to add more parameters like "bool lldp_verify_enable"

> 
> We can argue some more about how to best expose the FP adminStatus.
> Right now, I'm thinking that for all intents and purposes, the adminStatus
> really only makes practical sense when we have at least 2 traffic classes: one
> onto which we can map the preemptable priorities, and one onto which we
> can map the express ones. In turn, it means that the 'priorities collapsing into
> the same traffic class' problem that we have when we delete the
> taprio/mqprio qdisc can be solved if we put the FP adminStatus into the same
> netlink interface that also configures the prio:tc map and the number of traffic
> classes (i.e. yes, in tc).
> 
> I'll let someone more knowledgeable of our sysrepo-tsn implementation of a
> NETCONF server (like Xiaoliang, maybe even Rui or Richie?)
> https://github.com/real-time-edge-sw/real-time-edge-sysrepo
> comment on whether this would create any undesirable side effect.
> The implication is that user space, when asked to change the adminStatus, will
> have to figure out whether we are using a taprio, or an mqprio qdisc, and
> create different netlink attributes to talk to the kernel to request that
> configuration. User space will also have to send an error towards the NETCONF
> client if no such qdisc exists (meaning that the user wants to combine priorities
> of different express/preemptable values in the same traffic class, TC0). The
> qdiscs will also centrally validate whether such invalid mixed configurations
> are requested.
The Netconf yang model haven't defined the mac preemption feature of 802.3br,
but it defines the 802.1Qbu preemption. It uses priority to configure the
adminStatus of frame-preemption-status-table. The priority map traffic class is
independent in Netconf, so there is no effect whether we combine FP status
configuration with tc-mqprio or not, just translated it in Kernel is OK.

> 
> I'm mostly OK with this: an UAPI where FP adminStatus is per priority, but it
> gets translated into per-tc by the qdisc, and passed to the driver through
> ndo_setup_tc(). I am certainly still very much against the "preemptable queue
> mask" that you proposed, and IMO you'll have to do something and clarify
> what a traffic class even means for Intel hardware, especially in relationship to
> multiple queues per tc (mqprio queues 2@1).
> 
> Anyone else who has an opinion of any sort, please feel free.
I didn't think about it too much, but I think combined with tc-mqprio command
is more easy to implement, this eliminates the priority of translating to TC.
Example:
tc qdisc add dev eth0 root handle 1: mqprio num_tc 8 map 0 1 2 3 4 5 6 7 \
	fp-prios 0xff

Powered by blists - more mailing lists