From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id D3644CAC582 for ; Fri, 12 Sep 2025 11:50:01 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Transfer-Encoding: Content-Type:MIME-Version:References:In-Reply-To:Message-ID:Subject:CC:To: From:Date:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=4N9sPLpj4t5dzo+52v7D21HJkD1BJ7Bm+g0TNWi0dwQ=; b=QizPa6QqcXwEqILBYEUQLswW0Y lLl9fhWgCFdrphOpMPBTJnMblE7V5zC5dNYeefBxpcRc9UjlOusRt9YdG6uHcYFQePP89Z1V1GXZO fWYw5NXqbSkslR8FXIeDC8NQi0Nulxajdq54HNJUEMpyIOosHmM7GxWAYKpD3kFxjIBv+OQi23k37 mqXjVKqoPo4NyAPFBBm3lxHZNUkYoBQ0tb10Utx/dZLiKd92mzOuBdOBiQbvFR0RtXwGx7hG3oTnW 7yCccVWq52Hry1nQ2AJ7tMp/7eQQxpgRRNW0BpxXyBKmTIMus+PESUZl8dbiMv6qTE2Xksq5TTZHi aZWQeyow==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1ux2Hr-00000008z5p-1hxv; Fri, 12 Sep 2025 11:49:55 +0000 Received: from frasgout.his.huawei.com ([185.176.79.56]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1ux2Hn-00000008z2l-38S7 for linux-arm-kernel@lists.infradead.org; Fri, 12 Sep 2025 11:49:53 +0000 Received: from mail.maildlp.com (unknown [172.18.186.216]) by frasgout.his.huawei.com (SkyGuard) with ESMTP id 4cNXfq1y1Zz6L64k; Fri, 12 Sep 2025 19:45:23 +0800 (CST) Received: from frapeml500008.china.huawei.com (unknown [7.182.85.71]) by mail.maildlp.com (Postfix) with ESMTPS id EC68E1402EA; Fri, 12 Sep 2025 19:49:39 +0800 (CST) Received: from localhost (10.203.177.15) by frapeml500008.china.huawei.com (7.182.85.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.39; Fri, 12 Sep 2025 13:49:38 +0200 Date: Fri, 12 Sep 2025 12:49:37 +0100 From: Jonathan Cameron To: James Morse CC: , , , D Scott Phillips OS , , , , , , Jamie Iles , Xin Hao , , , , David Hildenbrand , Dave Martin , Koba Ko , Shanker Donthineni , , , Rob Herring , Rohit Mathew , "Rafael Wysocki" , Len Brown , Lorenzo Pieralisi , Hanjun Guo , Sudeep Holla , Catalin Marinas , "Will Deacon" , Greg Kroah-Hartman , Danilo Krummrich , Ben Horgan Subject: Re: [PATCH v2 14/29] arm_mpam: Merge supported features during mpam_enable() into mpam_class Message-ID: <20250912124937.00004250@huawei.com> In-Reply-To: <20250910204309.20751-15-james.morse@arm.com> References: <20250910204309.20751-1-james.morse@arm.com> <20250910204309.20751-15-james.morse@arm.com> X-Mailer: Claws Mail 4.3.0 (GTK 3.24.42; x86_64-w64-mingw32) MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.203.177.15] X-ClientProxiedBy: lhrpeml500012.china.huawei.com (7.191.174.4) To frapeml500008.china.huawei.com (7.182.85.71) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20250912_044952_081020_4917470F X-CRM114-Status: GOOD ( 31.73 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, 10 Sep 2025 20:42:54 +0000 James Morse wrote: > To make a decision about whether to expose an mpam class as > a resctrl resource we need to know its overall supported > features and properties. > > Once we've probed all the resources, we can walk the tree > and produce overall values by merging the bitmaps. This > eliminates features that are only supported by some MSC > that make up a component or class. > > If bitmap properties are mismatched within a component we > cannot support the mismatched feature. > > Care has to be taken as vMSC may hold mismatched RIS. > > Signed-off-by: James Morse > Reviewed-by: Ben Horgan A trivial things inline. Reviewed-by: Jonathan Cameron > +/* > + * Combine two props fields. > + * If this is for controls that alias the same resource, it is safe to just > + * copy the values over. If two aliasing controls implement the same scheme > + * a safe value must be picked. > + * For non-aliasing controls, these control different resources, and the > + * resulting safe value must be compatible with both. When merging values in > + * the tree, all the aliasing resources must be handled first. > + * On mismatch, parent is modified. > + */ > +static void __props_mismatch(struct mpam_props *parent, > + struct mpam_props *child, bool alias) > +{ > + if (CAN_MERGE_FEAT(parent, child, mpam_feat_cpor_part, alias)) { > + parent->cpbm_wd = child->cpbm_wd; > + } else if (MISMATCHED_FEAT(parent, child, mpam_feat_cpor_part, > + cpbm_wd, alias)) { > + pr_debug("%s cleared cpor_part\n", __func__); > + mpam_clear_feature(mpam_feat_cpor_part, &parent->features); > + parent->cpbm_wd = 0; > + } > + > + if (CAN_MERGE_FEAT(parent, child, mpam_feat_mbw_part, alias)) { > + parent->mbw_pbm_bits = child->mbw_pbm_bits; > + } else if (MISMATCHED_FEAT(parent, child, mpam_feat_mbw_part, > + mbw_pbm_bits, alias)) { > + pr_debug("%s cleared mbw_part\n", __func__); > + mpam_clear_feature(mpam_feat_mbw_part, &parent->features); > + parent->mbw_pbm_bits = 0; > + } > + > + /* bwa_wd is a count of bits, fewer bits means less precision */ > + if (alias && !mpam_has_bwa_wd_feature(parent) && mpam_has_bwa_wd_feature(child)) { Seems like an overly long line given other local wrapping. > + parent->bwa_wd = child->bwa_wd; > + } else if (MISMATCHED_HELPER(parent, child, mpam_has_bwa_wd_feature, > + bwa_wd, alias)) { > + pr_debug("%s took the min bwa_wd\n", __func__); > + parent->bwa_wd = min(parent->bwa_wd, child->bwa_wd); > + } > + > + /* For num properties, take the minimum */ > + if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_csu, alias)) { > + parent->num_csu_mon = child->num_csu_mon; > + } else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_csu, > + num_csu_mon, alias)) { > + pr_debug("%s took the min num_csu_mon\n", __func__); > + parent->num_csu_mon = min(parent->num_csu_mon, child->num_csu_mon); > + } > + > + if (CAN_MERGE_FEAT(parent, child, mpam_feat_msmon_mbwu, alias)) { > + parent->num_mbwu_mon = child->num_mbwu_mon; > + } else if (MISMATCHED_FEAT(parent, child, mpam_feat_msmon_mbwu, > + num_mbwu_mon, alias)) { > + pr_debug("%s took the min num_mbwu_mon\n", __func__); > + parent->num_mbwu_mon = min(parent->num_mbwu_mon, child->num_mbwu_mon); > + } > + > +/* > + * If a vmsc doesn't match class feature/configuration, do the right thing(tm). > + * For 'num' properties we can just take the minimum. > + * For properties where the mismatched unused bits would make a difference, we > + * nobble the class feature, as we can't configure all the resources. > + * e.g. The L3 cache is composed of two resources with 13 and 17 portion > + * bitmaps respectively. > + */ > +static void > +__class_props_mismatch(struct mpam_class *class, struct mpam_vmsc *vmsc) I'm not really sure what the __ prefix denotes here. > +{ > + struct mpam_props *cprops = &class->props; > + struct mpam_props *vprops = &vmsc->props; > + > + lockdep_assert_held(&mpam_list_lock); /* we modify class */ > + > + pr_debug("%s: Merging features for class:0x%lx &= vmsc:0x%lx\n", > + dev_name(&vmsc->msc->pdev->dev), > + (long)cprops->features, (long)vprops->features); According to https://docs.kernel.org/core-api/printk-formats.html should be fine using %x for u16 values. So why dance through a cast to long? > + > + /* Take the safe value for any common features */ > + __props_mismatch(cprops, vprops, false); > +} > + > +static void > +__vmsc_props_mismatch(struct mpam_vmsc *vmsc, struct mpam_msc_ris *ris) > +{ > + struct mpam_props *rprops = &ris->props; > + struct mpam_props *vprops = &vmsc->props; > + > + lockdep_assert_held(&mpam_list_lock); /* we modify vmsc */ > + > + pr_debug("%s: Merging features for vmsc:0x%lx |= ris:0x%lx\n", > + dev_name(&vmsc->msc->pdev->dev), > + (long)vprops->features, (long)rprops->features); Same as above comment on casts being unnecessary. > + > + /* > + * Merge mismatched features - Copy any features that aren't common, > + * but take the safe value for any common features. > + */ > + __props_mismatch(vprops, rprops, true); > +} > + > +/* > + * Copy the first component's first vMSC's properties and features to the > + * class. __class_props_mismatch() will remove conflicts. > + * It is not possible to have a class with no components, or a component with > + * no resources. The vMSC properties have already been built. If it's not possible do we need the defensive _or_null and error checks? > + */ > +static void mpam_enable_init_class_features(struct mpam_class *class) > +{ > + struct mpam_vmsc *vmsc; > + struct mpam_component *comp; > + > + comp = list_first_entry_or_null(&class->components, > + struct mpam_component, class_list); > + if (WARN_ON(!comp)) > + return; > + > + vmsc = list_first_entry_or_null(&comp->vmsc, > + struct mpam_vmsc, comp_list); > + if (WARN_ON(!vmsc)) > + return; > + > + class->props = vmsc->props; > +} > +/* > + * Merge all the common resource features into class. > + * vmsc features are bitwise-or'd together, this must be done first. I'm not sure what 'this' is here - I think it's a missing plural that has me confused. Perhaps 'these must be done first.' > + * Next the class features are the bitwise-and of all the vmsc features. > + * Other features are the min/max as appropriate. > + * > + * To avoid walking the whole tree twice, the class->nrdy_usec property is > + * updated when working with the vmsc as it is a max(), and doesn't need > + * initialising first. Perhaps state that this comment is about what happens in each call of mpam_enable_merge_vmsc_features() Or move the comment to that function. > + */ > +static void mpam_enable_merge_features(struct list_head *all_classes_list) > +{ > + struct mpam_class *class; > + struct mpam_component *comp; > + > + lockdep_assert_held(&mpam_list_lock); > + > + list_for_each_entry(class, all_classes_list, classes_list) { > + list_for_each_entry(comp, &class->components, class_list) > + mpam_enable_merge_vmsc_features(comp); > + > + mpam_enable_init_class_features(class); > + > + list_for_each_entry(comp, &class->components, class_list) > + mpam_enable_merge_class_features(comp); > + } > +}