Re: [PATCH -next v5 3/3] cpuset: add helpers for cpus read and cpuset_mutex locks

[Chen Ridong]

On 2025/8/26 22:43, Waiman Long wrote:
> 
> On 8/26/25 10:23 AM, Michal Koutný wrote:
>> (I wrote this yesterday before merging but I'm still sending it to give
>> my opinion ;-))
>>
>> On Mon, Aug 25, 2025 at 03:23:52AM +0000, Chen Ridong <chenridong@xxxxxxxxxxxxxxx> wrote:
>>> From: Chen Ridong <chenridong@xxxxxxxxxx>
>>>
>>> cpuset: add helpers for cpus_read_lock and cpuset_mutex locks.
>>>
>>> Replace repetitive locking patterns with new helpers:
>>> - cpuset_full_lock()
>>> - cpuset_full_unlock()
>> I don't see many precedents elsewhere in the kernel for such naming
>> (like _lock and _full_lock()). Wouldn't it be more illustrative to have
>> cpuset_read_lock() and cpuset_write_lock()? (As I'm looking at current
>> users and your accompanying comments which could be substituted with
>> the more conventional naming.)
> Good naming is always an issue. Using cpuset_read_lock/cpuset_write_lock will be more confusing as
> the current locking scheme is exclusive.
>> (Also if you decide going this direction, please mention commit
>> 111cd11bbc548 ("sched/cpuset: Bring back cpuset_mutex") in the message
>> so that it doesn't tempt to do further changes.)
>>
>>
>>> This makes the code cleaner and ensures consistent lock ordering.
>> Lock guards anyone? (When you're touching this and seeking clean code.)
> 
> Yes, I guess we can use lock guards here. You are welcome to send a patch to do that.
> 

I attempted to define the cpuset_full_lock() macro, but the initial implementation was inconsistent
with our coding conventions.
Initial version:

#define cpuset_full_lock() \
  guard(cpus_read_lock)(); \
  guard(mutex)(&cpuset_mutex);

It was suggested to use a do-while construct for proper scoping. but it could not work if we define as:

#define cpuset_full_lock() \
 do { 			   \
  guard(cpus_read_lock)(); \
  guard(mutex)(&cpuset_mutex); \
 } while(0)

So I sent this patch version.

-- 
Best regards,
Ridong