[dpdk-dev,v2,02/15] ethdev: add linkstatus get/set helper functions
Checks
Commit Message
Many drivers are all doing copy/paste of the same code to atomically
update the link status. Reduce duplication, and allow for future
changes by having common function for this.
Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
---
lib/librte_ether/rte_ethdev.c | 35 +++++++++++++++++++++++++++++++++++
lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
2 files changed, 63 insertions(+)
Comments
On 01/06/2018 04:06 AM, Stephen Hemminger wrote:
> Many drivers are all doing copy/paste of the same code to atomically
> update the link status. Reduce duplication, and allow for future
> changes by having common function for this.
>
> Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> ---
> lib/librte_ether/rte_ethdev.c | 35 +++++++++++++++++++++++++++++++++++
> lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> 2 files changed, 63 insertions(+)
>
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 27b52131ef01..4897e1dcda14 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1495,6 +1495,41 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
> }
> }
>
> +int
> +_rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> + const struct rte_eth_link *new_link)
> +{
> + volatile uint64_t *dev_link
> + = (volatile uint64_t *)&(dev->data->dev_link);
> + union {
> + uint64_t val64;
> + struct rte_eth_link link;
> + } orig;
> +
> + RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> +
> + orig.val64 = rte_atomic64_exchange(dev_link,
> + *(const uint64_t *)new_link);
> +
> + return (orig.link.link_status == new_link->link_status) ? -1 : 0;
It looks like it contradicts to the function description saying that 0 is
returned if link status is the same.
> +}
> +
> +void
> +_rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> + struct rte_eth_link *link)
> +{
> + const uint64_t *src = (const uint64_t *)&(dev->data->dev_link);
> + volatile uint64_t *dst = (uint64_t *)link;
> +
> + RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> +
> + /*
> + * Note: this should never fail since both destination and expected
> + * values are the same and are a pointer from caller.
> + */
> + rte_atomic64_cmpset(dst, *dst, *src);
As I understand nobody says that *link (i.e. *dst) is initialized, so it
could be
reading uninitialized value. Not fatal, but not good as well.
May be it is better to use something like rte_atomic64_read().
> +}
> +
> int
> rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
> {
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index b10e2a92da71..b47402b797c3 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2218,6 +2218,34 @@ int rte_eth_dev_set_link_down(uint16_t port_id);
> */
> void rte_eth_dev_close(uint16_t port_id);
>
> +
> +/**
> + * @internal
> + * Atomically set the link status for the specific device.
> + * It is for use by DPDK device driver use only.
> + * User applications should not call it
> + *
> + * @param dev
> + * Pointer to struct rte_eth_dev.
> + * @param link
> + * New link status value.
> + * @return
> + * -1 if link state has changed, 0 if the same.
> + */
> +int _rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> + const struct rte_eth_link *link);
> +
> +/**
> + * @internal
> + * Atomically get the link speed and status.
> + * @param dev
> + * Pointer to struct rte_eth_dev.
> + * @param link
> + * link status value.
> + */
> +void _rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> + struct rte_eth_link *link);
> +
> /**
> * Reset a Ethernet device and keep its port id.
> *
On Sat, 6 Jan 2018 13:49:50 +0300
Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> On 01/06/2018 04:06 AM, Stephen Hemminger wrote:
> > Many drivers are all doing copy/paste of the same code to atomically
> > update the link status. Reduce duplication, and allow for future
> > changes by having common function for this.
> >
> > Signed-off-by: Stephen Hemminger <stephen@networkplumber.org>
> > ---
> > lib/librte_ether/rte_ethdev.c | 35 +++++++++++++++++++++++++++++++++++
> > lib/librte_ether/rte_ethdev.h | 28 ++++++++++++++++++++++++++++
> > 2 files changed, 63 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> > index 27b52131ef01..4897e1dcda14 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -1495,6 +1495,41 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
> > }
> > }
> >
> > +int
> > +_rte_eth_linkstatus_set(struct rte_eth_dev *dev,
> > + const struct rte_eth_link *new_link)
> > +{
> > + volatile uint64_t *dev_link
> > + = (volatile uint64_t *)&(dev->data->dev_link);
> > + union {
> > + uint64_t val64;
> > + struct rte_eth_link link;
> > + } orig;
> > +
> > + RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
> > +
> > + orig.val64 = rte_atomic64_exchange(dev_link,
> > + *(const uint64_t *)new_link);
> > +
> > + return (orig.link.link_status == new_link->link_status) ? -1 : 0;
>
> It looks like it contradicts to the function description saying that 0 is
> returned if link status is the same.
Looks like a mismatch in original code. Will fix that.
> > +}
> > +
> > +void
> > +_rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
> > + struct rte_eth_link *link)
> > +{
> > + const uint64_t *src = (const uint64_t *)&(dev->data->dev_link);
> > + volatile uint64_t *dst = (uint64_t *)link;
> > +
> > + RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
> > +
> > + /*
> > + * Note: this should never fail since both destination and expected
> > + * values are the same and are a pointer from caller.
> > + */
> > + rte_atomic64_cmpset(dst, *dst, *src);
>
> As I understand nobody says that *link (i.e. *dst) is initialized, so it
> could be
> reading uninitialized value. Not fatal, but not good as well.
> May be it is better to use something like rte_atomic64_read().
>
link is a pointer passed in by the caller.
If it is uninitialized, that is still ok since it will have some value.
The different question is that since is almost backwards use of cmpset,
not sure if processor really guarantees atomic read of source pointer.
But this code is cloned from original in Intel driver so it is bug
for bug compatiable!
On 01/06/2018 05:24 PM, Stephen Hemminger wrote:
> On Sat, 6 Jan 2018 13:49:50 +0300
> Andrew Rybchenko <arybchenko@solarflare.com> wrote:
>
>> On 01/06/2018 04:06 AM, Stephen Hemminger wrote:
>>> +}
>>> +
>>> +void
>>> +_rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
>>> + struct rte_eth_link *link)
>>> +{
>>> + const uint64_t *src = (const uint64_t *)&(dev->data->dev_link);
>>> + volatile uint64_t *dst = (uint64_t *)link;
>>> +
>>> + RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
>>> +
>>> + /*
>>> + * Note: this should never fail since both destination and expected
>>> + * values are the same and are a pointer from caller.
>>> + */
>>> + rte_atomic64_cmpset(dst, *dst, *src);
>> As I understand nobody says that *link (i.e. *dst) is initialized, so it
>> could be
>> reading uninitialized value. Not fatal, but not good as well.
>> May be it is better to use something like rte_atomic64_read().
>>
> link is a pointer passed in by the caller.
> If it is uninitialized, that is still ok since it will have some value.
I thought about valgrind. Yes, it is problematic to run with hugepages,
but should be possible (with some patches). If so, it will/can complain
about usage of uninitialized value.
> The different question is that since is almost backwards use of cmpset,
> not sure if processor really guarantees atomic read of source pointer.
> But this code is cloned from original in Intel driver so it is bug
> for bug compatiable!
I see, but if so it was local to corresponding Intel driver. Now it is
becoming global.
@@ -1495,6 +1495,41 @@ rte_eth_link_get_nowait(uint16_t port_id, struct rte_eth_link *eth_link)
}
}
+int
+_rte_eth_linkstatus_set(struct rte_eth_dev *dev,
+ const struct rte_eth_link *new_link)
+{
+ volatile uint64_t *dev_link
+ = (volatile uint64_t *)&(dev->data->dev_link);
+ union {
+ uint64_t val64;
+ struct rte_eth_link link;
+ } orig;
+
+ RTE_BUILD_BUG_ON(sizeof(*new_link) != sizeof(uint64_t));
+
+ orig.val64 = rte_atomic64_exchange(dev_link,
+ *(const uint64_t *)new_link);
+
+ return (orig.link.link_status == new_link->link_status) ? -1 : 0;
+}
+
+void
+_rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
+ struct rte_eth_link *link)
+{
+ const uint64_t *src = (const uint64_t *)&(dev->data->dev_link);
+ volatile uint64_t *dst = (uint64_t *)link;
+
+ RTE_BUILD_BUG_ON(sizeof(*link) != sizeof(uint64_t));
+
+ /*
+ * Note: this should never fail since both destination and expected
+ * values are the same and are a pointer from caller.
+ */
+ rte_atomic64_cmpset(dst, *dst, *src);
+}
+
int
rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats *stats)
{
@@ -2218,6 +2218,34 @@ int rte_eth_dev_set_link_down(uint16_t port_id);
*/
void rte_eth_dev_close(uint16_t port_id);
+
+/**
+ * @internal
+ * Atomically set the link status for the specific device.
+ * It is for use by DPDK device driver use only.
+ * User applications should not call it
+ *
+ * @param dev
+ * Pointer to struct rte_eth_dev.
+ * @param link
+ * New link status value.
+ * @return
+ * -1 if link state has changed, 0 if the same.
+ */
+int _rte_eth_linkstatus_set(struct rte_eth_dev *dev,
+ const struct rte_eth_link *link);
+
+/**
+ * @internal
+ * Atomically get the link speed and status.
+ * @param dev
+ * Pointer to struct rte_eth_dev.
+ * @param link
+ * link status value.
+ */
+void _rte_eth_linkstatus_get(const struct rte_eth_dev *dev,
+ struct rte_eth_link *link);
+
/**
* Reset a Ethernet device and keep its port id.
*