On Thu, Jan 14, 2021 at 7:14 PM Alexander Duyck
<alexander.du...@gmail.com> wrote:
>
> On Thu, Jan 14, 2021 at 4:33 PM Wei Wang <wei...@google.com> wrote:
> >
<snip>
> > +void napi_enable(struct napi_struct *n)
> > +{
> > + BUG_ON(!test_bit(NAPI_STATE_SCHED, &n->state));
> > + smp_mb__before_atomic();
> > + clear_bit(NAPI_STATE_SCHED, &n->state);
> > + clear_bit(NAPI_STATE_NPSVC, &n->state);
> > + WARN_ON(napi_set_threaded(n, n->dev->threaded));
>
> I am not sure what the point is in having a return value if you are
> just using it to trigger a WARN_ON. It might make more sense to
> actually set the WARN_ON inside of napi_set_threaded instead of having
> it here as you could then identify the error much more easily. Or for
> that matter you might be able to use something like pr_warn which
> would allow you a more detailed message about the specific netdev that
> experienced the failure.
One additional change I would make here. The call to napi_set_threaded
should be moved to before the smp_mb__before_atomic(). That way we can
guarantee that the threaded flag and task_struct pointer are visible
to all consumers before they can set NAPI_STATE_SCHED. Otherwise I
think we run the risk of a race where a napi request could fire before
we have finished configuring it.
