Re: [PATCH net-next v6 10/11] net: sched: atomically check-allocate action

From: Cong Wang <xiyou.wangcong@gmail.com>
To: Vlad Buslov <vladbu@mellanox.com>
Cc: Linux Kernel Network Developers <netdev@vger.kernel.org>,
	David Miller <davem@davemloft.net>,
	Jamal Hadi Salim <jhs@mojatatu.com>,
	Jiri Pirko <jiri@resnulli.us>,
	Alexei Starovoitov <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	Yevgeny Kliteynik <kliteyn@mellanox.com>,
	Jiri Pirko <jiri@mellanox.com>
Subject: Re: [PATCH net-next v6 10/11] net: sched: atomically check-allocate action
Date: Tue, 7 Aug 2018 18:20:19 -0700	[thread overview]
Message-ID: <CAM_iQpU+tWo4HVmQV+UCcLCmULd=kfsPZg+M+2rVvMbHN=2jww@mail.gmail.com> (raw)
In-Reply-To: <1530800673-12280-11-git-send-email-vladbu@mellanox.com>

On Thu, Jul 5, 2018 at 7:24 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
> Implement function that atomically checks if action exists and either takes
> reference to it, or allocates idr slot for action index to prevent
> concurrent allocations of actions with same index. Use EBUSY error pointer
> to indicate that idr slot is reserved.

A dumb question:

How could "concurrent allocations of actions with same index" happen
as you already take idrinfo->lock for the whole tcf_idr_check_alloc()??

For me, it should be only one allocation could succeed, all others
should fail.

Maybe you are trying to prevent others treat it like existing one,
but in that case you can just hold the idinfo->lock for all idr operations.

And more importantly, upper layer is able to tell it is a creation or
just replace, you don't have to check this in this complicated way.

IOW, all of these complicated code should not exist.