Re: [RFC PATCH 1/1] sched: ttwu_queue_cond: perform queued wakeups across different L2 caches

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Vincent, Mathieu,

On 8/17/2023 9:31 PM, Vincent Guittot wrote:
> On Thu, 17 Aug 2023 at 17:34, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>> Skipping queued wakeups for all logical CPUs sharing an LLC means that
>> on a 192 cores AMD EPYC 9654 96-Core Processor (over 2 sockets), groups
>> of 8 cores (16 hardware threads) end up grabbing runqueue locks of other
>> runqueues within the same group for each wakeup, causing contention on
>> the runqueue locks.
>>
>> Improve this by only considering hardware threads sharing an L2 cache as
>> candidates for skipping use of the queued wakeups.
>>
>> This results in the following benchmark improvements:
>>
>>     hackbench -g 32 -f 20 --threads --pipe -l 480000 -s 100
>>
>> from 49s to 34s. (30% speedup)
>>
>> And similarly with perf bench:
>>
>>     perf bench sched messaging -g 32 -p -t -l 100000
>>
>> from 10.9s to 7.4s (32% speedup)
>>
>> This was developed as part of the investigation into a weird regression
>> reported by AMD where adding a raw spinlock in the scheduler context
>> switch accelerated hackbench. It turned out that changing this raw
>> spinlock for a loop of 10000x cpu_relax within do_idle() had similar
>> benefits.
>>
>> This patch achieves a similar effect without busy waiting nor changing
>> anything about runqueue selection on wakeup. It considers that only
>> hardware threads sharing an L2 cache should skip the queued
>> try-to-wakeup and directly grab the target runqueue lock, rather than
>> allowing all hardware threads sharing an LLC to do so.
>>
>> I would be interested to hear feedback about performance impact of this
>> patch (improvement or regression) on other workloads and hardware,
>> especially for Intel CPUs. One thing that we might want to empirically
>> figure out from the topology is whether there is a maximum number of
>> hardware threads within an LLC below which it would make sense to use
>> the LLC rather than L2 as group within which queued wakeups can be
>> skipped.
>>
>> [ Only tested on AMD CPUs so far. ]
>>
>> Link: https://lore.kernel.org/r/09e0f469-a3f7-62ef-75a1-e64cec2dcfc5@amd.com
>> Link: https://lore.kernel.org/lkml/20230725193048.124796-1-mathieu.desnoyers@efficios.com/
>> Link: https://lore.kernel.org/lkml/20230810140635.75296-1-mathieu.desnoyers@efficios.com/
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Cc: Valentin Schneider <vschneid@redhat.com>
>> Cc: Steven Rostedt <rostedt@goodmis.org>
>> Cc: Ben Segall <bsegall@google.com>
>> Cc: Mel Gorman <mgorman@suse.de>
>> Cc: Daniel Bristot de Oliveira <bristot@redhat.com>
>> Cc: Vincent Guittot <vincent.guittot@linaro.org>
>> Cc: Juri Lelli <juri.lelli@redhat.com>
>> Cc: Swapnil Sapkal <Swapnil.Sapkal@amd.com>
>> Cc: Aaron Lu <aaron.lu@intel.com>
>> Cc: x86@kernel.org
>> ---
>>  arch/Kconfig                   |  6 ++++++
>>  arch/x86/Kconfig               |  1 +
>>  drivers/base/Kconfig           |  1 +
>>  include/linux/sched/topology.h |  3 ++-
>>  kernel/sched/core.c            | 26 +++++++++++++++++++++++---
>>  5 files changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/Kconfig b/arch/Kconfig
>> index 205fd23e0cad..e5aac1741712 100644
>> --- a/arch/Kconfig
>> +++ b/arch/Kconfig
>> @@ -340,6 +340,12 @@ config HAVE_ASM_MODVERSIONS
>>           <asm/asm-prototypes.h> to support the module versioning for symbols
>>           exported from assembly code.
>>
>> +config HAVE_CLUSTERGROUP
>> +       bool
>> +       help
>> +         This symbol should be selected by an architecture if it
>> +         implements CPU clustergroup.
>> +
>>  config HAVE_REGS_AND_STACK_ACCESS_API
>>         bool
>>         help
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index cb1031018afa..07813a1a9a58 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -299,6 +299,7 @@ config X86
>>         select FUNCTION_ALIGNMENT_4B
>>         imply IMA_SECURE_AND_OR_TRUSTED_BOOT    if EFI
>>         select HAVE_DYNAMIC_FTRACE_NO_PATCHABLE
>> +       select HAVE_CLUSTERGROUP
>>
>>  config INSTRUCTION_DECODER
>>         def_bool y
>> diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
>> index 2b8fd6bb7da0..408aaf7a4bd1 100644
>> --- a/drivers/base/Kconfig
>> +++ b/drivers/base/Kconfig
>> @@ -218,6 +218,7 @@ config DMA_FENCE_TRACE
>>
>>  config GENERIC_ARCH_TOPOLOGY
>>         bool
>> +       select HAVE_CLUSTERGROUP
>>         help
>>           Enable support for architectures common topology code: e.g., parsing
>>           CPU capacity information from DT, usage of such information for
>> diff --git a/include/linux/sched/topology.h b/include/linux/sched/topology.h
>> index 816df6cc444e..714386070463 100644
>> --- a/include/linux/sched/topology.h
>> +++ b/include/linux/sched/topology.h
>> @@ -178,7 +178,8 @@ extern void partition_sched_domains(int ndoms_new, cpumask_var_t doms_new[],
>>  cpumask_var_t *alloc_sched_domains(unsigned int ndoms);
>>  void free_sched_domains(cpumask_var_t doms[], unsigned int ndoms);
>>
>> -bool cpus_share_cache(int this_cpu, int that_cpu);
>> +bool cpus_share_cluster(int this_cpu, int that_cpu);   /* Share L2. */
>> +bool cpus_share_cache(int this_cpu, int that_cpu);     /* Share LLC. */
> 
> I think that Yicong is doing what you want with
> cpus_share_lowest_cache() which points to cluster when available or
> LLC otherwise
> https://lore.kernel.org/lkml/20220720081150.22167-1-yangyicong@hisilicon.com/t/#m0ab9fa0fe0c3779b9bbadcfbc1b643dce7cb7618
Please correct me if I'm wrong, but with Yicong's latest version
(https://lore.kernel.org/lkml/20230719092838.2302-2-yangyicong@huawei.com/)
"sd_share_id" will follow "sd_llc_id" if the SD_CLUSTER domain is
degenerated, which is the case with any system where CLUSTER domain is
same as SMT domain.

On logging the cpu and sd_share_id on my 3rd Gen EPYC system I see,

    CPU(0) sd_share_id(0)
    CPU(1) sd_share_id(0)
    CPU(2) sd_share_id(0)
    CPU(3) sd_share_id(0)
    CPU(4) sd_share_id(0)
    CPU(5) sd_share_id(0)
    CPU(6) sd_share_id(0)
    CPU(7) sd_share_id(0)
    CPU(8) sd_share_id(8)
    CPU(9) sd_share_id(8)
    ...
    CPU(127) sd_share_id(120)
    CPU(128) sd_share_id(0)
    CPU(129) sd_share_id(0)
    CPU(130) sd_share_id(0)
    CPU(131) sd_share_id(0)
    CPU(132) sd_share_id(0)
    CPU(133) sd_share_id(0)
    CPU(134) sd_share_id(0)
    CPU(135) sd_share_id(0)
    CPU(136) sd_share_id(8)
    ...

"sd_share_id" follows the "sd_llc_id" since CLUSTER domain is
degenerated.

    # echo Y > /sys/kernel/debug/sched/verbose
    # cat /sys/kernel/debug/sched/domains/cpu0/domain*/flags
    SD_BALANCE_NEWIDLE SD_BALANCE_EXEC SD_BALANCE_FORK SD_WAKE_AFFINE SD_SHARE_CPUCAPACITY SD_SHARE_PKG_RESOURCES SD_PREFER_SIBLING # SMT Domain
    SD_BALANCE_NEWIDLE SD_BALANCE_EXEC SD_BALANCE_FORK SD_WAKE_AFFINE SD_SHARE_PKG_RESOURCES SD_PREFER_SIBLING # MC Domain
    SD_BALANCE_NEWIDLE SD_BALANCE_EXEC SD_BALANCE_FORK SD_WAKE_AFFINE SD_PREFER_SIBLING # DIE Domain
    SD_BALANCE_NEWIDLE SD_BALANCE_EXEC SD_BALANCE_FORK SD_WAKE_AFFINE SD_SERIALIZE SD_OVERLAP SD_NUMA # NUMA Domain

But I believe Mathieu's case would require falling back to "core_id" if
the cluster domain has degenerated.

--
Thanks and Regards,
Prateek