[dpdk-dev] [PATCHv4 1/5] ethdev: add new API to retrieve RX/TX queue information

[Ananyev, Konstantin]

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon at 6wind.com]
> Sent: Thursday, July 23, 2015 5:26 PM
> To: Ananyev, Konstantin
> Cc: dev at dpdk.org
> Subject: Re: [dpdk-dev] [PATCHv4 1/5] ethdev: add new API to retrieve RX/TX queue information
> 
> 2015-07-22 19:28, Konstantin Ananyev:
> > +	if (!rte_eth_dev_is_valid_port(port_id)) {
> > +		PMD_DEBUG_TRACE("Invalid port_id=%d\n", port_id);
> > +		return -EINVAL;
> > +	}
> 
> Please use VALID_PORTID_OR_ERR_RET.
> 
> > + * Ethernet device RX queue information strcuture.
> 
> Typo here (and same for TX).
> 
> > +struct rte_eth_rxq_info {
> > +	struct rte_mempool *mp;     /**< mempool used by that queue. */
> > +	struct rte_eth_rxconf conf; /**< queue config parameters. */
> > +	uint8_t scattered_rx;       /**< scattered packets RX supported. */
> > +	uint16_t nb_desc;           /**< configured number of RXDs. */
> 
> Shouldn't we move nb_desc in rte_eth_rxconf?
> So rte_eth_rx_queue_setup() would have less parameters.

I thought about that too, but it seems more drawbacks then pluses with that idea:
1. Right now it is possible to call rte_eth_rx_queue_setup(..., rx_conf=NULL, ...);
In that case rte_eth_rx_queue_setup()will use default for that device rx_conf.
If we'll move mempool into rxconf, will break that ability.
2.  A bit unclear what mempool should be returned as default_rxconf by rte_eth_dev_info_get().
Should it be just NULL.
3. ABI breakage and we (and all customers) will need  to modify each and every RX setup/configure code.

For me it just seems like too much hassle, without any clear advanatage.

> 
> > -#ifdef __cplusplus
> > -}
> > -#endif
> > -
> >  /**
> >   * Set the list of multicast addresses to filter on an Ethernet device.
> >   *
> > @@ -3882,4 +3952,9 @@ extern int rte_eth_timesync_read_rx_timestamp(uint8_t port_id,
> >   */
> >  extern int rte_eth_timesync_read_tx_timestamp(uint8_t port_id,
> >  					      struct timespec *timestamp);
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> >  #endif /* _RTE_ETHDEV_H_ */
> 
> Please send this change in a separate patch alone.

Ok, will do.

Konstantin