Re: [dpdk-dev] [PATCH] vhost: fix race condition in fdset_add

[Matthias Gatto <matthias.gatto@outscale.com>]

On Tue, Dec 11, 2018 at 7:11 PM Maxime Coquelin
<maxime.coquelin@redhat.com> wrote:
>
> Hi Matthias,
>
> On 12/6/18 5:00 PM, Matthias Gatto wrote:
> > fdset_add can call fdset_shrink_nolock which call fdset_move
> > concurrently to poll that is call in fdset_event_dispatch.
> >
> > This patch add a mutex to protect poll from been call at the same time
> > fdset_add call fdset_shrink_nolock.
> >
> > Signed-off-by: Matthias Gatto <matthias.gatto@outscale.com>
> > ---
> >   lib/librte_vhost/fd_man.c | 4 ++++
> >   lib/librte_vhost/fd_man.h | 1 +
> >   lib/librte_vhost/socket.c | 1 +
> >   3 files changed, 6 insertions(+)
> >
> > diff --git a/lib/librte_vhost/fd_man.c b/lib/librte_vhost/fd_man.c
> > index 38347ab..55d4856 100644
> > --- a/lib/librte_vhost/fd_man.c
> > +++ b/lib/librte_vhost/fd_man.c
> > @@ -129,7 +129,9 @@
> >       pthread_mutex_lock(&pfdset->fd_mutex);
> >       i = pfdset->num < MAX_FDS ? pfdset->num++ : -1;
> >       if (i == -1) {
> > +             pthread_mutex_lock(&pfdset->fd_pooling_mutex);
> >               fdset_shrink_nolock(pfdset);
> > +             pthread_mutex_unlock(&pfdset->fd_pooling_mutex);
> >               i = pfdset->num < MAX_FDS ? pfdset->num++ : -1;
> >               if (i == -1) {
> >                       pthread_mutex_unlock(&pfdset->fd_mutex);
> > @@ -246,7 +248,9 @@
> >               numfds = pfdset->num;
> >               pthread_mutex_unlock(&pfdset->fd_mutex);
> >
> > +             pthread_mutex_lock(&pfdset->fd_pooling_mutex);
> >               val = poll(pfdset->rwfds, numfds, 1000 /* millisecs */);
> > +             pthread_mutex_unlock(&pfdset->fd_pooling_mutex);
>
> Any reason we cannot use the existing fd_mutex?

yes, using the existing fd_mutex would block fdset_add during the polling in
fdset_event_dispatch.

here fd_pooling_mutex block only fdset_shrink_nolock inside
fdset_add which happen only in very rare occasions.

> >               if (val < 0)
> >                       continue;
> >
> > diff --git a/lib/librte_vhost/fd_man.h b/lib/librte_vhost/fd_man.h
> > index 3331bcd..3ab5cfd 100644
> > --- a/lib/librte_vhost/fd_man.h
> > +++ b/lib/librte_vhost/fd_man.h
> > @@ -24,6 +24,7 @@ struct fdset {
> >       struct pollfd rwfds[MAX_FDS];
> >       struct fdentry fd[MAX_FDS];
> >       pthread_mutex_t fd_mutex;
> > +     pthread_mutex_t fd_pooling_mutex;
> >       int num;        /* current fd number of this fdset */
> >
> >       union pipefds {
> > diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> > index d630317..cc4e748 100644
> > --- a/lib/librte_vhost/socket.c
> > +++ b/lib/librte_vhost/socket.c
> > @@ -88,6 +88,7 @@ struct vhost_user {
> >       .fdset = {
> >               .fd = { [0 ... MAX_FDS - 1] = {-1, NULL, NULL, NULL, 0} },
> >               .fd_mutex = PTHREAD_MUTEX_INITIALIZER,
> > +             .fd_pooling_mutex = PTHREAD_MUTEX_INITIALIZER,
> >               .num = 0
> >       },
> >       .vsocket_cnt = 0,
> >