From: Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> To: Parav Pandit <pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> Cc: cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>, Liran Liss <liranl-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>, "Hefty, Sean" <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>, Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>, Jonathan Corbet <corbet-T1hC0tSOHrs@public.gmane.org>, james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org, serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org, Or Gerlitz <ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>, Matan Barak <matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>, raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org, akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org, linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Subject: Re: [PATCHv6 1/3] rdmacg: Added rdma cgroup controller Date: Thu, 25 Feb 2016 16:30:45 +0200 [thread overview] Message-ID: <56CF1015.4040408@mellanox.com> (raw) In-Reply-To: <CAG53R5UZb=9WR7zk2b5C_FuKmt+WdNkbcrVbW+g1-oAj6J=w_Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> On 25/02/2016 15:34, Parav Pandit wrote: > On Thu, Feb 25, 2016 at 5:33 PM, Haggai Eran <haggaie-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> wrote: >>>>> +retry: >>>>> + spin_lock(&cg->rpool_list_lock); >>>>> + rpool = find_cg_rpool_locked(cg, device); >>>>> + if (!rpool) { >>>>> + spin_unlock(&cg->rpool_list_lock); >>>>> + ret = alloc_cg_rpool(cg, device); >>>>> + if (ret) >>>>> + goto err; >>>>> + else >>>>> + goto retry; >>>> Instead of retrying after allocation of a new rpool, why not just return the >>>> newly allocated rpool (or the existing one) from alloc_cg_rpool? >>> >>> It can be done, but locking semantics just becomes difficult to >>> review/maintain with that where alloc_cg_rpool will unlock and lock >>> conditionally later on. >> Maybe I'm missing something, but couldn't you simply lock rpool_list_lock >> inside alloc_cg_rpool()? It already does that around its call to >> find_cg_rpool_locked() and the insertion to cg_list. > > No. ref_count and usage counters are updated at level where lock is > taken in charge_cg_resource(). > If I move locking rpool_list_lock inside alloc_cg_rpool, unlocking > will continue outside, alloc_cg_rpool() when its found or allocated. > As you acknowledged in below comment that this makes confusing to > lock/unlock from different context, I think current implementation > achieves both. > (a) take lock from single context > (b) keep functionality of find and alloc in two separate individual functions Okay, fair enough. >> I thought that was about functions that only locked the lock, called the >> find function, and released the lock. What I'm suggesting is to have one >> function that does "lock + find + allocate if needed + unlock", > > I had similar function in past which does, > "lock + find + allocate if needed + + inc_ref_cnt + unlock", (get_cg_rpool) > update usage_counter atomically, because other thread/process might update too. > check atomic_dec_cnt - on reaching zero, "lock + del_entry + unlock + free". > > Tejun asked to simplify this to, > > "lock + find + allocate if needed + inc_ref_cnt_without_atomic" + unlock". > which I did in this patch v6. Okay. >> and another >> function that does (under caller's lock) "check ref count + check max count + >> release rpool". > This can be done. Have one dumb basic question for thiat. > Can we call kfree() with spin_lock held? All these years I tend to > avoid doing so. > I think so. This is an old link but I think it still applies: https://lkml.org/lkml/2004/11/21/130
WARNING: multiple messages have this Message-ID (diff)
From: Haggai Eran <haggaie@mellanox.com> To: Parav Pandit <pandit.parav@gmail.com> Cc: <cgroups@vger.kernel.org>, <linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-rdma@vger.kernel.org>, Tejun Heo <tj@kernel.org>, <lizefan@huawei.com>, Johannes Weiner <hannes@cmpxchg.org>, Doug Ledford <dledford@redhat.com>, Liran Liss <liranl@mellanox.com>, "Hefty, Sean" <sean.hefty@intel.com>, Jason Gunthorpe <jgunthorpe@obsidianresearch.com>, Jonathan Corbet <corbet@lwn.net>, <james.l.morris@oracle.com>, <serge@hallyn.com>, Or Gerlitz <ogerlitz@mellanox.com>, Matan Barak <matanb@mellanox.com>, <raindel@mellanox.com>, <akpm@linux-foundation.org>, <linux-security-module@vger.kernel.org> Subject: Re: [PATCHv6 1/3] rdmacg: Added rdma cgroup controller Date: Thu, 25 Feb 2016 16:30:45 +0200 [thread overview] Message-ID: <56CF1015.4040408@mellanox.com> (raw) In-Reply-To: <CAG53R5UZb=9WR7zk2b5C_FuKmt+WdNkbcrVbW+g1-oAj6J=w_Q@mail.gmail.com> On 25/02/2016 15:34, Parav Pandit wrote: > On Thu, Feb 25, 2016 at 5:33 PM, Haggai Eran <haggaie@mellanox.com> wrote: >>>>> +retry: >>>>> + spin_lock(&cg->rpool_list_lock); >>>>> + rpool = find_cg_rpool_locked(cg, device); >>>>> + if (!rpool) { >>>>> + spin_unlock(&cg->rpool_list_lock); >>>>> + ret = alloc_cg_rpool(cg, device); >>>>> + if (ret) >>>>> + goto err; >>>>> + else >>>>> + goto retry; >>>> Instead of retrying after allocation of a new rpool, why not just return the >>>> newly allocated rpool (or the existing one) from alloc_cg_rpool? >>> >>> It can be done, but locking semantics just becomes difficult to >>> review/maintain with that where alloc_cg_rpool will unlock and lock >>> conditionally later on. >> Maybe I'm missing something, but couldn't you simply lock rpool_list_lock >> inside alloc_cg_rpool()? It already does that around its call to >> find_cg_rpool_locked() and the insertion to cg_list. > > No. ref_count and usage counters are updated at level where lock is > taken in charge_cg_resource(). > If I move locking rpool_list_lock inside alloc_cg_rpool, unlocking > will continue outside, alloc_cg_rpool() when its found or allocated. > As you acknowledged in below comment that this makes confusing to > lock/unlock from different context, I think current implementation > achieves both. > (a) take lock from single context > (b) keep functionality of find and alloc in two separate individual functions Okay, fair enough. >> I thought that was about functions that only locked the lock, called the >> find function, and released the lock. What I'm suggesting is to have one >> function that does "lock + find + allocate if needed + unlock", > > I had similar function in past which does, > "lock + find + allocate if needed + + inc_ref_cnt + unlock", (get_cg_rpool) > update usage_counter atomically, because other thread/process might update too. > check atomic_dec_cnt - on reaching zero, "lock + del_entry + unlock + free". > > Tejun asked to simplify this to, > > "lock + find + allocate if needed + inc_ref_cnt_without_atomic" + unlock". > which I did in this patch v6. Okay. >> and another >> function that does (under caller's lock) "check ref count + check max count + >> release rpool". > This can be done. Have one dumb basic question for thiat. > Can we call kfree() with spin_lock held? All these years I tend to > avoid doing so. > I think so. This is an old link but I think it still applies: https://lkml.org/lkml/2004/11/21/130
next prev parent reply other threads:[~2016-02-25 14:30 UTC|newest] Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-02-20 11:00 [PATCHv6 0/3] rdmacg: IB/core: rdma controller support Parav Pandit 2016-02-20 11:00 ` Parav Pandit 2016-02-20 11:00 ` [PATCHv6 2/3] IB/core: added support to use rdma cgroup controller Parav Pandit [not found] ` <1455966006-13774-3-git-send-email-pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-02-24 13:43 ` Haggai Eran 2016-02-24 13:43 ` Haggai Eran 2016-02-24 16:05 ` Parav Pandit [not found] ` <1455966006-13774-1-git-send-email-pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-02-20 11:00 ` [PATCHv6 1/3] rdmacg: Added " Parav Pandit 2016-02-20 11:00 ` Parav Pandit 2016-02-21 7:43 ` Leon Romanovsky 2016-02-21 11:33 ` Parav Pandit [not found] ` <CAG53R5XGJESb+_-FtjLsK+jHoEmbePLd==G2knBMOcrfdPm62Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-02-21 13:45 ` Leon Romanovsky 2016-02-21 13:45 ` Leon Romanovsky [not found] ` <20160221134518.GM30450-2ukJVAZIZ/Y@public.gmane.org> 2016-02-21 14:11 ` Parav Pandit 2016-02-21 14:11 ` Parav Pandit 2016-02-21 15:09 ` Leon Romanovsky 2016-02-21 15:15 ` Parav Pandit 2016-02-24 13:13 ` Haggai Eran 2016-02-24 13:13 ` Haggai Eran [not found] ` <56CDAC7A.6030206-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 2016-02-24 16:16 ` Parav Pandit 2016-02-24 16:16 ` Parav Pandit [not found] ` <CAG53R5Uof+Ve7CndWy=BrgtxxCisQpzP_Ls0kw=Q270DhoEsZw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-02-25 12:03 ` Haggai Eran 2016-02-25 12:03 ` Haggai Eran [not found] ` <56CEED81.7010507-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org> 2016-02-25 13:34 ` Parav Pandit 2016-02-25 13:34 ` Parav Pandit 2016-02-25 14:26 ` Parav Pandit [not found] ` <CAG53R5WxXYZof4QzuJndakzvG1+t388pDRXL2O466eEDkYJ+bw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-02-25 14:42 ` Haggai Eran 2016-02-25 14:42 ` Haggai Eran [not found] ` <CAG53R5UZb=9WR7zk2b5C_FuKmt+WdNkbcrVbW+g1-oAj6J=w_Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2016-02-25 14:30 ` Haggai Eran [this message] 2016-02-25 14:30 ` Haggai Eran 2016-02-20 11:00 ` [PATCHv6 3/3] rdmacg: Added documentation for rdmacg Parav Pandit 2016-02-20 11:00 ` Parav Pandit 2016-02-20 11:00 ` Parav Pandit [not found] ` <1455966006-13774-4-git-send-email-pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> 2016-02-24 14:26 ` Haggai Eran 2016-02-24 14:26 ` Haggai Eran 2016-02-24 15:21 ` Parav Pandit 2016-02-28 8:55 ` Haggai Eran 2016-02-28 8:55 ` Haggai Eran 2016-02-28 9:02 ` Parav Pandit 2016-02-22 4:59 ` [PATCHv6 0/3] rdmacg: IB/core: rdma controller support Parav Pandit
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=56CF1015.4040408@mellanox.com \ --to=haggaie-vpraknaxozvwk0htik3j/w@public.gmane.org \ --cc=akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org \ --cc=cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=corbet-T1hC0tSOHrs@public.gmane.org \ --cc=dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org \ --cc=hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org \ --cc=james.l.morris-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org \ --cc=jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org \ --cc=linux-doc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=liranl-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org \ --cc=lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org \ --cc=matanb-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org \ --cc=ogerlitz-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org \ --cc=pandit.parav-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org \ --cc=raindel-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org \ --cc=sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org \ --cc=serge-A9i7LUbDfNHQT0dZR+AlfA@public.gmane.org \ --cc=tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.