From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 1E60C15FA73 for ; Thu, 18 Apr 2024 15:21:15 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713453678; cv=none; b=WmDTDGw6O3sVJd/zJev/AI9qPR1PMm+qeg958vJwYQ+n+4/01AjU0Bl3l5o0QV3g4A9iMlzp44Lln7599NPgk100CiNOkZm2eZWUtDZ0+ZBN7ZEAyIGBJFjH3StdhgpPQKUcftBhJp49NzhiCo5hAqOtfIZtlKfa4X25jrKEH2s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1713453678; c=relaxed/simple; bh=BEQoL8Xy1DfYZX7VkjWLZGPArlBBiN1RTUiQ1OWe6kY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=iQ/LaB8NWvdPEunkLhFruHpxu4S51kI98x3WlOkHn2TYMX7ols7oOQX0JFgq3bQJnNub8bhi2AqNY2kyu99xjqUkU4k7fVxjqJQqqMyBrCCnojCqGjVf6Xijgd9o3iHMDETGV5rXgqC7ZVXeoiCHMpjUCnAi/BV99k4YEe8NuBg= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id D77092F; Thu, 18 Apr 2024 08:21:42 -0700 (PDT) Received: from e133380.arm.com (e133380.arm.com [10.1.197.52]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BCC4D3F738; Thu, 18 Apr 2024 08:21:11 -0700 (PDT) Date: Thu, 18 Apr 2024 16:21:03 +0100 From: Dave Martin To: Reinette Chatre Cc: James Morse , x86@kernel.org, linux-kernel@vger.kernel.org, Fenghua Yu , Thomas Gleixner , Ingo Molnar , Borislav Petkov , H Peter Anvin , Babu Moger , shameerali.kolothum.thodi@huawei.com, D Scott Phillips OS , carl@os.amperecomputing.com, lcherian@marvell.com, bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com, baolin.wang@linux.alibaba.com, Jamie Iles , Xin Hao , peternewman@google.com, dfustini@baylibre.com, amitsinght@marvell.com, David Hildenbrand , Rex Nie Subject: Re: [PATCH v1 05/31] x86/resctrl: Remove rdtgroup from update_cpu_closid_rmid() Message-ID: References: <20240321165106.31602-1-james.morse@arm.com> <20240321165106.31602-6-james.morse@arm.com> <6bbff669-cbe5-4284-b64a-4825a541b35f@intel.com> <0698418a-aede-40d6-90ce-dbf6e8796916@intel.com> <069c0e2e-81e1-4343-8a9f-21f1cb74bde6@intel.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <069c0e2e-81e1-4343-8a9f-21f1cb74bde6@intel.com> On Wed, Apr 17, 2024 at 10:12:35PM -0700, Reinette Chatre wrote: > Hi Dave, > > On 4/16/2024 9:16 AM, Dave Martin wrote: > > On Mon, Apr 15, 2024 at 10:47:55AM -0700, Reinette Chatre wrote: > >> On 4/12/2024 9:12 AM, Dave Martin wrote: > >>> On Mon, Apr 08, 2024 at 08:16:08PM -0700, Reinette Chatre wrote: > >>>> On 3/21/2024 9:50 AM, James Morse wrote: > .. > > >> Do you imply that this would maintain the order in this patch? It does > >> not look to me that it would but I may be looking wrong. > > > > I'm not sure without looking again, but since this discussion is not a > > good use of your time I'll just go ahead and implement the change at > > [*] above, while restoring referse FIR order, if that is good for you. > > > >> > >> sidenote: the "on_each_cpu_mask()" in update_closid_rmid() can be on > >> one line. > > > > I guess that might have been split to stick to the 80-char limit. > > > > Due the the small size of this function, shall I just rename defaults_p to p? > > Alternatively, there are already a few non-printk lines over 80 chars, so > > maybe we can tolerate one more here? > > 80 chars are not enforced so strictly that it impacts readability. You > may refer to how update_task_closid_rmid() looks for more confidence in/ > motivation for placing this on one line. Agreed. (I did in fact rename default_p to p in my fixup, which shortens the affected line as a side-effect anyway.) > > > > >> > >> .. > >> > >>>>> + * struct resctrl_cpu_sync, or NULL. > >>>>> + */ > >>>> > >>>> Updating the CPU's defaults is not the primary goal of this function and because > >>>> of that I do not think this should be the focus with the main goal (updating > >>>> RMID and CLOSID on CPU) ignored. Specifically, this function only updates > >>>> the defaults if *info is set but it _always_ ensures CPU is running with > >>>> appropriate CLOSID/RMID (which may or may not be from a CPU default). > >>>> > >>>> I think resctrl_arch_sync_cpu_closid_rmid() may be more appropriate > >>>> and the comment needs to elaborate what the function does. > >>>> > >>>>> +void resctrl_arch_sync_cpu_defaults(void *info); > >>> > >>> That seems reasonable, and follows the original naming and what the > >>> code does: > >>> > >>> What about: > >>> > >>> /** > >>> * resctrl_arch_sync_cpu_defaults() - Refresh the CPU's CLOSID and RMID. > >>> * Call via IPI. > >> > >> Did you intend to change function name? > > > > Er, yes, I meant to use your suggestion here, so: > > resctrl_arch_sync_cpu_closid_rmid(). > > > > I'm a bit confused here when comparing with your response in [1] mentioning > a change to another name. > > [1] https://lore.kernel.org/lkml/Zh6kgs1%2fbji1P1Hl@e133380.arm.com/ My bad (sorry Babu!). I read that suggestion carelessly and assumed it was aligned with Reinette's. The most important thing seems to be to transfer the "defaults" from the name of the function to the name of the struct, since the struct is about defaults (and only about defaults), while the function is about defaults and the running task. To avoid extra busy-work, I'll stick with resctrl_arch_sync_cpu_closid_rmid() for now, but I don't mind changing it if people prefer. > > Also, Babu Moger's suggestion to rename struct resctrl_cpu_sync > > to resctrl_cpu_defaults seems good, since that accurately describes what > > is specified in the struct (and what is *not* specified if NULL is > > passed). > > Sounds good, yes. > > Reinette > Cheers ---Dave