What are libvhost-user locking requirements

What are libvhost-user locking requirements

[Vivek Goyal]

Hi,

Current virtiofsd code uses libvhost-user and I am assuming virtiofsd-rs
uses it too. I am wondering what are the locking requirements for
this library.

Looking at it it does not look like thread safe. Well parts of of kind
of look thread safe. For example, David Gilbert introduced a slave_mutex
to control reading/writeing on slave_fd. But dev->slave_fd can be modified
vu_set_slave_req_fd() without any locks. Similiarly _vu_queue_notify()
uses dev->slave_fd but  does not take any lock. May be these are just
bugs and we can take slave_mutex in those paths so not a big deal.

But this library does not talk about locking at all. Of course there
are many shared data structures like "struct VuDev" and helpers which
access this structure. Is client supposed to provide locking and
make sure not more than one thread is calling into the library
at one point of time.

But in virtiofsd I see that we seem to be in mixed mode. In some cases
we are holding ->vu_dispatch_rwlock in read-only mode. So that will
allow multipler threads to call into library for one queue.

In other places like lo_setupmapping() and lo_removemapping(), we are
not holding ->vu_dispatch_rwlock() at all and simply call into
library vu_fs_cache_request(VHOST_USER_SLAVE_FS_MAP/...). So multiple
threads can call in. I think precisely for this use case dev->slave_mutex
has been introduced in library.

So few queries.

- what's the locking model needed to use libvhost-user. Is there one? 

- Is it ok to selectively add locking for some data structures in
  libvhost-user. As slave_mutex has been added. So user will have to
  go through the code to figure out which paths can be called without
  locks and which paths can't be.

/me is confused and trying to wrap my head around the locking requirements
while using libvhost-user.

Vivek

[Virtio-fs] What are libvhost-user locking requirements

[Vivek Goyal]

Hi,

Current virtiofsd code uses libvhost-user and I am assuming virtiofsd-rs
uses it too. I am wondering what are the locking requirements for
this library.

Looking at it it does not look like thread safe. Well parts of of kind
of look thread safe. For example, David Gilbert introduced a slave_mutex
to control reading/writeing on slave_fd. But dev->slave_fd can be modified
vu_set_slave_req_fd() without any locks. Similiarly _vu_queue_notify()
uses dev->slave_fd but  does not take any lock. May be these are just
bugs and we can take slave_mutex in those paths so not a big deal.

But this library does not talk about locking at all. Of course there
are many shared data structures like "struct VuDev" and helpers which
access this structure. Is client supposed to provide locking and
make sure not more than one thread is calling into the library
at one point of time.

But in virtiofsd I see that we seem to be in mixed mode. In some cases
we are holding ->vu_dispatch_rwlock in read-only mode. So that will
allow multipler threads to call into library for one queue.

In other places like lo_setupmapping() and lo_removemapping(), we are
not holding ->vu_dispatch_rwlock() at all and simply call into
library vu_fs_cache_request(VHOST_USER_SLAVE_FS_MAP/...). So multiple
threads can call in. I think precisely for this use case dev->slave_mutex
has been introduced in library.

So few queries.

- what's the locking model needed to use libvhost-user. Is there one? 

- Is it ok to selectively add locking for some data structures in
  libvhost-user. As slave_mutex has been added. So user will have to
  go through the code to figure out which paths can be called without
  locks and which paths can't be.

/me is confused and trying to wrap my head around the locking requirements
while using libvhost-user.

Vivek

Re: What are libvhost-user locking requirements

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Hi,
> 
> Current virtiofsd code uses libvhost-user and I am assuming virtiofsd-rs
> uses it too. I am wondering what are the locking requirements for
> this library.

No, virtiofsd-rs uses the rust crate: https://github.com/rust-vmm/vhost-user-backend
I guess that's where they get their dose of 'Fearless concurrency'

> Looking at it it does not look like thread safe. Well parts of of kind
> of look thread safe. For example, David Gilbert introduced a slave_mutex
> to control reading/writeing on slave_fd. But dev->slave_fd can be modified
> vu_set_slave_req_fd() without any locks. Similiarly _vu_queue_notify()
> uses dev->slave_fd but  does not take any lock. May be these are just
> bugs and we can take slave_mutex in those paths so not a big deal.

That would be my assumption; I don't think libvhost-user really thought
about it much.

> But this library does not talk about locking at all. Of course there
> are many shared data structures like "struct VuDev" and helpers which
> access this structure. Is client supposed to provide locking and
> make sure not more than one thread is calling into the library
> at one point of time.

I don't think it's defined.

> But in virtiofsd I see that we seem to be in mixed mode. In some cases
> we are holding ->vu_dispatch_rwlock in read-only mode. So that will
> allow multipler threads to call into library for one queue.

I think that lock is really protecting against the queue management
actions on vhost-user remapping the queue conflicting with things
operating on the queue.

> In other places like lo_setupmapping() and lo_removemapping(), we are
> not holding ->vu_dispatch_rwlock() at all and simply call into
> library vu_fs_cache_request(VHOST_USER_SLAVE_FS_MAP/...). So multiple
> threads can call in. I think precisely for this use case dev->slave_mutex
> has been introduced in library.

Note that those calls don't actually read/write interact on the queue
itself; so I don't *think* they need the vu_dispatch_rwlock.

> So few queries.
> 
> - what's the locking model needed to use libvhost-user. Is there one? 

I don't think it really had one.

> - Is it ok to selectively add locking for some data structures in
>   libvhost-user. As slave_mutex has been added. So user will have to
>   go through the code to figure out which paths can be called without
>   locks and which paths can't be.

Well it certainly needed something added; hence why I added slave_mutex,
but the slave_mutex is mostly separate from the actual queue processing,
and actually rarely used.

> /me is confused and trying to wrap my head around the locking requirements
> while using libvhost-user.

It's not well defined at all.

Dave

> 
> Vivek
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Virtio-fs] What are libvhost-user locking requirements

[Dr. David Alan Gilbert]

* Vivek Goyal (vgoyal@redhat.com) wrote:
> Hi,
> 
> Current virtiofsd code uses libvhost-user and I am assuming virtiofsd-rs
> uses it too. I am wondering what are the locking requirements for
> this library.

No, virtiofsd-rs uses the rust crate: https://github.com/rust-vmm/vhost-user-backend
I guess that's where they get their dose of 'Fearless concurrency'

> Looking at it it does not look like thread safe. Well parts of of kind
> of look thread safe. For example, David Gilbert introduced a slave_mutex
> to control reading/writeing on slave_fd. But dev->slave_fd can be modified
> vu_set_slave_req_fd() without any locks. Similiarly _vu_queue_notify()
> uses dev->slave_fd but  does not take any lock. May be these are just
> bugs and we can take slave_mutex in those paths so not a big deal.

That would be my assumption; I don't think libvhost-user really thought
about it much.

> But this library does not talk about locking at all. Of course there
> are many shared data structures like "struct VuDev" and helpers which
> access this structure. Is client supposed to provide locking and
> make sure not more than one thread is calling into the library
> at one point of time.

I don't think it's defined.

> But in virtiofsd I see that we seem to be in mixed mode. In some cases
> we are holding ->vu_dispatch_rwlock in read-only mode. So that will
> allow multipler threads to call into library for one queue.

I think that lock is really protecting against the queue management
actions on vhost-user remapping the queue conflicting with things
operating on the queue.

> In other places like lo_setupmapping() and lo_removemapping(), we are
> not holding ->vu_dispatch_rwlock() at all and simply call into
> library vu_fs_cache_request(VHOST_USER_SLAVE_FS_MAP/...). So multiple
> threads can call in. I think precisely for this use case dev->slave_mutex
> has been introduced in library.

Note that those calls don't actually read/write interact on the queue
itself; so I don't *think* they need the vu_dispatch_rwlock.

> So few queries.
> 
> - what's the locking model needed to use libvhost-user. Is there one? 

I don't think it really had one.

> - Is it ok to selectively add locking for some data structures in
>   libvhost-user. As slave_mutex has been added. So user will have to
>   go through the code to figure out which paths can be called without
>   locks and which paths can't be.

Well it certainly needed something added; hence why I added slave_mutex,
but the slave_mutex is mostly separate from the actual queue processing,
and actually rarely used.

> /me is confused and trying to wrap my head around the locking requirements
> while using libvhost-user.

It's not well defined at all.

Dave

> 
> Vivek
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: What are libvhost-user locking requirements

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

On Wed, Jan 20, 2021 at 10:45:01AM +0000, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > - what's the locking model needed to use libvhost-user. Is there one? 
> 
> I don't think it really had one.

Same. Most libvhost-user applications are single-threaded.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [Virtio-fs] What are libvhost-user locking requirements

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

On Wed, Jan 20, 2021 at 10:45:01AM +0000, Dr. David Alan Gilbert wrote:
> * Vivek Goyal (vgoyal@redhat.com) wrote:
> > - what's the locking model needed to use libvhost-user. Is there one? 
> 
> I don't think it really had one.

Same. Most libvhost-user applications are single-threaded.

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]