Re: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 17 Jul 2017 19:12:01 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:

> On 07/17/2017 06:58 PM, Stephen Hemminger wrote:
> > On Sun, 16 Jul 2017 16:26:06 +0300
> > Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> >  
> >> On 07/14/2017 09:30 PM, Stephen Hemminger wrote:  
> >>> Many drivers are all doing copy/paste of the same code to atomicly
> >>> update the link status. Reduce duplication, and allow for future
> >>> changes by having common function for this.
> >>>
> >>> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> >>> ---
> >>>    lib/librte_ether/rte_ethdev.c | 36 ++++++++++++++++++++++++++++++++++++
> >>>    lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> >>>    2 files changed, 64 insertions(+)
> >>>
> >>> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> >>> index a1b744704f3a..7532fc6b65f0 100644
> >>> --- a/lib/librte_ether/rte_ethdev.c
> >>> +++ b/lib/librte_ether/rte_ethdev.c
> >>> @@ -1332,6 +1332,42 @@ rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *eth_link)
> >>>    }
> >>>    
> >>>    int
> >>> +_rte_eth_link_update(struct rte_eth_dev *dev,
> >>> +		    const struct rte_eth_link *link)
> >>> +{
> >>> +	volatile struct rte_eth_link *dev_link = &(dev->data->dev_link);
> >>> +	struct rte_eth_link old;
> >>> +
> >>> +	RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> >>> +
> >>> +	old = *dev_link;
> >>> +
> >>> +	/* Only reason we use cmpset rather than set is
> >>> +	 * that on some architecture may use sign bit as a flag value.  
> >> May I ask to provide more details here.  
> >
> > rte_atomic64_set() takes an int64 argument.
> > This code (taken from ixgbe, virtio and other drivers) uses cmpset
> > to allow using uint64_t.
> >
> > My assumption is that some architecture in the past was using the
> > sign bit a a lock value or something. On 64 bit no special support
> > for 64bit atomic assignment is necessary. Not sure how this code
> > got inherited that way.  
> 
> Many thanks. May be it would be useful in the comment as well.

Maybe one of the original developers could clarify.
It would be cleaner just to do rte_atomcic64_set(), it might just
be a leftover semantic from Linux/BSD/??? where the original developer
was looking.

> 
> >>> +	 */
> >>> +	while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> >>> +				    *(volatile uint64_t *)dev_link,
> >>> +				   *(const uint64_t *)link) == 0)  
> >> Shouldn't it be:
> >> do {
> >>         old = *dev_link;
> >> } while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> >> *(uint64_t *)&old, *(const uint64_t *)link) == 0);
> >>
> >> At least it has some sense to guarantee transition from old to new
> >> talking below comparison into account.  
> > Since dev_link is volatile, the compiler is required to refetch
> > the pointer every time it evaluates the expression. Maybe clearer
> > to alias devlink to a volatile uint64_t ptr.  
> 
> I meant that dev_link value may change after old value saved in original 
> patch,
> but before cmpset which actually replaces dev_link value here. As the result
> two _rte_eth_link_update() run in parallel changing to the same value 
> may return
> "changes done", but actually only one did the job.
> I'm not sure if it is really important here, since requirements are not 
> clear.

Since there is no locking here. There can not be a guarantee of ordering possible.
The only guarantee is that the set of values (duplex, speed, flags) is consistent.
I.e one caller wins, the streams don't get crossed.