[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-netdev
Subject:    Re: [PATCH net v2] net: fix race between napi kthread mode and busy poll
From:       Jakub Kicinski <kuba () kernel ! org>
Date:       2021-02-28 19:17:10
Message-ID: 20210228111710.4e82a88e () kicinski-fedora-pc1c0hjn ! dhcp ! thefacebook ! com
[Download RAW message or body]

On Sat, 27 Feb 2021 15:23:56 -0800 Wei Wang wrote:
> > > Indeed, looks like the task will be in WAKING state until it runs?
> > > We can switch the check in ____napi_schedule() from
> > >
> > >         if (thread->state == TASK_RUNNING)
> > >
> > > to
> > >
> > >         if (!(thread->state & TASK_INTERRUPTIBLE))
> > >
> > > ?  
> >
> > Hmm... I am not very sure what state the thread will be put in after
> > kthread_create(). Could it be in TASK_INTERRUPTIBLE?  
> 
> I did a printk and confirmed that the thread->state is
> TASK_UNINTERRUPTIBLE after kthread_create() is called.
> So I think if we change the above state to:
>           if (thread->state != TASK_INTERRUPTIBLE)
>                   set_bit(NAPI_STATE_SCHED_THREADED, &napi->state);
> It should work.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 6c5967e80132..43607523ee99 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1501,17 +1501,18 @@ static int napi_kthread_create(struct napi_struct *n)
>  {
>         int err = 0;
> 
> -       /* Create and wake up the kthread once to put it in
> -        * TASK_INTERRUPTIBLE mode to avoid the blocked task
> -        * warning and work with loadavg.
> +       /* Avoid waking up the kthread during creation to prevent
> +        * potential race.
>          */
> -       n->thread = kthread_run(napi_threaded_poll, n, "napi/%s-%d",
> -                               n->dev->name, n->napi_id);
> +       n->thread = kthread_create(napi_threaded_poll, n, "napi/%s-%d",
> +                                  n->dev->name, n->napi_id);

Does kthread_run() make the thread go into TASK_INTERRUPTIBLE ?
It just calls wake_up_process(), which according to a comment in the
kdoc..

 * Conceptually does:
 *
 *   If (@state & @p->state) @p->state = TASK_RUNNING.

So I think we could safely stick to kthread_run() if the condition in
at the NAPI wake point checks for INTERRUPTIBLE?
[prev in list] [next in list] [prev in thread] [next in thread]