Re: [dpdk-dev] [PATCH v5 resend 07/12] virtio: resolve for control queue

[Yuanhan Liu <yuanhan.liu@linux.intel.com>]

On Sun, Sep 20, 2015 at 12:21:14PM +0300, Michael S. Tsirkin wrote:
> On Fri, Sep 18, 2015 at 11:10:56PM +0800, Yuanhan Liu wrote:
> > From: Changchun Ouyang <changchun.ouyang@intel.com>
> > 
> > Fix the max virtio queue pair read issue.
> > 
> > Control queue can't work for vhost-user mulitple queue mode,
> > so introduce a counter to void the dead loop when polling
> > the control queue.
> > 
> > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com>
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> 
> Per virtio spec, the multiqueue feature depends on control queue -
> what do you mean when you say it can't work?
> 
> > ---
> >  drivers/net/virtio/virtio_ethdev.c | 12 +++++++-----
> >  1 file changed, 7 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c
> > index 465d3cd..b2f4120 100644
> > --- a/drivers/net/virtio/virtio_ethdev.c
> > +++ b/drivers/net/virtio/virtio_ethdev.c
> > @@ -1162,7 +1162,6 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> >  	struct virtio_hw *hw = eth_dev->data->dev_private;
> >  	struct virtio_net_config *config;
> >  	struct virtio_net_config local_config;
> > -	uint32_t offset_conf = sizeof(config->mac);
> >  	struct rte_pci_device *pci_dev;
> >  
> >  	RTE_BUILD_BUG_ON(RTE_PKTMBUF_HEADROOM < sizeof(struct virtio_net_hdr));
> > @@ -1222,7 +1221,9 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> >  		config = &local_config;
> >  
> >  		if (vtpci_with_feature(hw, VIRTIO_NET_F_STATUS)) {
> > -			offset_conf += sizeof(config->status);
> > +			vtpci_read_dev_config(hw,
> > +				offsetof(struct virtio_net_config, status),
> > +				&config->status, sizeof(config->status));
> >  		} else {
> >  			PMD_INIT_LOG(DEBUG,
> >  				     "VIRTIO_NET_F_STATUS is not supported");
> > @@ -1230,15 +1231,16 @@ eth_virtio_dev_init(struct rte_eth_dev *eth_dev)
> >  		}
> >  
> >  		if (vtpci_with_feature(hw, VIRTIO_NET_F_MQ)) {
> > -			offset_conf += sizeof(config->max_virtqueue_pairs);
> > +			vtpci_read_dev_config(hw,
> > +				offsetof(struct virtio_net_config, max_virtqueue_pairs),
> > +				&config->max_virtqueue_pairs,
> > +				sizeof(config->max_virtqueue_pairs));
> >  		} else {
> >  			PMD_INIT_LOG(DEBUG,
> >  				     "VIRTIO_NET_F_MQ is not supported");
> >  			config->max_virtqueue_pairs = 1;
> >  		}
> >  
> > -		vtpci_read_dev_config(hw, 0, (uint8_t *)config, offset_conf);
> > -
> >  		hw->max_rx_queues =
> >  			(VIRTIO_MAX_RX_QUEUES < config->max_virtqueue_pairs) ?
> >  			VIRTIO_MAX_RX_QUEUES : config->max_virtqueue_pairs;
> 
> 
> Does the patch actually do what the commit log says?

Sorry, the commit log is wrong as you said.

It was actually a bug in our code, which happens to be revealed when
MQ is enabled. The old code adjusts the config bytes we want to read
depending on what kind of features we have, but we later cast the
entire buf we read with "struct virtio_net_config", which is obviously
wrong.

The right way to go is to read related config bytes when corresponding
feature is set, which is exactly what this patch does.

> It seems tobe about reading the device confing,
> not breaking out of a loop ...

It's just a (bad) side effect of getting the vritio_net_config wrongly:
the wrong config causes a dead loop in our code.

And sorry for the buggy commit log, will fix it next version.

Thanks.

	--yliu