[dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap

Morten Brørup mb at smartsharesystems.com
Tue Sep 15 11:39:22 CEST 2015


Nelio, Marc,

 

I think Marc is on the right track here: You should design the API based on requirements and usefulness, not based on what is already implemented in some drivers. And as Marc pointed out, this is control plane stuff, so performance should not be an issue.

 

Let me throw in another detail. Read clause 22.2.4.2.13 in the IEEE 802.3 standard very carefully. It says that the Link Status register in the PHY latches a link failure until the Link Status bit has been read. The purpose of this is to make it possible for slowly polling software to detect that the link has been temporarily down, even though it has come up again. Why would the application need to know that the link has been temporarily down, when it is already up again? Because the link might have come up in another speed/duplex, auto negotiation or flow control mode than it was before.

 

And while I’m at it: Remember the flow control (PAUSE) operations. It is important to be able to enable/disable flow control – even cheap “web managed” switches have the ability to disable flow control.

 

 

Med venlig hilsen / kind regards

- Morten Brørup

 

From: Marc Sune [mailto:marcdevel at gmail.com] 
Sent: 15. september 2015 10:48
To: Nélio Laranjeiro
Cc: Morten Brørup; Thomas Monjalon; dev at dpdk.org; Olga Shern; Adrien Mazarguil
Subject: Re: [dpdk-dev] [PATCH v4 0/2] ethdev: add port speed capability bitmap

 

I will answer Morten in another mail, because I got his point on the AUTONEG as a separate bit, and it _makes_ sense to me.

 

But Neilo,

 

2015-09-15 10:25 GMT+02:00 Nélio Laranjeiro <nelio.laranjeiro at 6wind.com>:

On Tue, Sep 15, 2015 at 12:50:11AM +0200, Morten Brørup wrote:
> Comments inline, marked MB>.
>
> Med venlig hilsen / kind regards
> - Morten Brørup
>
> Marc Sune <marcdevel at gmail.com> on 14. september 2015 23:34 wrote:
>
> 2015-09-14 12:52 GMT+02:00 Morten Brørup <mb at smartsharesystems.com>:
> > It is important to consider that a multipath link (bonding etc.) is not a physical link, but a logical link (built on top of multiple physical links). Regardless whether it is a Layer2 link aggregate (IEEE 802.1ad, Ethernet bonding, EtherChannel, DSL pair bonding, etc.) or a Layer3 multipath link (e.g. simultaneously using Wi-Fi and mobile networks). So it doesn't make sense trying to impose physical link properties on a purely logical link. Likewise, it doesn't make sense to impose logical link properties on physical links. In other words: Don't consider bonding or any other logical link types when designing the PHY API.
>
> +1

+1.

>  
>
> > I think there is consensus that 1/ (PHY capabilities) and 2/ (PHY advertisements) should use the same definitions, specifically a bitmap field. And when you disregard bonding, I don't see any reason to use different definitions for 3/ (PHY negotiation result). This makes it one unified API for all three purposes.
>
> Agree.

I don't agree with this one, some PMDs don't use the advertise of
autoneg result to get the speed or the duplex.  You make a
generality from your case above all PMDs.

 

can you please explain how a particular PMD is recovering the actual link speed and the duplex has to do with the design of the (general) API?

 

	
	Mellanox get the speed, duplex and status information from IOCTLs
	which are not related to your bitmap.  So at least for this PMD, there
	is already a conversion from 3 fields to a bitmap, knowing that it will
	use the speed as an integer after.  What is the benefit of your solution?

 

I said already I don't have a strong preference for 3/. But steering the design of an API through a "minimum common denominator" principle is not a good idea, specially since we are talking about a super simple mapping issue for this specific PMD.

 

	
	> > Nelio suggested adding a support function to convert the bitmap field to a speed value as an integer. I strongly support this, because you cannot expect the bitmap to be ordered by speed.
	>
	> Agree with Nelio&you. This is useful.
	
	It was exactly the extreme opposite, a function which takes a
	rte_eth_link to a bitmap i.e. speed_to_bm (rte_eth_link link) because,
	the speed is mostly used as an integer and not some kind of bitmap.
	
	> > This support function will be able to determine which speed is higher when exotic speeds are added to the bitmap. Please extend this conversion function to give three output parameters: speed, full/half duplex, auto negotiation/non-auto negotiation, or add two separate functions to get the duplex and auto-negotiation.
	>
	> Since, Full/Half duplex is for legacy 10/100Mbps only (afaik), I have my doubts on using a bit for all speeds. I would suggest to define (unroll) 100M (or 100M_FD) and 100M_HD, and the same 10Mbps/1gbps, as Thomas was suggesting some mails ago.
	>
	> This was done in v4 (implicitely 100M == 100M_FD). See below.
	>  
	> MB> I didn't intend two bits to be allocated in the bitmap for all speeds to support full/half duplex, only for the relevant speeds. Since full duplex is dominant, I agree with the previous decision (originally suggested by Thomas, I think) to make full duplex implicit unless half duplex is explicitly specified. E.g. 10M_HD, 10M (alias 10M_FD), 100M_HD, 100M (alias 100M_FD), 1000M (or 1G), 2500M, 10G, 40G, 100G, etc.
	>
	>
	> > I haven't read the suggested code, but there should be some means in 2/ (advertisements) to disable auto negotiation, e.g. a single bit in the bitmap to indicate if the speed/duplex-indicating bits in the bitmap means forced speed/duplex (in which case only a single speed/duplex-bit should be set) or auto negotiation advertised speed/duplex (in which case multiple speed/duplex-bits can be set).
	>
	> Agree.
	>
	> v3/4 of this patch adds the bitmap in the advertised, as per discussed, to select a group of speeds This is not implemented by drivers yet (!).
	>
	> So, as of v4 of this patch, there could be: a) autoneg any supported speed (=> bitmap == 0) b) autoneg over group of speeds (=> more than one bit set in the bitmap) c) forced speed (one and only one set in the bitmap).
	>
	> I think this is precisely what you meant + b) as a bonus
	>
	> MB> This was not what I meant, but it wasn't very clearly written, so I'll try again: Add an additional single bit "NO_AUTONEG" (or whatever you want to name it) to the 2/ (advertisements) bitmap that explicitly turns off auto negotiation and tries to force the selected speed/duplex (i.e. only one other bit can be set in the bitmap when the NO_AUTONEG bit is set). Your c) makes it impossible to use auto negotiation to advertise a specific speed/duplex, e.g. 100M_FD. My suggested NO_AUTONEG bit can also be used in 3/ (result) to indicate that the speed was a result of Parallel Detection, i.e. that auto negotiation failed or was disabled in either end of the link.
	>
	> MB> However, I like your suggestion a).
	>
	>  
	> > And some means in 3/ (result) and maybe 2/ (advertisements) to select and/or indicate physical interface in dual-personality ports (e.g. ports where the PHY has both an SFP and a RJ45 connector, but only one of the two can be used at any time).
	>
	> For rte_eth_link_get() I don't have such a strong opinion. You either
	>
	> * encode the link speed and duplex as of now, separating duplex and numeric speed. I would suggest to add the encoded speed+duplex bitmap flag for consistency (although redundant).
	> * or you return a single value, the bitmap with a single flag set of the unrolled speeds, and then have the helpers int rte_eth_speed_from_bm(int val_bm) and bool rte_eth_duplex_from_bm(int val_bm).
	>
	> MB> I prefer the latter of the two, only because it makes 3/ (result) consistent with 1/ (capabilities) and 2/ (advertisements).
	
	So I agree for 1/ capabilities and 2/ advertisements.
	
	But, I don't agree to modify rte_eth_link_get API
	(and rte_eth_link structure) thus 3/ result.
	We don't need a "consistent" result, we need something usable.  This is
	not the case of the bitmap and using some conversion functions.
	Remember that the speed and duplex will not change until the next link
	down and there is a lot of code using speeds as integers.
	Your solution will just increase the number of instruction to get the
	same result, is that a benefit?

 

So do you think we should care about roughly 10cycles (at very most) which is what a unique switch case mapping will take? We are not talking about the dataplane here, Neilo.

 

The benefit that Morten is arguing here is to have a unified API, consistent for the user. Once more, I don't have a preference, though I see what he means. I am not sure if the bitmap for retrieving the link is really more usable than the current API, which is IMHO what should steer the discussion, not performance.

 

marc

 

	
	In addition, some PMDs need the speed to make some stuff with it,
	so this structure will be replicated all over DPDK.
	
	--
	Nélio Laranjeiro
	6WIND

 



More information about the dev mailing list