RE: [PATCH V15 11/11] x86,cgroup/intel_rdt : Add a cgroup interface to manage Intel cache allocation

["Yu, Fenghua" <fenghua.yu@intel.com>]

> From: Marcelo Tosatti [mailto:mtosatti@redhat.com]
> Sent: Wednesday, November 18, 2015 1:27 PM
> To: Yu, Fenghua <fenghua.yu@intel.com>
> Cc: H Peter Anvin <hpa@zytor.com>; Ingo Molnar <mingo@redhat.com>;
> Thomas Gleixner <tglx@linutronix.de>; Peter Zijlstra
> <peterz@infradead.org>; linux-kernel <linux-kernel@vger.kernel.org>; x86
> <x86@kernel.org>; Vikas Shivappa <vikas.shivappa@linux.intel.com>
> Subject: Re: [PATCH V15 11/11] x86,cgroup/intel_rdt : Add a cgroup interface
> to manage Intel cache allocation
> 
> On Thu, Oct 01, 2015 at 11:09:45PM -0700, Fenghua Yu wrote:
> > Add a new cgroup 'intel_rdt' to manage cache allocation. Each cgroup
> +	/*
> +	 * Try to get a reference for a different CLOSid and release the
> +	 * reference to the current CLOSid.
> +	 * Need to put down the reference here and get it back in case we
> +	 * run out of closids. Otherwise we run into a problem when
> +	 * we could be using the last closid that could have been available.
> +	 */
> +	closid_put(ir->closid);
> +	if (cbm_search(cbmvalue, &closid)) {
> 
> Can't you move closid_put here?

No. This cannot be moved here.

If it's moved here, it won't work in the case of the current rdt is the only usage of the closid.
In this case, the closid will released from the cbm table and a new cbm will be allocated.
So the closid_put() is in the right place and can handle both the only usage of closid or recycling
case, I think.

> 
> +		ir->closid = closid;
> +		closid_get(closid);
> +	} else {
> +		closid = ir->closid;
> 
> Variable unused.

You are right. I'll remove this statement.

> 
> +		err = closid_alloc(&ir->closid);
> +		if (err) {
> +			closid_get(ir->closid);
> +			goto out;
> +		}
> 
> This makes you cycle closid when changing the cbm, not necessary.
> (not very important, but closid_put is nerving because it can possibly set
> l3_cbm to zero).

I think the current code is ok. If closid_put sets l3_cbm to zero (i.e. the closid only has this usage),
a new closid allocaation will be started to get a new closid.

Thanks.

-Fenghua