Message-ID: <DB8PR04MB57858C5B15C75CEA34FF5FFAF0049@DB8PR04MB5785.eurprd04.prod.outlook.com>
Date:   Tue, 15 Nov 2022 03:01:24 +0000
From:   Xiaoliang Yang <xiaoliang.yang_1@....com>
To:     Vladimir Oltean <vladimir.oltean@....com>
CC:     Vinicius Costa Gomes <vinicius.gomes@...el.com>,
        "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

Hi Vladimir,

Thanks for your reply, I comment below and make a summarize at the end.

On Mon, Nov 14, 2022 Vladimir wrote:
> On Thu, Nov 10, 2022 at 10:33:50AM +0000, Xiaoliang Yang wrote:
> > 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.
> 
> "Enable" and "activate" are different things only if verification is used.
> Otherwise, they mean the same thing.
Yes, I know that "Enable" and "activate" are different things. I mean that if we
want to enable the next lldp exchange verification process, we need to enable
it but not let the MAC merge layer working until lldp verify successfully.
There are two verification procedure, one is LLDP verification, based on chapter
"99.4.2 Determining that the link partner supports preemption" of 802.3 spec.
This need lldp exchange and ensure preemption is enabled on remote port, and
this verification doesn’t have parameter to enable/disable it. I just want to add
this feature. Another is SMD-r/v verification, this is hardware verification and
already has "verify_disable" to enable/disable it.

Or we can enable by default through LLDP. That don’t need a new "active"
parameter. Once the LLDP application has verified that remote port supports
preemption, it will enable preemption on local port, and tell remote port to
enable it. If we use ethtool to enable preemption, the preemption will be force
enabled without LLDP verification.


> 
> In turn, LLDP does not control whether verification is used or not. So, from the
> perspective of the LLDP daemon, you can't "enable" but "not activate"
> preemption.
Yes, LLDP does not control whether hardware verification is used or not. Maybe I
shouldn't use the word "active", I mean preemption can't be working before LLDP
verification, even though it's enabled by ethtool.

> 
> And I wouldn't expect the LLDP flow to require use of ethtool.
If LLDP application can’t require to use ethtool driver, how to enable the preemption
After LLDP verification?

> 
> And I think that enabling preemption at step 1 is too early. Why enable it, if we
> don't know if the link partner supports it?
Same as mentioned above. Enabling at step 1 does not actually enable preemption,
it means enable the LLDP verification.

> 
> >  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.
> 
> Verification as defined by 802.3 is, as mentioned, independent of the
> Additional Ethernet Capabilities TLV.
> Which seems to be a good thing, considering that 2 link partners A and B don't
> need to have coordinated settings for verification (verify_disable doesn't need
> to be in sync). Any device with the MAC Merge layer turned on is required to
> send SMD-R (response) frames in response to SMD-V (verification), regardless
> of whether it sends SMD-V frames of its own.
> 
> So those who enable verification should get a response from those who don't
> enable it, and those who don't enable verification don't cause the link partners
> who do enable it to fail their own verification.
The hardware may not obey this rule. I have tested on LS1028a and i.mx8mp boards,
The verification needs to be enabled at the same time for link partners. So it needs
LLDP verification, and let LLDP application to enable both partner ports at the same
time(The hardware verify three times, and each verify time is 10ms in default).

> 
> The only thing to agree on is this: if the link partner supports preemption, and
> you support preemption, then how does the LLDP daemon decide whether to
> actually *enable* preemption? My assumption is that it's fine to
> unconditionally enable it as long as it's supported.
> No configuration or user override is needed.
Yes, I also consider this way. Enable the preemption once LLDP exchange ensure
that both partner support it. But how to enable preemption via LLDP daemon?
I tried to use the ioctl API of ethtool to enable preemption. Do you have any
suggestion?

> 
> FWIW, I have my own patch on the openlldp project (yes, not lldpd) which adds
> support for the MAC Merge layer. I hope I can wrap everything up some time
> next week and resend everything in a state that reflects what I've been cooking
> in the meantime. I don't have a solution for the adminStatus yet, but I guess I'll
> try to add it via the netlink interface of tc-taprio and tc-mqprio.
It's a good news, I can keep up with upstream patches after you upstream the patches.

> 
> >  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.
> 
> What sense does it make to "force" preemption as active? What would this do
> in terms of driving the internal state machines of the hardware, i.e.
> force FP active in which way? It's active when enabled and verification is either
> disabled, or enabled and complete.
This is not mean active status of SMD-r/v verification. It means let preemption
Working.

> 
> > 	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"
> 
> Control the LLDP exchange via kernel UAPI? But why? What difference does it
> make to the kernel whether there is an LLDP daemon or not? LLDP frames are
> normal packets sent via PF_PACKET sockets, no special driver involvement
> needed. Not the same thing as SMD-V/SMD-R hardware verification.
I mean that if user want to enable the preemption without LLDP. It's not needed
If we use the second way I mentioned above.

I will summarize my opinion. Enable the preemption by default through LLDP.
Once the LLDP application has verified that remote port supports preemption,
it will enable preemption on local port, and tell remote port to enable it.
If user want to force enable preemption, use ethtool to enable it directly.
The lldp daemon use the ioctl API of ethtool to enable preemption.

Thanks,
Xiaoliang

Powered by blists - more mailing lists