From mboxrd@z Thu Jan  1 00:00:00 1970
From: Cong Wang <xiyou.wangcong@gmail.com>
Subject: Re: [PATCH net-next] net: sched: change tcf_del_walker() to use
 concurrent-safe delete
Date: Thu, 6 Sep 2018 12:58:42 -0700
Message-ID: <CAM_iQpVhTeKMQmt55zuZ3vTgtrMajzDo3H-3Nw7+UdrGJCJDrA@mail.gmail.com>
References: <1535958361-6778-1-git-send-email-vladbu@mellanox.com>
 <CAM_iQpUP7zu1QyUWw+ATt=mzoeqxX8EQPqBdBhzpyTWghcXrFQ@mail.gmail.com>
 <vbfsh2q2ueh.fsf@reg-r-vrt-018-180.mtr.labs.mlnx> <CAM_iQpXSC0aNpEWPdhcBmfvP8xjvNKoX3Fn-7=r0xf2ttUt2aQ@mail.gmail.com>
 <vbfbm9cbf4o.fsf@reg-r-vrt-018-180.mtr.labs.mlnx> <CAM_iQpUY_-itn2oRQ-CEtOxZ-u1sKxCfbYWgPS1v6NTkT-NbSA@mail.gmail.com>
 <vbfworyx4hr.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Mime-Version: 1.0
Content-Type: text/plain; charset="UTF-8"
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
        Jamal Hadi Salim <jhs@mojatatu.com>,
        Jiri Pirko <jiri@resnulli.us>,
        David Miller <davem@davemloft.net>
To: Vlad Buslov <vladbu@mellanox.com>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-pf1-f196.google.com ([209.85.210.196]:43613 "EHLO
        mail-pf1-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1727955AbeIGAf5 (ORCPT
        <rfc822;netdev@vger.kernel.org>); Thu, 6 Sep 2018 20:35:57 -0400
Received: by mail-pf1-f196.google.com with SMTP id j26-v6so5800862pfi.10
        for <netdev@vger.kernel.org>; Thu, 06 Sep 2018 12:58:55 -0700 (PDT)
In-Reply-To: <vbfworyx4hr.fsf@reg-r-vrt-018-180.mtr.labs.mlnx>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Thu, Sep 6, 2018 at 4:14 AM Vlad Buslov <vladbu@mellanox.com> 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);