[dpdk-dev] [RFC 01/14] ethdev: add link status read/write functions

Stephen Hemminger stephen at networkplumber.org
Fri Oct 13 17:12:15 CEST 2017


On Wed, 11 Oct 2017 08:32:12 +0000
"Yang, Qiming" <qiming.yang at intel.com> wrote:

> > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Stephen Hemminger
> > Sent: Saturday, July 15, 2017 2:30 AM
> > To: dev at dpdk.org
> > Cc: Stephen Hemminger <stephen at networkplumber.org>; Stephen Hemminger
> > <sthemmin at 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 at 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.



More information about the dev mailing list