Re: [dpdk-dev] [PATCH v4 02/15] ethdev: add support for hairpin queue

[Ori Kam <orika@mellanox.com>]

Hi Thomas,

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, October 23, 2019 10:05 AM
> To: Ori Kam <orika@mellanox.com>
> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yigit@intel.com>; Andrew Rybchenko
> <arybchenko@solarflare.com>; jingjing.wu@intel.com;
> stephen@networkplumber.org
> Subject: Re: [dpdk-dev] [PATCH v4 02/15] ethdev: add support for hairpin queue
> 
> 17/10/2019 17:32, Ori Kam:
> > --- a/lib/librte_ethdev/rte_ethdev.h
> > +++ b/lib/librte_ethdev/rte_ethdev.h
> >  /**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * A structure used to return the hairpin capabilities that are supported.
> > + */
> > +struct rte_eth_hairpin_cap {
> > +	uint16_t max_n_queues;
> > +	/**< The max number of hairpin queues (different bindings). */
> > +	uint16_t max_rx_2_tx;
> > +	/**< Max number of Rx queues to be connected to one Tx queue. */
> > +	uint16_t max_tx_2_rx;
> > +	/**< Max number of Tx queues to be connected to one Rx queue. */
> > +	uint16_t max_nb_desc; /**< The max num of descriptors. */
> > +};
> 
> I think you can switch to "comment-first style" for this struct.
> 

O.K I will change.

> 
> > +#define RTE_ETH_MAX_HAIRPIN_PEERS 32
> 
> Usually I think such define is in the build config.
> Any other opinion?
> 

I need to check. But if you don't mind let's keep it this way, and modify it 
later after we see how other manufactures will add hairpin.

> 
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * A structure used to hold hairpin peer data.
> > + */
> > +struct rte_eth_hairpin_peer {
> > +	uint16_t port; /**< Peer port. */
> > +	uint16_t queue; /**< Peer queue. */
> > +};
> 
> It may be the right place to give more words about what is a peer,
> can we have multiple peers, etc.
> 


I'm not sure what I can say to make it clearer but I will try.

> 
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change, or be removed, without prior
> notice
> > + *
> > + * A structure used to configure hairpin binding.
> > + */
> > +struct rte_eth_hairpin_conf {
> > +	uint16_t peer_n; /**< The number of peers. */
> 
> In general, I don't like one-letter abbreviations.
> Is peer_count better?
> 

O.K. I will change to count.

> > +	struct rte_eth_hairpin_peer peers[RTE_ETH_MAX_HAIRPIN_PEERS];
> > +};
> 
>