On 07/17/2017 07:21 PM, Stephen Hemminger wrote:
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.
Agree.
+ */
+ 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.
Results of the update operation is used by some driver to log link up/down
change. So, it could result in duplicate up/down logs. Not a big deal, but
could be confusing.
I guess many are very busy right now with 17.08 release. So, I hope
we'll see more feedback when 17.08 release is done.
