Messages in this thread | | | Date | Fri, 10 Mar 2023 16:22:16 -0800 | Subject | Re: [PATCH v2 18/18] x86/resctrl: Separate arch and fs resctrl locks | From | Reinette Chatre <> |
| |
Hi James,
On 3/6/2023 3:34 AM, James Morse wrote: > Hi Reinette, > > On 02/02/2023 23:50, Reinette Chatre wrote: >> On 1/13/2023 9:54 AM, James Morse wrote: >>> resctrl has one mutex that is taken by the architecture specific code, >>> and the filesystem parts. The two interact via cpuhp, where the >>> architecture code updates the domain list. Filesystem handlers that >>> walk the domains list should not run concurrently with the cpuhp >>> callback modifying the list. >>> >>> Exposing a lock from the filesystem code means the interface is not >>> cleanly defined, and creates the possibility of cross-architecture >>> lock ordering headaches. The interaction only exists so that certain >>> filesystem paths are serialised against cpu hotplug. The cpu hotplug >>> code already has a mechanism to do this using cpus_read_lock(). >>> >>> MPAM's monitors have an overflow interrupt, so it needs to be possible >>> to walk the domains list in irq context. RCU is ideal for this, >>> but some paths need to be able to sleep to allocate memory. >>> >>> Because resctrl_{on,off}line_cpu() take the rdtgroup_mutex as part >>> of a cpuhp callback, cpus_read_lock() must always be taken first. >>> rdtgroup_schemata_write() already does this. >>> >>> All but one of the filesystem code's domain list walkers are >>> currently protected by the rdtgroup_mutex taken in >>> rdtgroup_kn_lock_live(). The exception is rdt_bit_usage_show() >>> which takes the lock directly. >> >> The new BMEC code also. You can find it on tip's x86/cache branch, >> see mbm_total_bytes_config_write() and mbm_local_bytes_config_write(). >> >>> >>> Make the domain list protected by RCU. An architecture-specific >>> lock prevents concurrent writers. rdt_bit_usage_show() can >>> walk the domain list under rcu_read_lock(). >>> The other filesystem list walkers need to be able to sleep. >>> Add cpus_read_lock() to rdtgroup_kn_lock_live() so that the >>> cpuhp callbacks can't be invoked when file system operations are >>> occurring. >>> >>> Add lockdep_assert_cpus_held() in the cases where the >>> rdtgroup_kn_lock_live() call isn't obvious. >>> >>> Resctrl's domain online/offline calls now need to take the >>> rdtgroup_mutex themselves. > > >>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c >>> index 7896fcf11df6..dc1ba580c4db 100644 >>> --- a/arch/x86/kernel/cpu/resctrl/core.c >>> +++ b/arch/x86/kernel/cpu/resctrl/core.c >>> @@ -25,8 +25,14 @@ >>> #include <asm/resctrl.h> >>> #include "internal.h" >>> >>> -/* Mutex to protect rdtgroup access. */ >>> -DEFINE_MUTEX(rdtgroup_mutex); >>> +/* >>> + * rdt_domain structures are kfree()d when their last cpu goes offline, >>> + * and allocated when the first cpu in a new domain comes online. >>> + * The rdt_resource's domain list is updated when this happens. The domain >>> + * list is protected by RCU, but callers can also take the cpus_read_lock() >>> + * to prevent modification if they need to sleep. All writers take this mutex: >> >> Using "callers can" is not specific (compare to "callers should"). Please provide >> clear guidance on how the locks should be used. Reader may wonder "why take cpus_read_lock() >> to prevent modification, why not just take the mutex to prevent modification?" > > 'if they need to sleep' is the answer to this. I think a certain amount of background > knowledge can be assumed. My aim here wasn't to write an essay, but indicate not all > readers do the same thing. This is already the case in resctrl, and the MPAM pmu stuff > makes that worse. > > Is this more robust: > | * rdt_domain structures are kfree()d when their last cpu goes offline, > | * and allocated when the first cpu in a new domain comes online. > | * The rdt_resource's domain list is updated when this happens. Readers of > | * the domain list must either take cpus_read_lock(), or rely on an RCU > | * read-side critical section, to avoid observing concurrent modification. > | * For information about RCU, see Docuemtation/RCU/rcu.rst. > | * All writers take this mutex: > > ?
Yes, I do think this is more robust. Since you do mention, "'if they need to sleep' is the answer to this", how about "... must take cpus_read_lock() if they need to sleep, or otherwise rely on an RCU read-side critical section, ..."? I do not think it is necessary to provide a link to the documentation. If you do prefer to keep it, please note the typo.
Also, please cpu -> CPU.
>>> @@ -569,30 +579,27 @@ static void clear_closid_rmid(int cpu) >>> static int resctrl_arch_online_cpu(unsigned int cpu) >>> { >>> struct rdt_resource *r; >>> - int err; >>> >>> - mutex_lock(&rdtgroup_mutex); >>> + mutex_lock(&domain_list_lock); >>> for_each_capable_rdt_resource(r) >>> domain_add_cpu(cpu, r); >>> clear_closid_rmid(cpu); >>> + mutex_unlock(&domain_list_lock); > >> Why is clear_closid_rmid(cpu) protected by mutex? > > It doesn't need to be, its just an artefact of changing the lock, then moving the > filesystem calls out. (its doesn't need to be protected by rdtgroup_mutex today). > > If you don't think its churn, I'll move it to make it clearer. >
I do not see a problem with keeping the lock/unlock as before but if you do find that you can make the locking clearer then please do.
Reinette
| |