On Mon, 17 Jul 2017 19:12:01 +0300
Andrew Rybchenko <arybche...@solarflare.com> wrote:
> On 07/17/2017 06:58 PM, Stephen Hemminger wrote:
> > On Sun, 16 Jul 2017 16:26:06 +0300
> > Andrew Rybchenko <arybche...@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 <sthem...@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.
