From: Stephen Hemminger <stephen@networkplumber.org>
To: Andrew Rybchenko <arybchenko@solarflare.com>
Cc: <dev@dpdk.org>, Stephen Hemminger <sthemmin@microsoft.com>
Subject: Re: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions
Date: Mon, 17 Jul 2017 09:21:51 -0700 [thread overview]
Message-ID: <20170717092151.507a9ecb@xeon-e3> (raw)
In-Reply-To: <dda1e0ff-fe16-eac4-d887-3cb24c9c95e6@solarflare.com>
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.
next prev parent reply other threads:[~2017-07-17 16:21 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-07-14 18:30 [dpdk-dev] [RFC 00/14] link status API improvement and bugfixes Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions Stephen Hemminger
2017-07-16 13:26 ` Andrew Rybchenko
2017-07-17 15:58 ` Stephen Hemminger
2017-07-17 16:12 ` Andrew Rybchenko
2017-07-17 16:21 ` Stephen Hemminger [this message]
2017-07-17 16:31 ` Andrew Rybchenko
2017-10-11 8:32 ` Yang, Qiming
2017-10-13 15:12 ` Stephen Hemminger
2018-01-05 14:24 ` Thomas Monjalon
2018-01-05 20:15 ` Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 02/14] virtio: use eth_link_read/write (and bug fix) Stephen Hemminger
2017-07-16 12:33 ` Andrew Rybchenko
2017-07-17 16:01 ` Stephen Hemminger
2017-07-17 16:14 ` [dpdk-dev] ***Spam*** " Andrew Rybchenko
2017-07-17 16:28 ` [dpdk-dev] " Stephen Hemminger
2018-01-05 15:04 ` Thomas Monjalon
2017-07-14 18:30 ` [dpdk-dev] [RFC 03/14] bnxt: use rte_link_update Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 04/14] vmxnet3: use rte_eth_link_update Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 05/14] dpaa2: " Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 06/14] nfp: " Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 07/14] e1000: " Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 08/14] ixgbe: " Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 09/14] sfc: use new rte_eth_link helpers Stephen Hemminger
2017-07-16 13:48 ` Andrew Rybchenko
2017-07-17 16:02 ` Stephen Hemminger
2017-07-17 16:19 ` Andrew Rybchenko
2017-07-14 18:30 ` [dpdk-dev] [RFC 10/14] i40e: use rte_eth_link_update (and bug fix) Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 11/14] liquidio: use _rte_eth_link_update Stephen Hemminger
2017-07-18 10:17 ` Shijith Thotton
2017-07-14 18:30 ` [dpdk-dev] [RFC 12/14] thunderx: " Stephen Hemminger
2017-07-14 18:30 ` [dpdk-dev] [RFC 13/14] szedata: " Stephen Hemminger
2017-07-16 12:46 ` Andrew Rybchenko
2017-07-14 18:30 ` [dpdk-dev] [RFC 14/14] enic: " Stephen Hemminger
2017-07-16 13:55 ` [dpdk-dev] [RFC 00/14] link status API improvement and bugfixes Andrew Rybchenko
2018-01-05 14:29 ` Thomas Monjalon
2018-01-05 20:18 ` Stephen Hemminger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170717092151.507a9ecb@xeon-e3 \
--to=stephen@networkplumber.org \
--cc=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=sthemmin@microsoft.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).