> -----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
