Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759171AbcDEOBP (ORCPT ); Tue, 5 Apr 2016 10:01:15 -0400 Received: from mail-yw0-f194.google.com ([209.85.161.194]:33362 "EHLO mail-yw0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756913AbcDEOBK (ORCPT ); Tue, 5 Apr 2016 10:01:10 -0400 Date: Tue, 5 Apr 2016 10:01:07 -0400 From: Tejun Heo To: Parav Pandit Cc: cgroups@vger.kernel.org, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org, lizefan@huawei.com, Johannes Weiner , Doug Ledford , Liran Liss , "Hefty, Sean" , Jason Gunthorpe , Haggai Eran , Jonathan Corbet , james.l.morris@oracle.com, serge@hallyn.com, Or Gerlitz , Matan Barak , akpm@linux-foundation.org, linux-security-module@vger.kernel.org Subject: Re: [PATCHv10 1/3] rdmacg: Added rdma cgroup controller Message-ID: <20160405140107.GB7822@mtj.duckdns.org> References: <1458850962-16057-1-git-send-email-pandit.parav@gmail.com> <1458850962-16057-2-git-send-email-pandit.parav@gmail.com> <20160404193640.GA7822@mtj.duckdns.org> <20160405012504.GG24661@htj.duckdns.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3244 Lines: 85 Hello, Parav. On Mon, Apr 04, 2016 at 07:22:38PM -0700, Parav Pandit wrote: > > Is it actually customary to have rdma core module updated more > > frequently separate from the kernel? Out-of-tree modules being > > updated separately happens quite a bit but this is an in-kernel > > module, which usually is tightly coupled with the rest of the kernel. > > > Yes. > rdma core module is compiled as kernel module. > Its often updated for new features, fixes. > So kernel version can be one but RDMA core module(s) get updated more > frequently than kernel. As it is a fairly isolated domain, to certain extent, it could be okay to let it go. At the same time, I know that these enterprise things tend to go completely wayward and am worried about individual drivers going crazy with custom attributes in a non-sensical way. The interface this patch is proposing easily allows that and that at the cost of internal engineering flexibility. I don't really want to be caught up in a situation where we're forced to include broken usages because that's what's already out in the wild. I personally would much prefer the resources to be defined rigidly. Let's see how the discussion with Christoph evolves. > > I don't remember the details well but the code was vastly more complex > > back then and the object lifetime management was muddy at best. If I > > reviewed in a contradicting way, my apologies, but given the current > > code, it'd be better to have objects creation upfront. > > Do you mean, > try_charge() should > lock() > run loop to allocate in hierarchy, if not allocated. > run loop again to charge. > unlock() > > If so, I prefer to run the loop once. In the original review message, I mentioned creating an interface which creates the hierarchy of objects as necessary and returns the target pool with lock held, can you please give it a shot? Something like the following. pool *get_pool(...) { lock; if (target pool exists) return pool w/ lock held; create the pool hierarchically (might involve unlock); if (succeeded) return pool w/ lock held; return NULL w/o lock; } > > It isn't necessarily about speed. It makes clear that the parent > > always should exist and makes the code easier to read and update. > > It doesn't have to exist. It can get allocated when charging occurs. > Otherwise even if rdma resources are not used, it ends up allocating > rpool in hierarchy. (We talked this before) Sure, create pools only for the used combinations but do that hierarchically so that a child pool always has a parent. I can promise you that the code will read a lot better with that. > > I don't know why you end up missing basic patterns so badly. It's > > making the review and iteration process pretty painful. Please don't > > be confrontational and try to read the review comments assuming good > > will. > > > My understanding of seq_printf() being blocking call and accessing seq_printf() can be called from any context; otherwise, it would be a horrible interface to use, wouldn't it? > pool_info in spin lock context, made me allocate memory to get all > values upfront through allocation. > Now that the lock is going away, I can do what you have described above. Thanks. -- tejun