Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Wednesday, October 23, 2019 10:05 AM
> To: Ori Kam <or...@mellanox.com>
> Cc: dev@dpdk.org; Ferruh Yigit <ferruh.yi...@intel.com>; Andrew Rybchenko
> <arybche...@solarflare.com>; jingjing...@intel.com;
> step...@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];
> > +};
>
>
