Received-SPF: pass (google.com: domain of linux-kernel+bounces-109657-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <18c39901-d1bb-45ab-86ae-1a74c3ff8060@virtuozzo.com>
Date: Thu, 21 Mar 2024 13:15:51 +0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mm/memcontrol: stop resize loop if limit was changed
 again
Content-Language: en-US
To: Waiman Long <longman@redhat.com>, Johannes Weiner <hannes@cmpxchg.org>,
 Michal Hocko <mhocko@kernel.org>, Roman Gushchin <roman.gushchin@linux.dev>,
 Shakeel Butt <shakeel.butt@linux.dev>, Muchun Song <muchun.song@linux.dev>,
 Andrew Morton <akpm@linux-foundation.org>
Cc: Vladimir Davydov <vdavydov.dev@gmail.com>, cgroups@vger.kernel.org,
 linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel@openvz.org
References: <20240320100556.463266-1-ptikhomirov@virtuozzo.com>
 <be05a470-bb31-47ef-b786-557c347de429@redhat.com>
From: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
In-Reply-To: <be05a470-bb31-47ef-b786-557c347de429@redhat.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?cGNIelFCMFdoUUtRbFlVREJYQ1NOQ0VlWkpJcmFXN1lSSXJ0WDFGcEVjQWtv?=
 =?utf-8?B?VFNwVGxLei91Y2RMTUZpUjh3NmlEdHlubm9xaUI5NHV2U2Q1dk1kajZEZ05D?=
 =?utf-8?B?aytkTXNPMDFzNnRaWU5xRkVhWGlPQVhkNS8rd1NTb2dPM05idWNiZnptUk5r?=
 =?utf-8?B?TmdNaVVJSzhFc0RMVlhKc0FsUDBKbkowSlV1YkN0Q3Qza2dDbXlSbU4vZzhM?=
 =?utf-8?B?Yysyc1JDdlEyUklzdzdyZDFzM01PQzNjS2tCb3R3VDZYdnhyTFBpakpMelht?=
 =?utf-8?B?Ti9ISlFLeHpwSTFyVk0rV2ZFOWJjUS9qU2p5eTdwc0Y2cWNidjhLZ0lndHYz?=
 =?utf-8?B?NVlUK0Z1T3F4cGN3T3R4c2Z6Z1Z1WmZxZUE0dlBQdml0QU1Ma0o4b0JOZDVn?=
 =?utf-8?B?V04yWlpJYkRuN3ZiL1JlamVYUm1IYnB5SXBQNG9DUmJFMWh0SzlObWtkbXhQ?=
 =?utf-8?B?am8yNFlSWGdlNDY1K0ZFeFgvRy9pYWRXdDY0QzhQcGhIdVB1bzZWZjhwWDhH?=
 =?utf-8?B?OTVRbzhPLzFxRUluQTlYc0ZiSDdyeVltWXpIUWh4MWpPL2VpUExZWkhXRXJD?=
 =?utf-8?B?Q1JXQjNDWkNkTFVueVJTeXdMMnpKWGdSNDUzdTVRTW16UTJwcjVpdWhmM2ZL?=
 =?utf-8?B?R3JCamV3RG4zSTAzS0VTUU5JdUFlVEtTTHVZdG9NZzZBTFZpYkNzY2RzUllH?=
 =?utf-8?B?d0pCQUR3ZVZxYjNSV21ENHZxT3FZa1g0Ti9hYURubm9CR09odE9wTTdlNUZw?=
 =?utf-8?B?UHFYOThRSisyOHBWMFJYL21YOHpzbmlweG9uZkhwc0tJK3dwVjl4Tm5ObUlW?=
 =?utf-8?B?Rzc1NHBoSUhVS1F2K1RDNFgrRnprK25keitJbXZOWFk1NmhHK3JmM2plaG9H?=
 =?utf-8?B?VmpvSGhsTWRJbnBTa1RLbUVWb2NRZHk1TWs3bThSQTZIUEgveHRBaGRFVTUr?=
 =?utf-8?B?ZHFxRXdyRGRLQWtjQzkwV2ExNUh5M2xqWWV1Y3BKQUZFTDJ3MHIrS3Z4VXNT?=
 =?utf-8?B?YjFPUytQaXFBUWsybUpEM2RmT1NDaFlPcDJ3UWFZYjFlcWpaTFMyTW5CV3F6?=
 =?utf-8?B?dEYxaEV1UWtUaDJYVUt5aGVJOGk1RTNlUEE1VUVpNjRaWnIxS05TUDlySjlH?=
 =?utf-8?B?VTErSVRsUmNZS0Y0YWRHam0xM2YyUnhUTlNXZWhkYjFqZFpPcHNkMXRYTUQy?=
 =?utf-8?B?Nm9IQVpBUzNaamFLZnRUNC9DZnljc3YwS1pXOGtjbURyNVE1OWRvUjFnN3Q2?=
 =?utf-8?B?ZHBycnEwc2V1YzkzRDk2MHNGaUxyVnRIcjZadjBpQ0lndGdFTWtBa3lna0dN?=
 =?utf-8?B?YnA3cDNyZDVvZVdJZ1BLcFhqUkZobjBwTzhEYW5JQVdNWUJaVGp3Yy9SMWJM?=
 =?utf-8?B?NzVhUkM3bkNncHBWL1hGdEpRV1F4MWFJY3NIK05oNXRGSnRGbmxJVElxWU0y?=
 =?utf-8?B?QWllbTRqUVg0bUd3Z3BKRC9WdEF0R2FYWE5YQjlONnBPQVBvQWFHZTFlazJS?=
 =?utf-8?B?amYwM3NpdTdBU0VvcDVRSHQvN2lTSlg0cVpIZGRGMjdjWjliei9NaDNJeFJp?=
 =?utf-8?B?RzgwbmtqeG02elBJeVBXdms3K2tsL1pucEViWFBHa0licmlYdGQxcXExQ3BS?=
 =?utf-8?B?TU9ZSlpsOTlKcHkrMXJUS1J6VVlYUDNSVHhPYzZnQ3g1bGNpbEJKcFRoV0U5?=
 =?utf-8?B?cFlPa2NwK0pSbGNuSFFuOVNKY0tYM1YzaDlnSkxsaU5VWGl6aThHOHBFSndV?=
 =?utf-8?B?RmhzMEdLQ2NXR1NVeXB3K0tRSGlTdVlxNm9TcHZXRC9RNnF4Mi9ZbkhvOXRm?=
 =?utf-8?B?cHNuTk1PWWJqbCtJSjFJRjNKSHNNUS94VEI1c3d1a2V1VEF0NERUSWhqb2Q5?=
 =?utf-8?B?dXllb2xsbXFRZU5GV1ZLeXNsbXFwZ2M2RU9CUHd2R3F2ZU11bC9idGJBUkRv?=
 =?utf-8?B?UjZRSlhMRW5nZm95OENXcFJFZ0FRQXVZNFgxQXpORWVMc0g3SHpmRHJzblpt?=
 =?utf-8?B?Ris1c0Z2bTJlS2Y3MGNYZUd1dWtJS3dXcHVEaEF3VTg2YjVEYUtzR0oyeSth?=
 =?utf-8?B?S09VdTArVEU1b0hreXU0bWFneFZlS2ZFK2lXQitUYWV4bElJRUovd3E3WE0v?=
 =?utf-8?B?ZVNscCtLN1B4QVJLSXpYLzYxQXMxdWFhU3d6LzNkTHdMU016KzdjdWpWN2Q0?=
 =?utf-8?B?R0J2ZEMyV0x2YkxqelVXdUxuSXN0Y3BBWldYaVl6U1d5OTZhZWhIRHlJT0ZE?=
 =?utf-8?B?a09vOVNaeGtraU9SdUVpd0YvTDNnPT0=?=
X-OriginatorOrg: virtuozzo.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4a051618-168e-4db5-b5da-08dc4965fec5
X-MS-Exchange-CrossTenant-AuthSource: DU0PR08MB9003.eurprd08.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Mar 2024 05:15:59.7696
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0bc7f26d-0264-416e-a6fc-8352af79c58f
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: g93jCpH2cZnrAGgmLNR/ztjyLC1+cnCJaj7JQNF0pKRhEq0Zs8RB6681EV1nABYXJP/3ZxiXjySIgmLrfrol05MjeDUFSWvgvr56Sg/5AD0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PA6PR08MB10396


On 21/03/2024 01:09, Waiman Long wrote:
> 
> On 3/20/24 06:03, Pavel Tikhomirov wrote:
>> In memory_max_write() we first set memcg->memory.max and only then
>> try to enforce it in loop. What if while we are in loop someone else
>> have changed memcg->memory.max but we are still trying to enforce
>> the old value? I believe this can lead to nasty consequence like getting
>> an oom on perfectly fine cgroup within it's limits or excess reclaim.
> 
> Concurrent write to the same cgroup control file is not possible as the 
> underlying kernfs_open_file structure has a mutex that serialize access 
> to the file. Concurrent write to different cgroup control files is 
> possible, though.

Thanks for pointing this out, now I see it, in kernfs_fop_write_iter() 
we take of->mutex before ops->write() -> cgroup_file_write(). That means 
patch is not needed.

> 
> Cheers,
> Longman
> 
>>
>> We also have exactly the same thing in memory_high_write().
>>
>> So let's stop enforcing old limits if we already have a new ones.
>>
>> Signed-off-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>
>> ---
>>   mm/memcontrol.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 61932c9215e7..81b303728491 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -6769,6 +6769,9 @@ static ssize_t memory_high_write(struct 
>> kernfs_open_file *of,
>>           unsigned long nr_pages = page_counter_read(&memcg->memory);
>>           unsigned long reclaimed;
>> +        if (memcg->memory.high != high)
>> +            break;
>> +
>>           if (nr_pages <= high)
>>               break;
>> @@ -6817,6 +6820,9 @@ static ssize_t memory_max_write(struct 
>> kernfs_open_file *of,
>>       for (;;) {
>>           unsigned long nr_pages = page_counter_read(&memcg->memory);
>> +        if (memcg->memory.max != max)
>> +            break;
>> +
>>           if (nr_pages <= max)
>>               break;
> 

-- 
Best regards, Tikhomirov Pavel
Senior Software Developer, Virtuozzo.