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 X-Spam-Level: X-Spam-Status: No, score=-10.3 required=3.0 tests=BAYES_00,DKIM_INVALID, DKIM_SIGNED,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 35C76C4338F for ; Wed, 28 Jul 2021 14:16:51 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E652D60F9C for ; Wed, 28 Jul 2021 14:16:50 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E652D60F9C Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 7295E6B0070; Wed, 28 Jul 2021 10:16:50 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6B2548D0001; Wed, 28 Jul 2021 10:16:50 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 52CC16B0072; Wed, 28 Jul 2021 10:16:50 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0041.hostedemail.com [216.40.44.41]) by kanga.kvack.org (Postfix) with ESMTP id 39B436B0070 for ; Wed, 28 Jul 2021 10:16:50 -0400 (EDT) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 3EC6A18226CF0 for ; Wed, 28 Jul 2021 14:16:48 +0000 (UTC) X-FDA: 78412197696.13.C5853DD Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf08.hostedemail.com (Postfix) with ESMTP id 97550300B56B for ; Wed, 28 Jul 2021 14:16:38 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1627481798; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=SBTvI7AyrQyvAwreHvMDRlU4GfJOEkkwow/YW2lcebI=; b=FDhT6iL/8fq31uDbFq2XHDFc5NBSBuBCoBoCrItaY5+mesKmybRN5+tQW4oIqPwxGzsaXv /8cam+4hWc4blGBLLHzDtyby1FJxEy+tDfTSKNLL1cCMG+xSjhebGgawrttHp+8iOm1h+8 kyTQT9YXKfkwfuAUSHFYBXI5gVALI3I= Received: from mail-wm1-f69.google.com (mail-wm1-f69.google.com [209.85.128.69]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-520-vipH2mVxN3e1KUOyvdJ1PA-1; Wed, 28 Jul 2021 10:16:33 -0400 X-MC-Unique: vipH2mVxN3e1KUOyvdJ1PA-1 Received: by mail-wm1-f69.google.com with SMTP id d72-20020a1c1d4b0000b029025164ff3ebfso1456624wmd.7 for ; Wed, 28 Jul 2021 07:16:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:to:cc:references:from:organization:subject :message-id:date:user-agent:mime-version:in-reply-to :content-language:content-transfer-encoding; bh=SBTvI7AyrQyvAwreHvMDRlU4GfJOEkkwow/YW2lcebI=; b=gyHG/bIO8JNBvtzUEryQS8oSvFsWbYsMtjGJvfpWkkVnwDKCMcd45x78TfsyXLxTZ8 XQPwpLUraZoe0w7Lu6Vc+K71x/ln0LwObumVzVp9VUVntBK1uS0RQo7N4B64nRQm47ll VqU55hwWhjzLfmsjVaHdE5548Nd8we0m6Cdb4h48pWz5NJx0QnZhLBY5Yc3SqfdKmY9p QYxRntPhis2Ayv2i0xlEaJHcAoJU1PFaykGhCMYLO5Do3fTEjtLVf0fki0pD5/TtjQCq olhjHrplXLf3R+VBKLKyc6GV7SFBrwpb8KoeqiRXdkd18wBoGsa9u7oPwf8pKRiAGxdl Vr2Q== X-Gm-Message-State: AOAM531V0dBSCqYdMm79Z6hvaHU0C7/bcq1KD25uRgmMgcTXU88t22Gg mwTJlXTHXAK+bcjvlC4YCkoVoFspByDxtucpYEy2vKlfBTlDnEBX4NyB+rrW7t8zNSaNc4TSJJH VTLuS/V3k2n0= X-Received: by 2002:a5d:4751:: with SMTP id o17mr23882221wrs.252.1627481792392; Wed, 28 Jul 2021 07:16:32 -0700 (PDT) X-Google-Smtp-Source: ABdhPJzp/VmSZ6MuqzGZoTa2wRKFNJXZiRxCOsLy7Zc304MtYust9qBmcciWmXmZwOF3zqp8wUvvWw== X-Received: by 2002:a5d:4751:: with SMTP id o17mr23882179wrs.252.1627481792086; Wed, 28 Jul 2021 07:16:32 -0700 (PDT) Received: from ?IPv6:2003:d8:2f0a:7f00:fad7:3bc9:69d:31f? (p200300d82f0a7f00fad73bc9069d031f.dip0.t-ipconnect.de. [2003:d8:2f0a:7f00:fad7:3bc9:69d:31f]) by smtp.gmail.com with ESMTPSA id f194sm6232701wmf.23.2021.07.28.07.16.30 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 28 Jul 2021 07:16:31 -0700 (PDT) To: Greg Kroah-Hartman Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , Vitaly Kuznetsov , "Michael S. Tsirkin" , Jason Wang , Marek Kedzierski , Hui Zhu , Pankaj Gupta , Wei Yang , Oscar Salvador , Michal Hocko , Dan Williams , Anshuman Khandual , Dave Hansen , Vlastimil Babka , Mike Rapoport , "Rafael J. Wysocki" , Len Brown , Pavel Tatashin , virtualization@lists.linux-foundation.org, linux-acpi@vger.kernel.org References: <20210723125210.29987-1-david@redhat.com> <20210723125210.29987-4-david@redhat.com> From: David Hildenbrand Organization: Red Hat Subject: Re: [PATCH v2 3/9] drivers/base/memory: introduce "memory groups" to logically group memory blocks Message-ID: <2eaa8ac5-9eaf-bd2a-ace6-3f1ac38c85ff@redhat.com> Date: Wed, 28 Jul 2021 16:16:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US X-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: 97550300B56B Authentication-Results: imf08.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b="FDhT6iL/"; spf=none (imf08.hostedemail.com: domain of david@redhat.com has no SPF policy when checking 170.10.133.124) smtp.mailfrom=david@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-Stat-Signature: jnm7cb6p94ce14jiucz9ht8nt3b43y4h X-HE-Tag: 1627481798-497348 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Hi Greg, >> =20 >> static void unregister_memory(struct memory_block *memory) >> @@ -681,6 +692,11 @@ static void unregister_memory(struct memory_block= *memory) >> =20 >> WARN_ON(xa_erase(&memory_blocks, memory->dev.id) =3D=3D NULL); >> =20 >> + if (memory->group) { >> + refcount_dec(&memory->group->refcount); >> + memory->group =3D NULL; >=20 > Who freed the memory for the group? try_remove_memory() will end up calling=20 remove_memory_block_devices()->unregister_memory(). try_remove_memory() will get called by drivers that added memory=20 previously and registered the memory groups. >> +static int register_memory_group(struct memory_group group) >> +{ >> + struct memory_group *new_group; >> + uint32_t mgid; >> + int ret; >> + >> + if (!node_possible(group.nid)) >> + return -EINVAL; >> + >> + new_group =3D kzalloc(sizeof(group), GFP_KERNEL); >> + if (!new_group) >> + return -ENOMEM; >> + *new_group =3D group; >=20 > You burried a memcpy here, why? Please be explicit as this is now a > dynamic structure. To make the two callers directly below nicer. This is a pure helper for=20 initialization. Suggestions welcome. >=20 >> + refcount_set(&new_group->refcount, 1); >=20 > Why not just use a kref? You seem to be treating it as a kref would > work, right? I shall have a look, thanks! >=20 >> + >> + ret =3D xa_alloc(&memory_groups, &mgid, new_group, xa_limit_31b, >> + GFP_KERNEL); >> + if (ret) >> + kfree(new_group); >> + return ret ? ret : mgid; >=20 > I hate ?: please spell this out: > if (ret) > return ret; > return mgid; I can avoid it in this case, but it feels kind of wrong to stick to the=20 personal preference of individuals if it's getting used all over the=20 code base and there is no clear coding style recommendation. >=20 > There, more obvious and you can read it in 10 years when you have to go > fix it up... >=20 Fair enough. >=20 >=20 >> +} >> + >> +int register_static_memory_group(int nid, unsigned long max_pages) >> +{ >> + struct memory_group group =3D { >> + .nid =3D nid, >> + .s =3D { >> + .max_pages =3D max_pages, >> + }, >> + }; >> + >> + if (!max_pages) >> + return -EINVAL; >> + return register_memory_group(group); >> +} >> +EXPORT_SYMBOL_GPL(register_static_memory_group); >=20 > Let's make our global namespace a bit nicer: > memory_group_register_static() > memory_group_register_dynamic() >=20 > and so on. Use prefixes please, not suffixes. Sure, no strong opinion, can do. >=20 >=20 >> + >> +int register_dynamic_memory_group(int nid, unsigned long unit_pages) >> +{ >> + struct memory_group group =3D { >> + .nid =3D nid, >> + .is_dynamic =3D true, >> + .d =3D { >> + .unit_pages =3D unit_pages, >> + }, >> + }; >> + >> + if (!unit_pages || !is_power_of_2(unit_pages) || >> + unit_pages < PHYS_PFN(memory_block_size_bytes())) >> + return -EINVAL; >> + return register_memory_group(group); >> +} >> +EXPORT_SYMBOL_GPL(register_dynamic_memory_group); >> + >> +int unregister_memory_group(int mgid) >> +{ >> + struct memory_group *group; >> + >> + if (mgid < 0) >> + return -EINVAL; >> + >> + group =3D xa_load(&memory_groups, mgid); >> + if (!group || refcount_read(&group->refcount) > 1) >> + return -EINVAL; >> + >> + xa_erase(&memory_groups, mgid); >> + kfree(group); >> + return 0; >> +} >> +EXPORT_SYMBOL_GPL(unregister_memory_group); >=20 > memory_group_unregister() Sure. >=20 >=20 >> + >> +struct memory_group *get_memory_group(int mgid) >> +{ >> + return xa_load(&memory_groups, mgid); >> +} >=20 > Global function? Called from mm/memory_hotplug.c:add_memory_resource(). Note that we do=20 not want to export that function to random modules. Any suggestion? >=20 >=20 >> diff --git a/include/linux/memory.h b/include/linux/memory.h >> index 97e92e8b556a..6e20a6174fe5 100644 >> --- a/include/linux/memory.h >> +++ b/include/linux/memory.h >> @@ -23,6 +23,42 @@ >> =20 >> #define MIN_MEMORY_BLOCK_SIZE (1UL << SECTION_SIZE_BITS) >> =20 >> +struct memory_group { >> + /* Nid the whole group belongs to. */ >> + int nid; >=20 > What is a "nid"? "Node id". Will clarify. >=20 >> + /* References from memory blocks + 1. */ >=20 > Blank line above this? Sure. >=20 > And put the structure comments in proper kernel doc so that others can > read them and we can verify it is correct over time. Can do. >=20 >> + refcount_t refcount; >> + /* >> + * Memory group type: static vs. dynamic. >> + * >> + * Static: All memory in the group belongs to a single unit, such as= , >> + * a DIMM. All memory belonging to the group will be added in >> + * one go and removed in one go -- it's static. >> + * >> + * Dynamic: Memory within the group is added/removed dynamically in >> + * units of the specified granularity of at least one memory block. >> + */ >> + bool is_dynamic; >> + >> + union { >> + struct { >> + /* >> + * Maximum number of pages we'll have in this static >> + * memory group. >> + */ >> + unsigned long max_pages; >> + } s; >> + struct { >> + /* >> + * Unit in pages in which memory is added/removed in >> + * this dynamic memory group. This granularity defines >> + * the alignment of a unit in physical address space. >> + */ >> + unsigned long unit_pages; >> + } d; >=20 > so is_dynamic determines which to use here? Please be explicit. Sure, can make that more explicit. Thanks for the feedback! --=20 Thanks, David / dhildenb