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

[Marc Sune <marcdevel@gmail.com>]

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@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@gmail.com> on 14. september 2015 23:34 wrote:
> >
> > 2015-09-14 12:52 GMT+02:00 Morten Brørup <mb@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
>