Re: [PATCH 7/7] vhost-vdpa: backend feature should set only once

[Si-Wei Liu]

On 3/30/2022 11:27 AM, Eugenio Perez Martin wrote:
> On Wed, Mar 30, 2022 at 7:32 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
> > On Wed, Mar 30, 2022 at 10:12:42AM -0700, Si-Wei Liu wrote:
> > > On 3/30/2022 9:24 AM, Stefano Garzarella wrote:
> > > > On Tue, Mar 29, 2022 at 11:33:17PM -0700, Si-Wei Liu wrote:
> > > > > The vhost_vdpa_one_time_request() branch in
> > > > > vhost_vdpa_set_backend_cap() incorrectly sends down
> > > > > iotls on vhost_dev with non-zero index. This may
> > > > Little typo s/iotls/ioctls
> > > Thanks! Will correct it in v2.
> > >
> > > > > end up with multiple VHOST_SET_BACKEND_FEATURES
> > > > > ioctl calls sent down on the vhost-vdpa fd that is
> > > > > shared between all these vhost_dev's.
> > > > >
> > > > > To fix it, send down ioctl only once via the first
> > > > > vhost_dev with index 0. Toggle the polarity of the
> > > > > vhost_vdpa_one_time_request() test would do the trick.
> > > > >
> > > > > Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> > > > > ---
> > > > > hw/virtio/vhost-vdpa.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > > index c5ed7a3..27ea706 100644
> > > > > --- a/hw/virtio/vhost-vdpa.c
> > > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > > @@ -665,7 +665,7 @@ static int vhost_vdpa_set_backend_cap(struct
> > > > > vhost_dev *dev)
> > > > >
> > > > >     features &= f;
> > > > >
> > > > > -    if (vhost_vdpa_one_time_request(dev)) {
> > > > > +    if (!vhost_vdpa_one_time_request(dev)) {
> > > > >         r = vhost_vdpa_call(dev, VHOST_SET_BACKEND_FEATURES, &features);
> > > > >         if (r) {
> > > > >             return -EFAULT;
> > > > > --
> > > > > 1.8.3.1
> > > > With that:
> > > >
> > > > Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > >
> > > >
> > > >
> > > > Unrelated to this patch, but the name vhost_vdpa_one_time_request()
> > > > is confusing IMHO.
> > > Coincidentally I got the same feeling and wanted to rename it to
> > > something else, too.
> > >
> > > > Not that I'm good with names, but how about we change it to
> > > > vhost_vdpa_skip_one_time_request()?
> > > How about vhost_vdpa_request_already_applied()? seems to be more
> > > readable in the context.
> > That's fine too, except you can't discern that it's a single request per
> > device, so maybe I'd add "dev," but I don't know if it gets too long:
> >
> > vhost_vdpa_dev_request_already_applied()
> >
> > And I would also add a comment to the function to explain that we use
> > that function for requests that only need to be applied once, regardless
> > of the number of queues.
> In my opinion it should express what it checks: vhost_vdpa_first, or
> vhost_vdpa_first_dev, vhost_vdpa_first_queue...
Indeed, the same name ever came to my mind that is to reflect what it 
actually checks. It is just that I have to flip the polarity of the 
vhost_vdpa_one_time_request() function that made me adopt another name. 
What matches best for the current code would be something similar to 
vhost_vdpa_dev_other_than_the_first(), though why bother using another 
function rather than write the check as-is in place is another question.

>   and to add a comment
> like the one you propose.
Will do.
>   I think the context of the caller gives
> enough information.
>
> I would add that the use is for requests that only need / must be
> applied once *and before setting up queues*, *at the beginning of
> operation*, or similar, because we do a similar check with
> dev->vq_index_end and these are not exchangeable.
Nods. Exactly.

-Siwei
> Thanks!
>
> > Thanks,
> > Stefano