Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:6 as permitted sender) client-ip=2620:137:e000::3:6;
References: <20230912082101.342002-1-ying.huang@intel.com>
 <20230912082101.342002-4-ying.huang@intel.com>
User-agent: mu4e 1.8.13; emacs 28.2
From:   Alistair Popple <apopple@nvidia.com>
To:     Huang Ying <ying.huang@intel.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org, Bharata B Rao <bharata@amd.com>,
        "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>,
        Wei Xu <weixugc@google.com>,
        Dan Williams <dan.j.williams@intel.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Michal Hocko <mhocko@kernel.org>,
        Yang Shi <shy828301@gmail.com>,
        Dave Jiang <dave.jiang@intel.com>,
        Rafael J Wysocki <rafael.j.wysocki@intel.com>
Subject: Re: [PATCH -V3 3/4] acpi, hmat: calculate abstract distance with HMAT
Date:   Tue, 19 Sep 2023 15:14:10 +1000
In-reply-to: <20230912082101.342002-4-ying.huang@intel.com>
Message-ID: <8734za6869.fsf@nvdebian.thelocal>
Content-Type: text/plain
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?L7odoNvVE96lmpWLEk1JMI4WtsFQ9w9oMhQ6gQ8mfiuVDn4xM1gROgy7u8hB?=
 =?us-ascii?Q?PK0d4ZESkzsqaP1Yb4wGcFPTkuOzYJ+/CI6Hn9c3ppmfOrD6AcdkieFjkj1t?=
 =?us-ascii?Q?XA1If2kkTcO4f9bg83KihOqy7pBbqR5XQXORoxIYDyNpXPTISvMBNgPd6Dwg?=
 =?us-ascii?Q?PZ3JL1IekQjXZp8Q3DGExAKzMAJkEhl9sbhkDhg/jTIfV8zQYjnyBn83D3GP?=
 =?us-ascii?Q?22nCZ51hjVuqetHZikInN6u9KG/Lw0uQmAakRgEfAT7/4wIiJV6fKrGLxAMQ?=
 =?us-ascii?Q?lKJ+wozcsLGigDUWjfpvNCRs8Jwoi+7WBKIo3O6fh0XCnb16QrkKK48vNK0v?=
 =?us-ascii?Q?Cquat+DfHKIR8IdmS18A0DLZQXp+d3UuqdWd5/wE1HNjlVhobinb1FeOJtNj?=
 =?us-ascii?Q?AsIJcnIOiZxwRCPyFSmEq3189mzYtmcLGwsz0GJO8foHyL0ypOKfv0cBjxQ+?=
 =?us-ascii?Q?+ZPVnUpFCowLvmK+WU1KbdAEXb1TeZr1Gbtm9jaW0+k4yU8Tsn6eOhJBf9eF?=
 =?us-ascii?Q?K+PUvKB7c/j5omyOcEfkIfxrXHX/3PCLRxBCWv+Tcl8mxajOcDWtT5wchPP/?=
 =?us-ascii?Q?Uq0zri1Au5kBq2VIWKAkLpWD+SC90RIyG/yH05ZRf7nyy0huKazbKGjJByp8?=
 =?us-ascii?Q?lm5WIS4yrTwJZdOdwrmx9bzihyIbMcLYIjenIznYrvMSeyDjPF8fHdKcKS0I?=
 =?us-ascii?Q?dAxJJ09xFwm8jvdMEhb3k2C7Q8BWF3cBp5rjywNigHGCgZtcVLMSwhDunqgk?=
 =?us-ascii?Q?D3EVUbAJgrqVYuYl01fEQsvlJr0VI1uFkkOWPj+CURa2cpRi1PoTrzW8eseB?=
 =?us-ascii?Q?4JiSKrNarZduscP5K/S/2pWCHRMOxeJ/h+YSWzREOttJ+ZNElHX1/seEgLaf?=
 =?us-ascii?Q?cQzQ0XbOTz79eGU53KI9n8+sx4v2AWes9eXyajME7tbGsKSxNC8iU0dj16Bv?=
 =?us-ascii?Q?pHZ1z5AdS4tsoLY7H1YzKu+LuuQ7J9m0yCPeVPzKPrlWXeBOOkaV/i7AqFr2?=
 =?us-ascii?Q?5BbFPGsUFysu10ZJ2JjolsGDY012TNBvm21UGMaydff7DcOBTF8si3MZxn68?=
 =?us-ascii?Q?hd7X5+hWi1BULHAL27h5WupAWd6kEIZYeZ2hQCpMSOLOzYOqSneMtBdSLDZd?=
 =?us-ascii?Q?UMsYJyygrPxoOm3fjUz0xgZeMLrSLAlpVQ9K3tAAxg+wl5/9k6UQKK/aHsJL?=
 =?us-ascii?Q?0Yq85BEsNKGBAGSoUYep3UY+gi+NFdDr5gtNkbLGj8ZaWK3k4Nnmemz45aal?=
 =?us-ascii?Q?wlJgkZK4DFUzoOLtCpBkWuuPES/I7JplzLFmfZO9bZcNq8W49b2MikEncffC?=
 =?us-ascii?Q?h/hvXHbO7Cia5RuWJzKoEMuGywhmDQPrB3zMWRVniWM2DKliXTAZcIwDPg/E?=
 =?us-ascii?Q?jiUE00ZMK5z4pZb7O/9I+GthyF3aa/gDW0kPNo9+X09M83RqOx9mIAuX9sGS?=
 =?us-ascii?Q?+7yU1ljyDrBgs2qJ6vy5bHiFekGmJ0w5qibnuRIISjNS9kmi/AB2aivJVdmM?=
 =?us-ascii?Q?tF2bp+9HVwX97NbyNSEJz1QEQz/Wzke6LYAsKB+nCJq0bD+VYCBxBiCZeis+?=
 =?us-ascii?Q?hzwAb1mtXcpWGKadb9AS8IOPI17D4l3mgTl2qmIs?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 654292ba-423e-4c63-465a-08dbb8d1a5db
X-MS-Exchange-CrossTenant-AuthSource: BYAPR12MB3176.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Sep 2023 05:31:17.4552
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: /5yxDQn9RpKRVyCbNENWtyGLLf6Mnd6yUFGBrEFStM0eV971DBGRPD01cfxH3rNfnrxSFf9NbPdVr/+bawz8yg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW4PR12MB7013
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable
	autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Mon, 18 Sep 2023 22:32:25 -0700 (PDT)


Thanks for making changes here, looks better to me at least.

Huang Ying <ying.huang@intel.com> writes:

>  static __init void hmat_free_structures(void)
>  {
>  	struct memory_target *target, *tnext;
> @@ -801,6 +857,7 @@ static __init int hmat_init(void)
>  	struct acpi_table_header *tbl;
>  	enum acpi_hmat_type i;
>  	acpi_status status;
> +	int usage;
>  
>  	if (srat_disabled() || hmat_disable)
>  		return 0;
> @@ -841,7 +898,10 @@ static __init int hmat_init(void)
>  	hmat_register_targets();
>  
>  	/* Keep the table and structures if the notifier may use them */
> -	if (!hotplug_memory_notifier(hmat_callback, HMAT_CALLBACK_PRI))
> +	usage = !hotplug_memory_notifier(hmat_callback, HMAT_CALLBACK_PRI);
> +	if (!hmat_set_default_dram_perf())
> +		usage += !register_mt_adistance_algorithm(&hmat_adist_nb);
> +	if (usage)
>  		return 0;

Can we simplify the error handling here? As I understand it
hotplug_memory_notifier() and register_mt_adistance_algorithm() aren't
expected to ever fail because hmat_init() should only be called once and
the notifier register shouldn't fail. So wouldn't the below be
effectively the same thing but clearer?

    if (hotplug_memory_notifier(hmat_callback, HMAT_CALLBACK_PRI))
        goto out_put;
       
    if (!hmat_set_default_dram_perf())
        register_mt_adistance_algorithm(&hmat_adist_nb);

    return 0;

>  out_put:
>  	hmat_free_structures();

Also as an aside while looking at this patch I noticed a minor bug:

	status = acpi_get_table(ACPI_SIG_HMAT, 0, &tbl);
	if (ACPI_FAILURE(status))
		goto out_put;

This will call acpi_put_table(tbl) even though we failed to get the
table.