Re: [dpdk-dev] [PATCH v13 2/2] vhost: Add VHOST PMD

["Loftus, Ciara" <ciara.loftus@intel.com>]

> 
> On 2016/03/22 10:55, Tetsuya Mukawa wrote:
> > On 2016/03/22 0:40, Loftus, Ciara wrote:
> >>> +
> >>> +static void
> >>> +eth_dev_info(struct rte_eth_dev *dev,
> >>> +	     struct rte_eth_dev_info *dev_info)
> >>> +{
> >>> +	dev_info->driver_name = drivername;
> >>> +	dev_info->max_mac_addrs = 1;
> >>> +	dev_info->max_rx_pktlen = (uint32_t)-1;
> >>> +	dev_info->max_rx_queues = dev->data->nb_rx_queues;
> >>> +	dev_info->max_tx_queues = dev->data->nb_tx_queues;
> >> I'm not entirely familiar with eth driver code so please correct me if I am
> wrong.
> >>
> >> I'm wondering if assigning the max queue values to dev->data-
> >nb_*x_queues is correct.
> >> A user could change the value of nb_*x_queues with a call to
> rte_eth_dev_configure(n_queues) which in turn calls
> rte_eth_dev_*x_queue_config(n_queues) which will set dev->data-
> >nb_*x_queues to the value of n_queues which can be arbitrary and
> decided by the user. If this is the case, dev->data->nb_*x_queues will no
> longer reflect the max, rather the value the user chose in the call to
> rte_eth_dev_configure. And the max could potentially change with multiple
> calls to configure. Is this intended behaviour?
> > Hi Ciara,
> >
> > Thanks for reviewing it. Here is a part of rte_eth_dev_configure().
> >
> > int
> > rte_eth_dev_configure(uint8_t port_id, uint16_t nb_rx_q, uint16_t
> nb_tx_q,
> >                       const struct rte_eth_conf *dev_conf)
> > {
> >         <snip>
> >         /*
> >          * Check that the numbers of RX and TX queues are not greater
> >          * than the maximum number of RX and TX queues supported by the
> >          * configured device.
> >          */
> >         (*dev->dev_ops->dev_infos_get)(dev, &dev_info);
> >
> >         if (nb_rx_q == 0 && nb_tx_q == 0) {
> >                <snip>
> >                 return -EINVAL;
> >         }
> >
> >         if (nb_rx_q > dev_info.max_rx_queues) {
> >                <snip>
> >                 return -EINVAL;
> >         }
> >
> >         if (nb_tx_q > dev_info.max_tx_queues) {
> >                <snip>
> >                 return -EINVAL;
> >         }
> >
> >         <snip>
> >
> >         /*
> >          * Setup new number of RX/TX queues and reconfigure device.
> >          */
> >         diag = rte_eth_dev_rx_queue_config(dev, nb_rx_q);
> >         <snip>
> >         diag = rte_eth_dev_tx_queue_config(dev, nb_tx_q);
> >         <snip>
> > }
> >
> > Anyway, rte_eth_dev_tx/rx_queue_config() will be called only after
> > checking the current maximum number of queues.
> > So the user cannot set the number of queues greater than current
> maximum
> > number.
> >
> > Regards,
> > Tetsuya
> 
> Hi Ciara,
> 
> Now, I understand what you say.
> Probably you pointed out the case that the user specified a value
> smaller than current maximum value.
> 
> For example, if we have 4 queues. Below code will be failed at last line.
> rte_eth_dev_configure(portid, 4, 4, ...);
> rte_eth_dev_configure(portid, 2, 2, ...);
> rte_eth_dev_configure(portid, 4, 4, ...);
> 
> I will submit a patch to fix it. Could you please review and ack it?

Hi Tetsuya,

Correct, sorry for the initial confusion. Thanks for the patch so quickly.
I've reviewed the code - looks good. I just want to run some tests and will give my Ack later today all going well.

Thanks,
Ciara

> 
> Regards,
> Tetsuya