Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <782287bf-8334-ebbc-1fde-d4bad859e004@intel.com>
Date:   Thu, 15 Jun 2023 15:05:03 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.11.2
Subject: Re: [PATCH v4 04/24] x86/resctrl: Move rmid allocation out of
 mkdir_rdt_prepare()
Content-Language: en-US
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        <shameerali.kolothum.thodi@huawei.com>,
        D Scott Phillips OS <scott@os.amperecomputing.com>,
        <carl@os.amperecomputing.com>, <lcherian@marvell.com>,
        <bobo.shaobowang@huawei.com>, <tan.shaopeng@fujitsu.com>,
        <xingxin.hx@openanolis.org>, <baolin.wang@linux.alibaba.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Xin Hao <xhao@linux.alibaba.com>, <peternewman@google.com>,
        <dfustini@baylibre.com>
References: <20230525180209.19497-1-james.morse@arm.com>
 <20230525180209.19497-5-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230525180209.19497-5-james.morse@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Q05sQ0RPNGVWVnlNRUJJRksxaUloYzB2Q3l1VnYxQ3ZIK3VHREhFNEE5NGNq?=
 =?utf-8?B?YW5uTDVFWGQyUHJNRm9TdUk3M2o5ZEdieVpnNjY3TVp1U0pZNndBSFJMcHZr?=
 =?utf-8?B?Ly9FWFFOd2RocjM4V0pDTitqMHNSaGdhd0VhdUViTkZweUdTRmUzTEVvK0Mz?=
 =?utf-8?B?UG8vTmFlbXRmRnJPUkVhaXBMaFJ4Nnc1K3lOT0JVNTRuNHR0cHVKU2dTS3dF?=
 =?utf-8?B?K2J3cVJZeHFnR3pEU2xtYnZqWWVnSlB2aHp2bFovRUM4S3krS3RRYWtWL0FE?=
 =?utf-8?B?clpvMlNqSzA5bzlySk1rRnExUlFpbkFsV0tKVXE2bklFRUlmNVphR1JEUmRX?=
 =?utf-8?B?anNaa0pXTUJ3d1JXMkZhSUJGWm9MR200ZlVkNHFHejk4YzlNajhRRkZETnh6?=
 =?utf-8?B?TGFLU2tQUmxDTmR3NHhrM1ZoWDloM2hLSklmQkluTWZLNHdLa2ZoYkR6bnlL?=
 =?utf-8?B?dVkrU2dlb2Q3dzNOUmVXcTltMFZiei9sYWNRMVJwWjRmL2gxTitWNmlwcjVs?=
 =?utf-8?B?VVUrb1lrTXZsNVRmWkphNUgxa1RjdUxGcUVGZUJEOWt1RXdmWXN1QnpOMklp?=
 =?utf-8?B?aXlYRjJjODlLdnc1TkJtdTFjOXVwcm1LTjlRNVY0ci9lR1RSZWs1ZUVRdkFH?=
 =?utf-8?B?N2p1ejU4U1NIZXozdTgwdXJLU1l3Y1BuZHhPdWZUNDNQa2twQTR0TjZQSUp3?=
 =?utf-8?B?NjcrUldXUVlSQTE2M0NhYXduNi9JbG9ORDNVaVNoa1BLMXpzSjdDa1ZIMmdy?=
 =?utf-8?B?c20rMzRNRXFVZkF6VDBhSjk0cEZhQ3BCcVJza08rbGNWMnJUS3pqSHR3SGF3?=
 =?utf-8?B?TkcrS2VtcFppTjg0K2FOUTltU1pOaGc0V3orNUpId2M1b2dQaE9NbjR3VEZq?=
 =?utf-8?B?eVE3QkZRWlZadWgwOHBJeTdhQ2xFVytHZ2d2M082blkrbUkwV094Wlh1YUFv?=
 =?utf-8?B?SWNsWEFWNDZpcHp5cWFpK1JZNXBvekIramVJdTd2SklrUDE0NFNiVi9xNjFE?=
 =?utf-8?B?eDN2K0wwVU14NnZqVGdubmZKMmwyNlNjSitBcVcyNzErOGlwdVVYTi80cVZV?=
 =?utf-8?B?b0plZ0hjVSs3bVovRmg2bGJyaWdIS21LY2hYMW1ySjM0VVB2U1M3U1Yvcm1i?=
 =?utf-8?B?a1JFdXVHbWpaUWMwSDJmdjY0ZVc3dFFRMnp2VDNKalM4QmJlWU5LRUE0K0dG?=
 =?utf-8?B?NkZ2T1NiOGN0bmxUUEJ5czBRQ1NXbHNDZkluTitvanYvUVh3S1VLM3QvNjEx?=
 =?utf-8?B?bWFMcittak42bTh1YUpmNHo0NGtiSXhielREdDJOU0s5Q3BpbkRWUHMza2p6?=
 =?utf-8?B?bGVLQytCa28rdXkwc1ppcUhTTnpKQzk0MDQxcGhEajBISmYxeXg2OElrTEdj?=
 =?utf-8?B?MzZCdEhKdllOZDlwRmIydlhsRzM0Y2s4WnZNcmNlL0tza3BrVlhKZ05ud0ts?=
 =?utf-8?B?MngzSjN2Z2RBRmtUWDNvclM4UVZWTklVdHFMejRqMDVXMHgvYmVHMzVGSXhy?=
 =?utf-8?B?OHk2dTdNMTVVSEkyZVlVS24raGt6NWVCY1ppaXd4NnYyMm53R0pUZFR3aEgw?=
 =?utf-8?B?cWgvYjRqVnRuVnlzc0ZVQUJublBWWVk5NXhjM0gzcE44S1JqV2RMMzZra3hl?=
 =?utf-8?B?UklLbi9qRVN3alprbUZtb2JhK1grR00rb3ZrenBNL1AxUHV0NVltOERrMlhm?=
 =?utf-8?B?cTlLSlQxWTJKWllGVEJQN0tMMCtjTGJWa255VXRPVVJVTHRPREUyQnM0dmZZ?=
 =?utf-8?B?d2pVSFlyUDBhR3JQZkxTTkZWcVpmcDhQRXhNbktzMlRXRGJleWVNRDFzQkdU?=
 =?utf-8?B?OUdJM0xvOWNZWHpudnN1Yll3OE0xcnQrWmV3OXBRTVVoYVFxLzFIc0xHOWJI?=
 =?utf-8?B?bSt2NHZLU0ZWYkVINytyWmlQR3NsbmJGejZuT0RzSm1NeXczMkpBN0pQRWNq?=
 =?utf-8?B?NlFVVUExMzJDR0U5dW5yRWFFTjd3VHhJTTNodUdoY0dLMGtRUGN1MHBvOVZT?=
 =?utf-8?B?bUlvZTNHV3J5UXlnZkZMZEJVVWQ5aTF0Yk8rb1BKZXJTMFIrMGR5cVNmNU9N?=
 =?utf-8?B?SnlUaXRWMEU3NjB2WXY4Ui82cEphYXVSaGdHVG0yRi9jQUs5LzNGcDVDcmQ3?=
 =?utf-8?B?aC9TMm8xRUU0NlY1TkI1NEFwSWtYMTFHZXZ1MVVwOU4yOHpmSWJhV251bWJJ?=
 =?utf-8?B?TFE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7c726ae4-c179-4bfb-4e55-08db6dec9356
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 15 Jun 2023 22:05:05.6055
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: bSDLs5FB1+uG9EgEamHnVAwR5zSoLS8U/lUV1fnHyUOcAMslmZKz03k4qAyn3PzJ/5vSd5dgxTZ+oGhVotsFvCN6TQKwHDbR5Re0RuXpYG0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR11MB7511
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi James,

On 5/25/2023 11:01 AM, James Morse wrote:
> RMID are allocated for each monitor or control group directory, because
> each of these needs its own RMID. For control groups,
> rdtgroup_mkdir_ctrl_mon() later goes on to allocate the CLOSID.
> 
> MPAM's equivalent of RMID is not an independent number, so can't be
> allocated until the CLOSID is known. An RMID allocation for one CLOSID
> may fail, whereas another may succeed depending on how many monitor
> groups a control group has.
> 
> The RMID allocation needs to move to be after the CLOSID has been
> allocated.

This third paragraph seems redundant.

> 
> Move the RMID allocation out of mkdir_rdt_prepare() to occur in its caller,
> after the mkdir_rdt_prepare() call. This allows the RMID allocator to
> know the CLOSID.
> 
> Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Signed-off-by: James Morse <james.morse@arm.com>
> ---
> Changes since v2:
>  * Moved kernfs_activate() later to preserve atomicity of files being visible
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 35 +++++++++++++++++++-------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 05774b185eec..8346a8f2ff9f 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3176,6 +3176,12 @@ static int mkdir_rdt_prepare_rmid_alloc(struct rdtgroup *rdtgrp)
>  	return 0;
>  }
>  
> +static void mkdir_rdt_prepare_rmid_free(struct rdtgroup *rgrp)
> +{
> +	if (rdt_mon_capable)
> +		free_rmid(rgrp->closid, rgrp->mon.rmid);
> +}
> +
>  static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>  			     const char *name, umode_t mode,
>  			     enum rdt_group_type rtype, struct rdtgroup **r)
> @@ -3241,12 +3247,6 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>  		goto out_destroy;
>  	}
>  
> -	ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp);
> -	if (ret)
> -		goto out_destroy;
> -
> -	kernfs_activate(kn);
> -
>  	/*
>  	 * The caller unlocks the parent_kn upon success.
>  	 */
> @@ -3265,7 +3265,6 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>  static void mkdir_rdt_prepare_clean(struct rdtgroup *rgrp)
>  {
>  	kernfs_remove(rgrp->kn);
> -	free_rmid(rgrp->closid, rgrp->mon.rmid);
>  	rdtgroup_remove(rgrp);
>  }
>  
> @@ -3287,12 +3286,21 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn,
>  	prgrp = rdtgrp->mon.parent;
>  	rdtgrp->closid = prgrp->closid;
>  
> +	ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp);
> +	if (ret) {
> +		mkdir_rdt_prepare_clean(rdtgrp);
> +		goto out_unlock;
> +	}
> +
> +	kernfs_activate(rdtgrp->kn);
> +
>  	/*
>  	 * Add the rdtgrp to the list of rdtgrps the parent
>  	 * ctrl_mon group has to track.
>  	 */
>  	list_add_tail(&rdtgrp->mon.crdtgrp_list, &prgrp->mon.crdtgrp_list);
>  
> +out_unlock:
>  	rdtgroup_kn_unlock(parent_kn);
>  	return ret;
>  }
> @@ -3323,10 +3331,17 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
>  	ret = 0;
>  
>  	rdtgrp->closid = closid;
> -	ret = rdtgroup_init_alloc(rdtgrp);
> -	if (ret < 0)
> +
> +	ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp);
> +	if (ret)
>  		goto out_id_free;
>  
> +	kernfs_activate(rdtgrp->kn);
> +

I can see why you added this kernfs_activate() here and it certainly matches
the resctrl pattern of its use. It does seem as though resctrl often calls
kernfs_activate() as it adds files instead of a single call when all files
are ready. This is not an issue with this patch but I do think this is an
area that deserves a closer look before any more additions.

Reinette