[dpdk-dev,RFC,01/14] ethdev: add link status read/write functions
Checks
Commit Message
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(+)
Comments
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.
> + */
> + 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.
> + continue;
> +
> + return (old.link_status == link->link_status) ? -1 : 0;
> +}
> +
> +void _rte_eth_link_read(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(uint8_t port_id, struct rte_eth_stats *stats)
> {
> struct rte_eth_dev *dev;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index f6837278521c..974657933f23 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct rte_eth_link *link);
> */
> void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
>
> +
> +/**
> + * @internal
> + * Atomically write 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_link_update(struct rte_eth_dev *dev,
> + const struct rte_eth_link *link);
> +
> +/**
> + * @internal
> + * Atomically read the link speed and status.
> + * @param dev
> + * Pointer to struct rte_eth_dev.
> + * @param link
> + * link status value.
> + */
> +void _rte_eth_link_read(struct rte_eth_dev *dev,
> + struct rte_eth_link *link);
> +
> /**
> * Retrieve the general I/O statistics of an Ethernet device.
> *
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.
>
> > + */
> > + 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.
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.
>>> + */
>>> + 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.
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.
On 07/17/2017 07:21 PM, Stephen Hemminger wrote:
> 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.
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.
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Saturday, July 15, 2017 2:30 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Stephen Hemminger
> <sthemmin@microsoft.com>
> Subject: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions
>
> 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.
> + */
> + while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> + *(volatile uint64_t *)dev_link,
> + *(const uint64_t *)link) == 0)
> + continue;
> +
> + return (old.link_status == link->link_status) ? -1 : 0; }
> +
> +void _rte_eth_link_read(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(uint8_t port_id, struct rte_eth_stats *stats) {
> struct rte_eth_dev *dev;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index
> f6837278521c..974657933f23 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct
> rte_eth_link *link);
> */
> void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
>
> +
> +/**
> + * @internal
> + * Atomically write 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_link_update(struct rte_eth_dev *dev,
> + const struct rte_eth_link *link);
> +
This function is only do the atomically write, what do you think to change the function name to _rte_eth_atomic_write_link_status,
Use name link_update makes me confused, and mix up it with dev_ops link_update.
> +/**
> + * @internal
> + * Atomically read the link speed and status.
> + * @param dev
> + * Pointer to struct rte_eth_dev.
> + * @param link
> + * link status value.
> + */
> +void _rte_eth_link_read(struct rte_eth_dev *dev,
> + struct rte_eth_link *link);
> +
This name is also not very clear. I think change to _rte_eth_atomic_read_link_status will better.
> /**
> * Retrieve the general I/O statistics of an Ethernet device.
> *
> --
> 2.11.0
On Wed, 11 Oct 2017 08:32:12 +0000
"Yang, Qiming" <qiming.yang@intel.com> wrote:
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Saturday, July 15, 2017 2:30 AM
> > To: dev@dpdk.org
> > Cc: Stephen Hemminger <stephen@networkplumber.org>; Stephen Hemminger
> > <sthemmin@microsoft.com>
> > Subject: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions
> >
> > 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.
> > + */
> > + while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> > + *(volatile uint64_t *)dev_link,
> > + *(const uint64_t *)link) == 0)
> > + continue;
> > +
> > + return (old.link_status == link->link_status) ? -1 : 0; }
> > +
> > +void _rte_eth_link_read(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(uint8_t port_id, struct rte_eth_stats *stats) {
> > struct rte_eth_dev *dev;
> > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index
> > f6837278521c..974657933f23 100644
> > --- a/lib/librte_ether/rte_ethdev.h
> > +++ b/lib/librte_ether/rte_ethdev.h
> > @@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct
> > rte_eth_link *link);
> > */
> > void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
> >
> > +
> > +/**
> > + * @internal
> > + * Atomically write 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_link_update(struct rte_eth_dev *dev,
> > + const struct rte_eth_link *link);
> > +
> This function is only do the atomically write, what do you think to change the function name to _rte_eth_atomic_write_link_status,
> Use name link_update makes me confused, and mix up it with dev_ops link_update.
> > +/**
> > + * @internal
> > + * Atomically read the link speed and status.
> > + * @param dev
> > + * Pointer to struct rte_eth_dev.
> > + * @param link
> > + * link status value.
> > + */
> > +void _rte_eth_link_read(struct rte_eth_dev *dev,
> > + struct rte_eth_link *link);
> > +
> This name is also not very clear. I think change to _rte_eth_atomic_read_link_status will better.
> > /**
> > * Retrieve the general I/O statistics of an Ethernet device.
> > *
> > --
> > 2.11.0
>
The first set of patches was just trying to combine multiple copies of same code.
Every place was doing same thing for atomic update.
Stephen,
Qiming was suggesting a name change for the functions.
What do you think?
13/10/2017 17:12, Stephen Hemminger:
> On Wed, 11 Oct 2017 08:32:12 +0000
> "Yang, Qiming" <qiming.yang@intel.com> wrote:
>
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > > Sent: Saturday, July 15, 2017 2:30 AM
> > > To: dev@dpdk.org
> > > Cc: Stephen Hemminger <stephen@networkplumber.org>; Stephen Hemminger
> > > <sthemmin@microsoft.com>
> > > Subject: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions
> > >
> > > 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.
> > > + */
> > > + while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> > > + *(volatile uint64_t *)dev_link,
> > > + *(const uint64_t *)link) == 0)
> > > + continue;
> > > +
> > > + return (old.link_status == link->link_status) ? -1 : 0; }
> > > +
> > > +void _rte_eth_link_read(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(uint8_t port_id, struct rte_eth_stats *stats) {
> > > struct rte_eth_dev *dev;
> > > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index
> > > f6837278521c..974657933f23 100644
> > > --- a/lib/librte_ether/rte_ethdev.h
> > > +++ b/lib/librte_ether/rte_ethdev.h
> > > @@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct
> > > rte_eth_link *link);
> > > */
> > > void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
> > >
> > > +
> > > +/**
> > > + * @internal
> > > + * Atomically write 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_link_update(struct rte_eth_dev *dev,
> > > + const struct rte_eth_link *link);
> > > +
> > This function is only do the atomically write, what do you think to change the function name to _rte_eth_atomic_write_link_status,
> > Use name link_update makes me confused, and mix up it with dev_ops link_update.
> > > +/**
> > > + * @internal
> > > + * Atomically read the link speed and status.
> > > + * @param dev
> > > + * Pointer to struct rte_eth_dev.
> > > + * @param link
> > > + * link status value.
> > > + */
> > > +void _rte_eth_link_read(struct rte_eth_dev *dev,
> > > + struct rte_eth_link *link);
> > > +
> > This name is also not very clear. I think change to _rte_eth_atomic_read_link_status will better.
> > > /**
> > > * Retrieve the general I/O statistics of an Ethernet device.
> > > *
> > > --
> > > 2.11.0
> >
>
> The first set of patches was just trying to combine multiple copies of same code.
> Every place was doing same thing for atomic update.
On Fri, 05 Jan 2018 15:24:48 +0100
Thomas Monjalon <thomas@monjalon.net> wrote:
> Stephen,
> Qiming was suggesting a name change for the functions.
> What do you think?
>
> 13/10/2017 17:12, Stephen Hemminger:
> > On Wed, 11 Oct 2017 08:32:12 +0000
> > "Yang, Qiming" <qiming.yang@intel.com> wrote:
> >
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> > > > Sent: Saturday, July 15, 2017 2:30 AM
> > > > To: dev@dpdk.org
> > > > Cc: Stephen Hemminger <stephen@networkplumber.org>; Stephen Hemminger
> > > > <sthemmin@microsoft.com>
> > > > Subject: [dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions
> > > >
> > > > 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.
> > > > + */
> > > > + while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
> > > > + *(volatile uint64_t *)dev_link,
> > > > + *(const uint64_t *)link) == 0)
> > > > + continue;
> > > > +
> > > > + return (old.link_status == link->link_status) ? -1 : 0; }
> > > > +
> > > > +void _rte_eth_link_read(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(uint8_t port_id, struct rte_eth_stats *stats) {
> > > > struct rte_eth_dev *dev;
> > > > diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h index
> > > > f6837278521c..974657933f23 100644
> > > > --- a/lib/librte_ether/rte_ethdev.h
> > > > +++ b/lib/librte_ether/rte_ethdev.h
> > > > @@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct
> > > > rte_eth_link *link);
> > > > */
> > > > void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
> > > >
> > > > +
> > > > +/**
> > > > + * @internal
> > > > + * Atomically write 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_link_update(struct rte_eth_dev *dev,
> > > > + const struct rte_eth_link *link);
> > > > +
> > > This function is only do the atomically write, what do you think to change the function name to _rte_eth_atomic_write_link_status,
> > > Use name link_update makes me confused, and mix up it with dev_ops link_update.
> > > > +/**
> > > > + * @internal
> > > > + * Atomically read the link speed and status.
> > > > + * @param dev
> > > > + * Pointer to struct rte_eth_dev.
> > > > + * @param link
> > > > + * link status value.
> > > > + */
> > > > +void _rte_eth_link_read(struct rte_eth_dev *dev,
> > > > + struct rte_eth_link *link);
> > > > +
> > > This name is also not very clear. I think change to _rte_eth_atomic_read_link_status will better.
> > > > /**
> > > > * Retrieve the general I/O statistics of an Ethernet device.
> > > > *
> > > > --
> > > > 2.11.0
> > >
> >
> > The first set of patches was just trying to combine multiple copies of same code.
> > Every place was doing same thing for atomic update.
I would just change the name to linkstatsus update.
Also since writes of unsigned long are guaranteed atomic, the code could
be optimized on 64bit platforms.
@@ -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.
+ */
+ while (rte_atomic64_cmpset((volatile uint64_t *)dev_link,
+ *(volatile uint64_t *)dev_link,
+ *(const uint64_t *)link) == 0)
+ continue;
+
+ return (old.link_status == link->link_status) ? -1 : 0;
+}
+
+void _rte_eth_link_read(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(uint8_t port_id, struct rte_eth_stats *stats)
{
struct rte_eth_dev *dev;
@@ -2219,6 +2219,34 @@ void rte_eth_link_get(uint8_t port_id, struct rte_eth_link *link);
*/
void rte_eth_link_get_nowait(uint8_t port_id, struct rte_eth_link *link);
+
+/**
+ * @internal
+ * Atomically write 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_link_update(struct rte_eth_dev *dev,
+ const struct rte_eth_link *link);
+
+/**
+ * @internal
+ * Atomically read the link speed and status.
+ * @param dev
+ * Pointer to struct rte_eth_dev.
+ * @param link
+ * link status value.
+ */
+void _rte_eth_link_read(struct rte_eth_dev *dev,
+ struct rte_eth_link *link);
+
/**
* Retrieve the general I/O statistics of an Ethernet device.
*