On Wed, 2016-08-24 at 22:11 +0200, Florian Westphal wrote:
> Eric Dumazet <eric.dumazet@xxxxxxxxx> wrote:
> > On Wed, 2016-08-24 at 13:55 +0200, Florian Westphal wrote:
> > > Conntrack gc worker to evict stale entries.
> >
> >
> > > static struct nf_conn *
> > > __nf_conntrack_alloc(struct net *net,
> > > const struct nf_conntrack_zone *zone,
> > > @@ -1527,6 +1597,7 @@ static int untrack_refs(void)
> > >
> > > void nf_conntrack_cleanup_start(void)
> > > {
> > > + conntrack_gc_work.exiting = true;
> > > RCU_INIT_POINTER(ip_ct_attach, NULL);
> > > }
> > >
> > > @@ -1536,6 +1607,9 @@ void nf_conntrack_cleanup_end(void)
> > > while (untrack_refs() > 0)
> > > schedule();
> > >
> > > + cancel_delayed_work_sync(&conntrack_gc_work.dwork);
> > > + /* can be re-scheduled once */
> >
> > Are you sure ?
> >
> > As conntrack_gc_work.exiting = true, I do not see how this can happen ?
>
> nf_conntrack_cleanup_start() sets exiting = true
>
> current cpu blocks in
>
> cancel_delayed_work_sync(&conntrack_gc_work.dwork);
>
> Iff the work queue was running on other cpu but was already past
> gc_work->exiting check then when cancel_delayed_work_sync() (first one)
> returns it will have re-armed itself via schedule_delayed_work().
>
> So I think the 2nd cancel_delayed_work_sync is needed.
>
> Let me know if you'd like to see a v3 with more verbose
> comment about this.
If you were using cancel_delayed_work() (instead of
cancel_delayed_work_sync()) I would understand the concern.
But here you are using the thing that was designed to exactly avoid the
issue, of both work running on another cpu and/or re-arming itself.
If what you are saying was true, we would have to fix hundreds of
cancel_delayed_work_sync() call sites ...
--
To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
