Message-ID: <20160131110420.GV32380@htj.duckdns.org>
Date:	Sun, 31 Jan 2016 06:04:20 -0500
From:	Tejun Heo <tj@...nel.org>
To:	Parav Pandit <pandit.parav@...il.com>
Cc:	cgroups@...r.kernel.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-rdma@...r.kernel.org,
	lizefan@...wei.com, Johannes Weiner <hannes@...xchg.org>,
	Doug Ledford <dledford@...hat.com>,
	Liran Liss <liranl@...lanox.com>,
	"Hefty, Sean" <sean.hefty@...el.com>,
	Jason Gunthorpe <jgunthorpe@...idianresearch.com>,
	Haggai Eran <haggaie@...lanox.com>,
	Jonathan Corbet <corbet@....net>, james.l.morris@...cle.com,
	serge@...lyn.com, Or Gerlitz <ogerlitz@...lanox.com>,
	Matan Barak <matanb@...lanox.com>, raindel@...lanox.com,
	akpm@...ux-foundation.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCHv3 1/3] rdmacg: Added rdma cgroup controller.

Hello, Parav.

On Sun, Jan 31, 2016 at 04:11:54PM +0530, Parav Pandit wrote:
> No. We agreed that let IB stack define in the header file that rdmacg
> can include.
> However when I started with that I realized that, such design has
> basic flaw that IB stack is compiled as loadable modules.
> cgroup which is compiled along with kernel, cannot rely on the header
> file of the loadable module, as it will lead to incompatibly and
> possible crash.

Yes, it can.  It just becomes a part of kernel ABI that the IB stack
module depends on.

> > Keep the resource types in an array in minimal way and match with the
> > information from core side.  It doesn't make any sense to use match
> > tokens in defining resources when the resource type is always fixed.
> >
> Match token is being used in other places also typically where user
> configuration is involved.
> so Match token infrastructure APIs help to avoid rewriting parser.
> What exactly is the issue in using match token?
> Resource type is fixed - sure it is with given version of loadable
> module. But if new feature/resource is added in IB stack, at that
> point new token will be added in the array.

Sure, use parser for parsing but you end up stripping =%d in other
places anyway.  Do it the other way.  Let the definitions contain
what's essential and do the extra stuff in code handling it, not the
other way around.

> Now coming to remove command's need.
> If user has previously configured limit of say mr=15. Now if wants to
> remove that configuration and don't want to bother for the limit.
> So the way, its done is by issuing "remove" command.
> Should I name is "reset".

How is this different from setting the limits to max?

> When user issues "remove" command it could still happen that there are
> active rdma resources. So we cannot really free the rpool object.
> That is freed when last resource is uncharged.
> Make sense?

Not really.

> > You're re-stating the same thing without explaining the reasoning
> > behind it.  Why is this different from other controllers?  What's the
> > justification?
> 
> As in above example of 15-85, if we keep it or create is we will end
> up allocating rpool objects for all the 100 cgroups, which might not
> be necessary. If it happens to be multi level hierarchy, it will end
> up having in each level.
> So therefore its allocated and freed dynamically.

Just keeping them around till device destruction would work just fine.
If you wanna insist on freeing them dynamically, free them when both
their usages and configs are nil.  How the object is created shouldn't
be a factor.

> If its created by user configuration, and if we free the rpool object
> when last resource is freed which is holding the configuration,
> user configuration is lost.
> Also it doesn't make much sense to have two different allocation for
> limit and usage configuration. Pointer storing overhead is more than
> the actual content.

See above.  If you want to free dynamically, free when the thing
doesn't contain any information.  Better, just don't worry about it.
It's unlikely to matter.

> > No, the locking scheme doesn't make any sense.  Except for some
> > special cases, sequence like the following indicates that the code is
> > buggy or at least silly.
> >
> Help me to understand, silly means - unacceptable?

Yes, it's a clear anti-pattern for a refcnted object.  Again, just
push the locking to the users and drop the atomics.

> Why you don't want them to be freed when there are no requester
> allocating the resource?
> Device usually stays for longer time, but applications go way and come
> back more often, so freeing them makes more sense when not in use.
> What exactly is the problem in freeing when uncharing occurs, due to
> which I should defer it to device unregistration stage?

Two things.  Freeing dynamically doesn't require creator type or
refcnting here, so the code makes no sense to me.  It's complicated
without any purpose.  Second, it's not that big a struct.  Just leave
them around till it's clear that this is problematic.  For the Nth
time in this whole thing, keep things simple.  Stop overengineering.

-- 
tejun

Powered by blists - more mailing lists