Re: [dpdk-dev] [PATCH v3 3/7] net/virtio_user: move vhost user specific code

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

On Wed, Jan 04, 2017 at 06:46:34AM +0000, Tan, Jianfeng wrote:
> Hi Yuanhan,
> 
> > -----Original Message-----
> > From: Yuanhan Liu [mailto:yuanhan.liu@linux.intel.com]
> > Sent: Wednesday, January 4, 2017 2:03 PM
> > To: Tan, Jianfeng
> > Cc: dev@dpdk.org; Yigit, Ferruh; Liang, Cunming
> > Subject: Re: [PATCH v3 3/7] net/virtio_user: move vhost user specific code
> > 
> > On Wed, Jan 04, 2017 at 03:59:22AM +0000, Jianfeng Tan wrote:
> > > To support vhost kernel as the backend of net_virtio_user in coming
> > > patches, we move vhost_user specific structs and macros into
> > > vhost_user.c, and only keep common definitions in vhost.h.
> > >
> > > Besides, remove VHOST_USER_MQ feature check.
> > 
> > Again, I have to ask, why? You don't only remove the check, also, you
> > removed this feature setting, which seems to break the MQ support?
> 
> I have answered it here:
> http://dpdk.org/ml/archives/dev/2016-December/053520.html

I thought we have made some agreements :/

> 
> To be more clear, VHOST_USER_MQ is a not-well-defined macro: #define VHOST_USER_MQ (1ULL << VHOST_USER_F_PROTOCOL_FEATURES),
> which is a feature bit in vhost user protocol.

Yes, it's again named wrongly.

> According to QEMU/ docs/specs/vhost-user.txt, "If VHOST_USER_F_PROTOCOL_FEATURES has not been negotiated, the ring is initialized in an enabled state. "
> 
> But our DPDK vhost library does not take care of this feature bit.
> Just make this as default: the ring is initialized in an disabled state. And our virtio_user with vhost-user does send VHOST_USER_SET_VRING_ENABLE to enable each queue pair.

VHOST_USER_F_PROTOCOL_FEATURES is a fundamental feature for quite many
vhost-user extended features, including the MQ. If it's not set, the MQ
should not work.

It may still work in your case, becase you made an assumtion that the
vhost backend supports the MQ feature (which is true in nowadays, as
the feature has been there for a quite while). However, that's not an
assumtion you can take while adding the vhost-user MQ support at that
time. And such feature bit (including the PROTOCOL_F_MQ) makes sure
that we will not try to enable MQ with and older vhost backend that
doesn't have the support.

Put simply, this feature is needed, and as the feature name states,
it's needed only for vhost-user.

	--yliu

> 
> So I think it's not necessary to add it back.
> 
> How do you think?
> 
> Thanks,
> Jianfeng
> 
> > 
> > 	--yliu