On Thu, Sep 6, 2018 at 4:14 AM Vlad Buslov <[email protected]> wrote:
> > Isn't a concurrent tcf_idr_check_alloc() able to livelock here with
> > your change?
> >
> > idr_for_each_entry_ul{
> > spin_lock(&idrinfo->lock);
> > idr_remove();
> > spin_unlock(&idrinfo->lock);
> > // tcf_idr_check_alloc() jumps in,
> > // allocates next ID which can be found
> > // by idr_get_next_ul()
> > } // the whole loop goes _literately_ infinite...
>
> idr_for_each_entry_ul traverses idr entries with ascending order of
> identifiers, so infinite livelock like this is not possible because it
> never goes back to newly added entries with id<current_id.
I said "literately infinite", it could go from 1 to UINT_MAX,
sufficient to prove my point of livelock.
> >
> > Also, idr_for_each_entry_ul() is supposed to be protected either
> > by RCU or idrinfo->lock, no? With your change or without any change,
> > it doesn't even have any lock after removing RTNL?
>
> After reading this comment I checked actual idr implementation and I
> think you are right. Even though idr_for_each_entry_ul() macro (and
> function idr_get_next_ul() that it uses to iterate over idr entries)
> doesn't specify any locking requirements in comment description (that is
> why this patch doesn't use any), its implementation seems to require
> external synchronization.
Yeah, it is also a reader, so either a reader lock like RCU or a writer lock
like idrinfo->lock.
>
> You suggest I should just hold idrinfo->lock for whole del_walker loop
> duration, or play nicely with potential concurrent users and
> take/release it per action?
My suggestion is pretty clear, you just missed it, let me copy-n-paste:
With what I suggest:
spin_lock(&idrinfo->lock);
idr_for_each_entry_ul{
idr_remove();
}
spin_unlock(&idrinfo->lock);
