Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v4 1/5] hugetlb: add demote hugetlb page sysfs interfaces
To:     Oscar Salvador <osalvador@suse.de>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        David Hildenbrand <david@redhat.com>,
        Michal Hocko <mhocko@suse.com>, Zi Yan <ziy@nvidia.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Naoya Horiguchi <naoya.horiguchi@linux.dev>,
        David Rientjes <rientjes@google.com>,
        "Aneesh Kumar K . V" <aneesh.kumar@linux.ibm.com>,
        Nghia Le <nghialm78@gmail.com>,
        Andrew Morton <akpm@linux-foundation.org>
References: <20211007181918.136982-1-mike.kravetz@oracle.com>
 <20211007181918.136982-2-mike.kravetz@oracle.com>
 <YV/4ZFCvoGRn2rtr@localhost.localdomain>
From:   Mike Kravetz <mike.kravetz@oracle.com>
Message-ID: <47e53389-638a-1af1-e94f-b3c7e5e7459e@oracle.com>
Date:   Fri, 8 Oct 2021 13:24:28 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.1.1
In-Reply-To: <YV/4ZFCvoGRn2rtr@localhost.localdomain>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZDAxSHB1cXNScFpNa21pSndrZjdkY2g3Tm9RRDNFT3p0dTEzdjgxbHV5SU5r?=
 =?utf-8?B?RTFOK1hYTUFRQUJaTjhqNGhobFQ3ZkZ3ZHVEYUxUOVh1dU9SbDFlQ0o5Wmlp?=
 =?utf-8?B?ZVZ1bjcyaXlPbUxHN1hEdnNjQktFSHNoeXAwRnNKN0pOeEhMbk5MVVozSktw?=
 =?utf-8?B?VVpMU1ZHdFZINkozVityUWpUbzZNRllIVU9uMi9oY3FCU3Joc1I0RVVDVCtP?=
 =?utf-8?B?TERoSlY0WjI4eGhua2I4cjlsYngrd0hUSHlhQ0lhaTg1cWNuQmZaNTVodFFu?=
 =?utf-8?B?M0gwNU1Kb3FMeEc2bnRmRWRQM1lZSEhiTXdlR1dUaW52SncwV0pTaXlqQVNY?=
 =?utf-8?B?OGo5L1VOR0t3aDVKbzg5T0tUdW5SN2toeHUrOGZ3QTF1ZVpEZXp3bW9sa1VV?=
 =?utf-8?B?cG1GOUlmaURxai9hS3hoNi9WVDlMTHo0V2thajdDdlVJRmEzV3BQcS96aGtm?=
 =?utf-8?B?VXlza1NjRWhXcVpMSUIxdnBJdG5taWw3aFVWTUFPWEMwWXdSdFBkN1htUEY0?=
 =?utf-8?B?Tm5JRTg3M0JMTjlzc1ZERlhBakQxcGQzY1pMMmFsN0JQdnBNN1FQdHRMZjB1?=
 =?utf-8?B?OEd1OXBSYTM3bjcxTVhkTFhja0o5dXY0UnZaUUZ5d2I3YUNxM2IxdXRzL2J2?=
 =?utf-8?B?T3dLTlcvQVdPYzhueFBwVFdnRzBLSldEaEU3QXVBenJmUHlPS2hnQlBhNzVD?=
 =?utf-8?B?MnNQUWdKN0F0UUs2eEg3dDU4ZGYvdnpIc2ZuZVYxL0o0aGREbTFNbnN6VHoy?=
 =?utf-8?B?YjZ0MWdNK0JYWmNLRXErdmgwR3JZb3pkV2g0VWg2L0orQ0RTUUNhUHZGSk9W?=
 =?utf-8?B?SDZLTW02aU80c3hKaDY0ODFYZnhzZFpJK3I1c3l3TGNMbGR4ZmtxcjZvOWR3?=
 =?utf-8?B?TXo4NlYwWUtTcGRkV3c1WjI5NS9TeTBBVzJ1dzNpakk1Y1pCamdFNk04eENR?=
 =?utf-8?B?dUpvUUVQV2Zhb1NXNCsyZWpYM1M1ZDdYR1B5Q0V3aStrUUdWMkYwN3lBSjcy?=
 =?utf-8?B?NEMrall3ZWh6aGRFSzlkWmx0NTdMMnlVK2ZFMEdOWWl1d3owalpobTJBQTVa?=
 =?utf-8?B?aEhkRzdndkVBNkRoazd5dlUwZ1VvMk5xc1c1Q2hMRjE5S1ZnaWg2dUxKdlhF?=
 =?utf-8?B?ZC93ME9FNFVHdkwrRCtCYksvOVgvRTJCcUhwRE1IeFllWHQrRW1EY29vOFVn?=
 =?utf-8?B?RFJGSk1EOG94QmF5TlN4MWh2dkhZRHFyaGpPd0V6YURBckh2NHBkZkhWdnh2?=
 =?utf-8?B?ZVFkUzFOTm4xNjlyMk0vWnRwMXQ1STBxR3Z2WGVuTGI1RVM4ejJxS3BzeGdZ?=
 =?utf-8?B?QjR5Snhkek9pdk45bWpmZVZTaFAwdTJCUkthV0hUMjAyaE9kdUVlVzh2U1F1?=
 =?utf-8?B?UmozZXd4L3Y4SUNJditocnFsMGNwK3czT0FMbU1JR2tPRnNnY1R3S1RIU0pI?=
 =?utf-8?B?Z0JCQ1pHNUp5MVVhSGlqd3lLUURxMXl6SFZBVzhhMnVHTVpYSE5xczNuQ3da?=
 =?utf-8?B?WVkvazJQOTMvRWhtVjg0YXA4bnhqMWpNOE9ObUxhd2E1bGZuMnlTcnJVamdO?=
 =?utf-8?B?UjArTWYrcXVUcE50cjE4M1FheWNsRDZsdWo3WFNPbExqWkZCNldldmNXamFO?=
 =?utf-8?B?V2h1dTU4Y2dPeG12cUYrUjdUSU84dUtabDkveTNVcWxzQmxFa2QwNGcyN1JC?=
 =?utf-8?B?cW1SaU13N1IrWkhlcndLekJFWDlNd3Frdy9wWGNkUGxORHNnL2VIK3Z4WVFz?=
 =?utf-8?Q?tib3QXiXp88ltbqE8f82Pc4JAAv8OZrGhJvHSxu?=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c3396a16-0d92-495c-221c-08d98a99a2ff
X-MS-Exchange-CrossTenant-AuthSource: CH2PR10MB4200.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Oct 2021 20:24:32.0915
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: EvLGzGsS1yXf2TdRTB8b0Clzh0/XaF61kX1a0qy7u7hqWofpXMzMsOlfw1gFvV1VVcqOtgPHgAycc1whJHg9FA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR10MB4857
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10131 signatures=668683
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 adultscore=0
 phishscore=0 malwarescore=0 bulkscore=0 mlxlogscore=999 mlxscore=0
 spamscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2109230001 definitions=main-2110080112
X-Proofpoint-ORIG-GUID: is_G1BQYdq19ZSZLwLlyOXFeCttwhwHv
X-Proofpoint-GUID: is_G1BQYdq19ZSZLwLlyOXFeCttwhwHv
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 10/8/21 12:51 AM, Oscar Salvador wrote:
> On Thu, Oct 07, 2021 at 11:19:14AM -0700, Mike Kravetz wrote:
>> +static ssize_t demote_store(struct kobject *kobj,
>> +	       struct kobj_attribute *attr, const char *buf, size_t len)
>> +{
>> +	unsigned long nr_demote;
>> +	unsigned long nr_available;
>> +	nodemask_t nodes_allowed, *n_mask;
>> +	struct hstate *h;
>> +	int err = 0;
>> +	int nid;
>> +
>> +	err = kstrtoul(buf, 10, &nr_demote);
>> +	if (err)
>> +		return err;
>> +	h = kobj_to_hstate(kobj, &nid);
>> +
>> +	/* Synchronize with other sysfs operations modifying huge pages */
>> +	mutex_lock(&h->resize_lock);
>> +
>> +	if (nid != NUMA_NO_NODE) {
>> +		init_nodemask_of_node(&nodes_allowed, nid);
>> +		n_mask = &nodes_allowed;
>> +	} else {
>> +		n_mask = &node_states[N_MEMORY];
>> +	}
> 
> Why this needs to be protected by the resize_lock? I do not understand
> what are we really protecting here and from what.

In general, the resize_lock prevents unexpected consequences when
multiple users are modifying the number of pages in a pool concurrently
from the proc/sysfs interfaces.  The mutex is acquired here because we
are modifying (decreasing) the pool size.

When the mutex was added, Michal asked about the need.  Theoretically,
all code making pool adjustment should be safe because at a low level
the hugetlb_lock is taken when the hstate is modified.  However, I did
point out that there is a hstate variable 'next_nid_to_alloc' which is
used outside the lock which could result in pages being allocated from
the wrong node.  One could argue that if two (root) sysadmin users are
modifying the pool concurrently, they should not be surprised by such
consequences.  The mutex seemed like a small price to avoid any such
potential issues.  It is taken here to be consistent with this model.

Coincidentally, I was running some stress testing with this series last
night and noticed some unexpected behavior.   As a result, we will also
need to take the resize_mutex of the 'target_hstate'.  This is all in
patch 5 of the series.  I will add details there.
-- 
Mike Kravetz