diff mbox series

[v1,1/2] padata: downgrade padata_do_multithreaded to serial execution for non-SMP

Message ID 20240213111347.3189206-2-gang.li@linux.dev
State New
Headers
Series hugetlb: two small improvements of hugetlb init parallelization |

Commit Message

Gang Li Feb. 13, 2024, 11:13 a.m. UTC 
  Randy Dunlap and kernel test robot reported a warning:

```
WARNING: unmet direct dependencies detected for PADATA
  Depends on [n]: SMP [=n]
  Selected by [y]:
  - HUGETLBFS [=y] && (X86 [=y] || SPARC64 || ARCH_SUPPORTS_HUGETLBFS [=n] || BROKEN [=n]) && (SYSFS [=y] || SYSCTL [=n])
```

hugetlb parallelization depends on PADATA, and PADATA depends on SMP.

PADATA consists of two distinct functionality: One part is
padata_do_multithreaded which disregards order and simply divides
tasks into several groups for parallel execution. Hugetlb
init parallelization depends on padata_do_multithreaded.

The other part is composed of a set of APIs that, while handling data in
an out-of-order parallel manner, can eventually return the data with
ordered sequence. Currently Only `crypto/pcrypt.c` use them.

All users of PADATA of non-SMP case currently only use
padata_do_multithreaded. It is easy to implement a serial one in
include/linux/padata.h. And it is not necessary to implement another
functionality unless the only user of crypto/pcrypt.c does not depend on
SMP in the future.

Fixes: a2cefb08be66 ("hugetlb: have CONFIG_HUGETLBFS select CONFIG_PADATA")
Reported-by: Randy Dunlap <rdunlap@infradead.org>
Closes: https://lore.kernel.org/lkml/ec5dc528-2c3c-4444-9e88-d2c48395b433@infradead.org/
Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202402020454.6EPkP1hi-lkp@intel.com/
Signed-off-by: Gang Li <ligang.bdlg@bytedance.com>
---
 fs/Kconfig             |  2 +-
 include/linux/padata.h | 13 +++++++++----
 2 files changed, 10 insertions(+), 5 deletions(-)

Comments

Muchun Song Feb. 13, 2024, 2:52 p.m. UTC | #1 
On 2024/2/13 19:13, Gang Li wrote:
> Randy Dunlap and kernel test robot reported a warning:
>
> ```
> WARNING: unmet direct dependencies detected for PADATA
>    Depends on [n]: SMP [=n]
>    Selected by [y]:
>    - HUGETLBFS [=y] && (X86 [=y] || SPARC64 || ARCH_SUPPORTS_HUGETLBFS [=n] || BROKEN [=n]) && (SYSFS [=y] || SYSCTL [=n])
> ```
>
> hugetlb parallelization depends on PADATA, and PADATA depends on SMP.
>
> PADATA consists of two distinct functionality: One part is
> padata_do_multithreaded which disregards order and simply divides
> tasks into several groups for parallel execution. Hugetlb
> init parallelization depends on padata_do_multithreaded.
>
> The other part is composed of a set of APIs that, while handling data in
> an out-of-order parallel manner, can eventually return the data with
> ordered sequence. Currently Only `crypto/pcrypt.c` use them.
>
> All users of PADATA of non-SMP case currently only use
> padata_do_multithreaded. It is easy to implement a serial one in
> include/linux/padata.h. And it is not necessary to implement another
> functionality unless the only user of crypto/pcrypt.c does not depend on
> SMP in the future.
>
> Fixes: a2cefb08be66 ("hugetlb: have CONFIG_HUGETLBFS select CONFIG_PADATA")
> Reported-by: Randy Dunlap <rdunlap@infradead.org>
> Closes: https://lore.kernel.org/lkml/ec5dc528-2c3c-4444-9e88-d2c48395b433@infradead.org/
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202402020454.6EPkP1hi-lkp@intel.com/
> Signed-off-by: Gang Li <ligang.bdlg@bytedance.com>
> ---
>   fs/Kconfig             |  2 +-
>   include/linux/padata.h | 13 +++++++++----
>   2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/fs/Kconfig b/fs/Kconfig
> index 4a51331f172e5..7963939592d70 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -261,7 +261,7 @@ menuconfig HUGETLBFS
>   	depends on X86 || SPARC64 || ARCH_SUPPORTS_HUGETLBFS || BROKEN
>   	depends on (SYSFS || SYSCTL)
>   	select MEMFD_CREATE
> -	select PADATA
> +	select PADATA if SMP

I'd like to drop this dependence since HugeTLB does not depend
on PADATA anymore. If some users take care about the kernel
image size, it also can disable PADATA individually.

>   	help
>   	  hugetlbfs is a filesystem backing for HugeTLB pages, based on
>   	  ramfs. For architectures that support it, say Y here and read
> diff --git a/include/linux/padata.h b/include/linux/padata.h
> index 8f418711351bc..7b84eb7d73e7f 100644
> --- a/include/linux/padata.h
> +++ b/include/linux/padata.h
> @@ -180,10 +180,6 @@ struct padata_instance {
>   
>   #ifdef CONFIG_PADATA
>   extern void __init padata_init(void);
> -#else
> -static inline void __init padata_init(void) {}
> -#endif
> -
>   extern struct padata_instance *padata_alloc(const char *name);
>   extern void padata_free(struct padata_instance *pinst);
>   extern struct padata_shell *padata_alloc_shell(struct padata_instance *pinst);
> @@ -194,4 +190,13 @@ extern void padata_do_serial(struct padata_priv *padata);
>   extern void __init padata_do_multithreaded(struct padata_mt_job *job);
>   extern int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
>   			      cpumask_var_t cpumask);
> +#else
> +static inline void __init padata_init(void) {}
> +static inline void __init padata_do_multithreaded(struct padata_mt_job *job)
> +{
> +	if (job->size)

I think we could drop this check, at least now there is no users will
pass a zero of ->size to this function, and even if someone does in the
future, I think it is really a corner case, it is unnecessary to optimize
it and ->thread_fn is supporsed to handle case of zero size if it dose
pass a zero size.

Thanks.

> +		job->thread_fn(job->start, job->start + job->size, job->fn_arg);
> +}
> +#endif
> +
>   #endif
Gang Li Feb. 13, 2024, 3:15 p.m. UTC | #2 
On 2024/2/13 22:52, Muchun Song wrote:
> On 2024/2/13 19:13, Gang Li wrote:
>> Randy Dunlap and kernel test robot reported a warning:
>>
>> ```
>> WARNING: unmet direct dependencies detected for PADATA
>>    Depends on [n]: SMP [=n]
>>    Selected by [y]:
>>    - HUGETLBFS [=y] && (X86 [=y] || SPARC64 || ARCH_SUPPORTS_HUGETLBFS 
>> [=n] || BROKEN [=n]) && (SYSFS [=y] || SYSCTL [=n])
>> ```
>>
>> hugetlb parallelization depends on PADATA, and PADATA depends on SMP.
>>
>> PADATA consists of two distinct functionality: One part is
>> padata_do_multithreaded which disregards order and simply divides
>> tasks into several groups for parallel execution. Hugetlb
>> init parallelization depends on padata_do_multithreaded.
>>
>> The other part is composed of a set of APIs that, while handling data in
>> an out-of-order parallel manner, can eventually return the data with
>> ordered sequence. Currently Only `crypto/pcrypt.c` use them.
>>
>> All users of PADATA of non-SMP case currently only use
>> padata_do_multithreaded. It is easy to implement a serial one in
>> include/linux/padata.h. And it is not necessary to implement another
>> functionality unless the only user of crypto/pcrypt.c does not depend on
>> SMP in the future.
>>
>> Fixes: a2cefb08be66 ("hugetlb: have CONFIG_HUGETLBFS select 
>> CONFIG_PADATA")
>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
>> Closes: 
>> https://lore.kernel.org/lkml/ec5dc528-2c3c-4444-9e88-d2c48395b433@infradead.org/
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: 
>> https://lore.kernel.org/oe-kbuild-all/202402020454.6EPkP1hi-lkp@intel.com/
>> Signed-off-by: Gang Li <ligang.bdlg@bytedance.com>
>> ---
>>   fs/Kconfig             |  2 +-
>>   include/linux/padata.h | 13 +++++++++----
>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/Kconfig b/fs/Kconfig
>> index 4a51331f172e5..7963939592d70 100644
>> --- a/fs/Kconfig
>> +++ b/fs/Kconfig
>> @@ -261,7 +261,7 @@ menuconfig HUGETLBFS
>>       depends on X86 || SPARC64 || ARCH_SUPPORTS_HUGETLBFS || BROKEN
>>       depends on (SYSFS || SYSCTL)
>>       select MEMFD_CREATE
>> -    select PADATA
>> +    select PADATA if SMP
> 
> I'd like to drop this dependence since HugeTLB does not depend
> on PADATA anymore. If some users take care about the kernel
> image size, it also can disable PADATA individually.
> 

Only CRYPTO_PCRYPT, HUGETLBFS and DEFERRED_STRUCT_PAGE_INIT select
PADATA. If drop this dependence, hugetlb init parallelization may not
work at all.

Maybe we can set PADATA enabled on default?

>>       help
>>         hugetlbfs is a filesystem backing for HugeTLB pages, based on
>>         ramfs. For architectures that support it, say Y here and read
>> diff --git a/include/linux/padata.h b/include/linux/padata.h
>> index 8f418711351bc..7b84eb7d73e7f 100644
>> --- a/include/linux/padata.h
>> +++ b/include/linux/padata.h
>> @@ -180,10 +180,6 @@ struct padata_instance {
>>   #ifdef CONFIG_PADATA
>>   extern void __init padata_init(void);
>> -#else
>> -static inline void __init padata_init(void) {}
>> -#endif
>> -
>>   extern struct padata_instance *padata_alloc(const char *name);
>>   extern void padata_free(struct padata_instance *pinst);
>>   extern struct padata_shell *padata_alloc_shell(struct 
>> padata_instance *pinst);
>> @@ -194,4 +190,13 @@ extern void padata_do_serial(struct padata_priv 
>> *padata);
>>   extern void __init padata_do_multithreaded(struct padata_mt_job *job);
>>   extern int padata_set_cpumask(struct padata_instance *pinst, int 
>> cpumask_type,
>>                     cpumask_var_t cpumask);
>> +#else
>> +static inline void __init padata_init(void) {}
>> +static inline void __init padata_do_multithreaded(struct 
>> padata_mt_job *job)
>> +{
>> +    if (job->size)
> 
> I think we could drop this check, at least now there is no users will
> pass a zero of ->size to this function, and even if someone does in the
> future, I think it is really a corner case, it is unnecessary to optimize
> it and ->thread_fn is supporsed to handle case of zero size if it dose
> pass a zero size.
> 
> Thanks.
> 
>> +        job->thread_fn(job->start, job->start + job->size, job->fn_arg);
>> +}
>> +#endif
>> +
>>   #endif
>
Muchun Song Feb. 15, 2024, 10:49 a.m. UTC | #3 
> On Feb 13, 2024, at 23:15, Gang Li <gang.li@linux.dev> wrote:
> 
> 
> 
>> On 2024/2/13 22:52, Muchun Song wrote:
>>> On 2024/2/13 19:13, Gang Li wrote:
>>> Randy Dunlap and kernel test robot reported a warning:
>>> 
>>> ```
>>> WARNING: unmet direct dependencies detected for PADATA
>>>    Depends on [n]: SMP [=n]
>>>    Selected by [y]:
>>>    - HUGETLBFS [=y] && (X86 [=y] || SPARC64 || ARCH_SUPPORTS_HUGETLBFS [=n] || BROKEN [=n]) && (SYSFS [=y] || SYSCTL [=n])
>>> ```
>>> 
>>> hugetlb parallelization depends on PADATA, and PADATA depends on SMP.
>>> 
>>> PADATA consists of two distinct functionality: One part is
>>> padata_do_multithreaded which disregards order and simply divides
>>> tasks into several groups for parallel execution. Hugetlb
>>> init parallelization depends on padata_do_multithreaded.
>>> 
>>> The other part is composed of a set of APIs that, while handling data in
>>> an out-of-order parallel manner, can eventually return the data with
>>> ordered sequence. Currently Only `crypto/pcrypt.c` use them.
>>> 
>>> All users of PADATA of non-SMP case currently only use
>>> padata_do_multithreaded. It is easy to implement a serial one in
>>> include/linux/padata.h. And it is not necessary to implement another
>>> functionality unless the only user of crypto/pcrypt.c does not depend on
>>> SMP in the future.
>>> 
>>> Fixes: a2cefb08be66 ("hugetlb: have CONFIG_HUGETLBFS select CONFIG_PADATA")
>>> Reported-by: Randy Dunlap <rdunlap@infradead.org>
>>> Closes: https://lore.kernel.org/lkml/ec5dc528-2c3c-4444-9e88-d2c48395b433@infradead.org/
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Closes: https://lore.kernel.org/oe-kbuild-all/202402020454.6EPkP1hi-lkp@intel.com/
>>> Signed-off-by: Gang Li <ligang.bdlg@bytedance.com>
>>> ---
>>>   fs/Kconfig             |  2 +-
>>>   include/linux/padata.h | 13 +++++++++----
>>>   2 files changed, 10 insertions(+), 5 deletions(-)
>>> 
>>> diff --git a/fs/Kconfig b/fs/Kconfig
>>> index 4a51331f172e5..7963939592d70 100644
>>> --- a/fs/Kconfig
>>> +++ b/fs/Kconfig
>>> @@ -261,7 +261,7 @@ menuconfig HUGETLBFS
>>>       depends on X86 || SPARC64 || ARCH_SUPPORTS_HUGETLBFS || BROKEN
>>>       depends on (SYSFS || SYSCTL)
>>>       select MEMFD_CREATE
>>> -    select PADATA
>>> +    select PADATA if SMP
>> I'd like to drop this dependence since HugeTLB does not depend
>> on PADATA anymore. If some users take care about the kernel
>> image size, it also can disable PADATA individually.
> 
> Only CRYPTO_PCRYPT, HUGETLBFS and DEFERRED_STRUCT_PAGE_INIT select
> PADATA. If drop this dependence, hugetlb init parallelization may not
> work at all.

Oh, right. In this case, maybe current choice is better.

> 
> Maybe we can set PADATA enabled on default?
> 
>>>       help
>>>         hugetlbfs is a filesystem backing for HugeTLB pages, based on
>>>         ramfs. For architectures that support it, say Y here and read
>>> diff --git a/include/linux/padata.h b/include/linux/padata.h
>>> index 8f418711351bc..7b84eb7d73e7f 100644
>>> --- a/include/linux/padata.h
>>> +++ b/include/linux/padata.h
>>> @@ -180,10 +180,6 @@ struct padata_instance {
>>>   #ifdef CONFIG_PADATA
>>>   extern void __init padata_init(void);
>>> -#else
>>> -static inline void __init padata_init(void) {}
>>> -#endif
>>> -
>>>   extern struct padata_instance *padata_alloc(const char *name);
>>>   extern void padata_free(struct padata_instance *pinst);
>>>   extern struct padata_shell *padata_alloc_shell(struct padata_instance *pinst);
>>> @@ -194,4 +190,13 @@ extern void padata_do_serial(struct padata_priv *padata);
>>>   extern void __init padata_do_multithreaded(struct padata_mt_job *job);
>>>   extern int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
>>>                     cpumask_var_t cpumask);
>>> +#else
>>> +static inline void __init padata_init(void) {}
>>> +static inline void __init padata_do_multithreaded(struct padata_mt_job *job)
>>> +{
>>> +    if (job->size)
>> I think we could drop this check, at least now there is no users will
>> pass a zero of ->size to this function, and even if someone does in the
>> future, I think it is really a corner case, it is unnecessary to optimize
>> it and ->thread_fn is supporsed to handle case of zero size if it dose
>> pass a zero size.
>> Thanks.
>>> +        job->thread_fn(job->start, job->start + job->size, job->fn_arg);
>>> +}
>>> +#endif
>>> +
>>>   #endif
diff mbox series

Patch

diff --git a/fs/Kconfig b/fs/Kconfig
index 4a51331f172e5..7963939592d70 100644
--- a/fs/Kconfig
+++ b/fs/Kconfig
@@ -261,7 +261,7 @@  menuconfig HUGETLBFS
 	depends on X86 || SPARC64 || ARCH_SUPPORTS_HUGETLBFS || BROKEN
 	depends on (SYSFS || SYSCTL)
 	select MEMFD_CREATE
-	select PADATA
+	select PADATA if SMP
 	help
 	  hugetlbfs is a filesystem backing for HugeTLB pages, based on
 	  ramfs. For architectures that support it, say Y here and read
diff --git a/include/linux/padata.h b/include/linux/padata.h
index 8f418711351bc..7b84eb7d73e7f 100644
--- a/include/linux/padata.h
+++ b/include/linux/padata.h
@@ -180,10 +180,6 @@  struct padata_instance {
 
 #ifdef CONFIG_PADATA
 extern void __init padata_init(void);
-#else
-static inline void __init padata_init(void) {}
-#endif
-
 extern struct padata_instance *padata_alloc(const char *name);
 extern void padata_free(struct padata_instance *pinst);
 extern struct padata_shell *padata_alloc_shell(struct padata_instance *pinst);
@@ -194,4 +190,13 @@  extern void padata_do_serial(struct padata_priv *padata);
 extern void __init padata_do_multithreaded(struct padata_mt_job *job);
 extern int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
 			      cpumask_var_t cpumask);
+#else
+static inline void __init padata_init(void) {}
+static inline void __init padata_do_multithreaded(struct padata_mt_job *job)
+{
+	if (job->size)
+		job->thread_fn(job->start, job->start + job->size, job->fn_arg);
+}
+#endif
+
 #endif