linux-kernel - Re: [PATCHv3 1/3] rdmacg: Added rdma cgroup controller.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160201184046.GD14091@mtj.duckdns.org>
Date:	Mon, 1 Feb 2016 13:40:46 -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 11:20:45PM +0530, Parav Pandit wrote:
> > Yes, it can.  It just becomes a part of kernel ABI that the IB stack
> > module depends on.
> >
> o.k. Its doable, but I believe its simple but its restrictive.

The whole point is to make it somewhat restrictive so that low level
drivers don't go crazy with it and the defined resources are clearly
identified and documented.

> If rest of the RDMA experts are ok with it, I will change it to do it that way.
> We get to hear their feedback before we put this as ABI.

So, I'm really not gonna go for individual drivers defining resources
on their own.  That's a trainwreck waiting to happen.  There needs to
be a lot more scrutiny than that.

> >> 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?
>
> We can set it to max. But during freeing time, checking array of 10+
> elements whether its max or not, didn't find efficient either.

So, in terms of user interface, it doesn't buy anything, right?  It's
generally a bad idea to mix implementation details like "checking 10+
elems on free" with interface decisions.  If the overhead of scanning
the array matters, it can easily be resolved by keeping the count of
non-max values.

It could be that setting all attributes to "max" is inconvenient from
interface POV (I don't think this would be a common use case tho).  If
so, please justify that, update the interface conventions along with
other similar knobs.  Don't introduce one-off custom thing.

...
> If we don't do this, and wait until device destruction, rpool just
> keeps growing!
>
> I think above is more important design aspect than just saving memory to me.
> 
> Let me know if you have different design thought here.
> (Like engineering can_attach() callback in rdma_cgroup, which I don't
> see necessary either).

Pin the css while things are in flight and free from css_free?

> > If you wanna insist on freeing them dynamically, free them when both
> > their usages and configs are nil.
> Agree. Thats what the code is doing using marking object type being default.
> If I remove the object type, as alternative,

You don't need object type for that.  Think about it again.  All you
need is a refcnt and cgroup already has a facility for that.

Thanks.

-- 
tejun