linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86: xen: remove the use of VLAIS
@ 2017-12-24 18:02 Nick Desaulniers
  2018-01-02  8:18 ` Juergen Gross
  0 siblings, 1 reply; 12+ messages in thread
From: Nick Desaulniers @ 2017-12-24 18:02 UTC (permalink / raw)
  Cc: ghackmann, mka, kees, srhines, Nick Desaulniers, Boris Ostrovsky,
	Juergen Gross, Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86,
	xen-devel, linux-kernel

Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
frowned upon by others.

https://lkml.org/lkml/2013/9/23/500

Here, the VLAIS was used because the size of the bitmap returned from
xen_mc_entry() depended on possibly (based on kernel configuration)
runtime sized data. Rather than declaring args as a VLAIS then calling
sizeof on *args, we can define the variable length array (mask) to be a
pointer, and calculate the appropriate sizeof args manually. Further, we
can get rid of the #ifdef's and rely on num_possible_cpus() (thanks to a
helpful checkpatch warning from an earlier version of this patch).

Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
 arch/x86/xen/mmu_pv.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 4d62c07..966976c 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
 {
 	struct {
 		struct mmuext_op op;
-#ifdef CONFIG_SMP
-		DECLARE_BITMAP(mask, num_processors);
-#else
-		DECLARE_BITMAP(mask, NR_CPUS);
-#endif
+		unsigned long *mask;
 	} *args;
 	struct multicall_space mcs;
+	const size_t mc_entry_size = sizeof(args->op) +
+		sizeof(*args->mask) * BITS_TO_LONGS(num_possible_cpus());
 
 	trace_xen_mmu_flush_tlb_others(cpus, info->mm, info->start, info->end);
 
 	if (cpumask_empty(cpus))
 		return;		/* nothing to do */
 
-	mcs = xen_mc_entry(sizeof(*args));
+	mcs = xen_mc_entry(mc_entry_size);
 	args = mcs.args;
 	args->op.arg2.vcpumask = to_cpumask(args->mask);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH] x86: xen: remove the use of VLAIS
  2017-12-24 18:02 [PATCH] x86: xen: remove the use of VLAIS Nick Desaulniers
@ 2018-01-02  8:18 ` Juergen Gross
  2018-01-06 21:39   ` [PATCH v2] " Nick Desaulniers
  0 siblings, 1 reply; 12+ messages in thread
From: Juergen Gross @ 2018-01-02  8:18 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: ghackmann, mka, kees, srhines, Boris Ostrovsky, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, x86, xen-devel, linux-kernel

On 24/12/17 19:02, Nick Desaulniers wrote:
> Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
> frowned upon by others.
> 
> https://lkml.org/lkml/2013/9/23/500
> 
> Here, the VLAIS was used because the size of the bitmap returned from
> xen_mc_entry() depended on possibly (based on kernel configuration)
> runtime sized data. Rather than declaring args as a VLAIS then calling
> sizeof on *args, we can define the variable length array (mask) to be a
> pointer, and calculate the appropriate sizeof args manually. Further, we
> can get rid of the #ifdef's and rely on num_possible_cpus() (thanks to a
> helpful checkpatch warning from an earlier version of this patch).

Using a pointer for mask seems to be wrong, as it is never initialized.

Why don't you just use:

DECLARE_BITMAP(mask, NR_CPUS);

and drop the #ifdef, while keeping the manual length calculation?


Juergen

> 
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> ---
>  arch/x86/xen/mmu_pv.c | 10 ++++------
>  1 file changed, 4 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index 4d62c07..966976c 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
>  {
>  	struct {
>  		struct mmuext_op op;
> -#ifdef CONFIG_SMP
> -		DECLARE_BITMAP(mask, num_processors);
> -#else
> -		DECLARE_BITMAP(mask, NR_CPUS);
> -#endif
> +		unsigned long *mask;
>  	} *args;
>  	struct multicall_space mcs;
> +	const size_t mc_entry_size = sizeof(args->op) +
> +		sizeof(*args->mask) * BITS_TO_LONGS(num_possible_cpus());
>  
>  	trace_xen_mmu_flush_tlb_others(cpus, info->mm, info->start, info->end);
>  
>  	if (cpumask_empty(cpus))
>  		return;		/* nothing to do */
>  
> -	mcs = xen_mc_entry(sizeof(*args));
> +	mcs = xen_mc_entry(mc_entry_size);
>  	args = mcs.args;
>  	args->op.arg2.vcpumask = to_cpumask(args->mask);
>  
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v2] x86: xen: remove the use of VLAIS
  2018-01-02  8:18 ` Juergen Gross
@ 2018-01-06 21:39   ` Nick Desaulniers
  2018-01-08  6:54     ` Juergen Gross
                       ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Nick Desaulniers @ 2018-01-06 21:39 UTC (permalink / raw)
  To: Juergen Gross
  Cc: ghackmann, mka, kees, srhines, Nick Desaulniers, Boris Ostrovsky,
	Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, xen-devel,
	linux-kernel

Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
frowned upon by others.

https://lkml.org/lkml/2013/9/23/500

Here, the VLAIS was used because the size of the bitmap returned from
xen_mc_entry() depended on possibly (based on kernel configuration)
runtime sized data. Rather than declaring args as a VLAIS then calling
sizeof on *args, we calculate the appropriate sizeof args manually.
Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
(thanks to a helpful checkpatch warning from an earlier version of this
patch).

Suggested-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
---
Changes in v2:
* Change mask to us DECLARE_BITMAP instead of pointer, as suggested.
* Update commit message to remove mention of pointer.
* Update sizeof calculation to work with array rather than pointer.

 arch/x86/xen/mmu_pv.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
index 4d62c07..d850762 100644
--- a/arch/x86/xen/mmu_pv.c
+++ b/arch/x86/xen/mmu_pv.c
@@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
 {
 	struct {
 		struct mmuext_op op;
-#ifdef CONFIG_SMP
-		DECLARE_BITMAP(mask, num_processors);
-#else
 		DECLARE_BITMAP(mask, NR_CPUS);
-#endif
 	} *args;
 	struct multicall_space mcs;
+	const size_t mc_entry_size = sizeof(args->op) +
+		sizeof(args->mask[0]) * BITS_TO_LONGS(num_possible_cpus());
 
 	trace_xen_mmu_flush_tlb_others(cpus, info->mm, info->start, info->end);
 
 	if (cpumask_empty(cpus))
 		return;		/* nothing to do */
 
-	mcs = xen_mc_entry(sizeof(*args));
+	mcs = xen_mc_entry(mc_entry_size);
 	args = mcs.args;
 	args->op.arg2.vcpumask = to_cpumask(args->mask);
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86: xen: remove the use of VLAIS
  2018-01-06 21:39   ` [PATCH v2] " Nick Desaulniers
@ 2018-01-08  6:54     ` Juergen Gross
  2018-01-08 15:54     ` Boris Ostrovsky
  2018-01-08 16:10     ` Peter Zijlstra
  2 siblings, 0 replies; 12+ messages in thread
From: Juergen Gross @ 2018-01-08  6:54 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: ghackmann, mka, kees, srhines, Boris Ostrovsky, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, x86, xen-devel, linux-kernel

On 06/01/18 22:39, Nick Desaulniers wrote:
> Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
> frowned upon by others.
> 
> https://lkml.org/lkml/2013/9/23/500
> 
> Here, the VLAIS was used because the size of the bitmap returned from
> xen_mc_entry() depended on possibly (based on kernel configuration)
> runtime sized data. Rather than declaring args as a VLAIS then calling
> sizeof on *args, we calculate the appropriate sizeof args manually.
> Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
> (thanks to a helpful checkpatch warning from an earlier version of this
> patch).
> 
> Suggested-by: Juergen Gross <jgross@suse.com>
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>

Reviewed-by: Juergen Gross <jgross@suse.com>


Juergen

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86: xen: remove the use of VLAIS
  2018-01-06 21:39   ` [PATCH v2] " Nick Desaulniers
  2018-01-08  6:54     ` Juergen Gross
@ 2018-01-08 15:54     ` Boris Ostrovsky
  2018-01-08 16:10     ` Peter Zijlstra
  2 siblings, 0 replies; 12+ messages in thread
From: Boris Ostrovsky @ 2018-01-08 15:54 UTC (permalink / raw)
  To: Nick Desaulniers, Juergen Gross
  Cc: ghackmann, mka, kees, srhines, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, x86, xen-devel, linux-kernel

On 01/06/2018 04:39 PM, Nick Desaulniers wrote:
> Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
> frowned upon by others.
>
> https://lkml.org/lkml/2013/9/23/500
>
> Here, the VLAIS was used because the size of the bitmap returned from
> xen_mc_entry() depended on possibly (based on kernel configuration)
> runtime sized data. Rather than declaring args as a VLAIS then calling
> sizeof on *args, we calculate the appropriate sizeof args manually.
> Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
> (thanks to a helpful checkpatch warning from an earlier version of this
> patch).
>
> Suggested-by: Juergen Gross <jgross@suse.com>
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>

Applied to for-linus-4.15.

-boris

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86: xen: remove the use of VLAIS
  2018-01-06 21:39   ` [PATCH v2] " Nick Desaulniers
  2018-01-08  6:54     ` Juergen Gross
  2018-01-08 15:54     ` Boris Ostrovsky
@ 2018-01-08 16:10     ` Peter Zijlstra
  2018-01-08 16:20       ` Boris Ostrovsky
  2018-01-08 16:26       ` Juergen Gross
  2 siblings, 2 replies; 12+ messages in thread
From: Peter Zijlstra @ 2018-01-08 16:10 UTC (permalink / raw)
  To: Nick Desaulniers
  Cc: Juergen Gross, ghackmann, mka, kees, srhines, Boris Ostrovsky,
	Thomas Gleixner, Ingo Molnar, H. Peter Anvin, x86, xen-devel,
	linux-kernel

On Sat, Jan 06, 2018 at 01:39:48PM -0800, Nick Desaulniers wrote:
> Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
> frowned upon by others.
> 
> https://lkml.org/lkml/2013/9/23/500
> 
> Here, the VLAIS was used because the size of the bitmap returned from
> xen_mc_entry() depended on possibly (based on kernel configuration)
> runtime sized data. Rather than declaring args as a VLAIS then calling
> sizeof on *args, we calculate the appropriate sizeof args manually.
> Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
> (thanks to a helpful checkpatch warning from an earlier version of this
> patch).
> 
> Suggested-by: Juergen Gross <jgross@suse.com>
> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> ---
> Changes in v2:
> * Change mask to us DECLARE_BITMAP instead of pointer, as suggested.
> * Update commit message to remove mention of pointer.
> * Update sizeof calculation to work with array rather than pointer.
> 
>  arch/x86/xen/mmu_pv.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> index 4d62c07..d850762 100644
> --- a/arch/x86/xen/mmu_pv.c
> +++ b/arch/x86/xen/mmu_pv.c
> @@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
>  {
>  	struct {
>  		struct mmuext_op op;
> -#ifdef CONFIG_SMP
> -		DECLARE_BITMAP(mask, num_processors);
> -#else
>  		DECLARE_BITMAP(mask, NR_CPUS);
> -#endif
>  	} *args;

Why is it OK for Xen to place this bitmap on-stack in the first place?
That NR_CPUS thing can be fairly huge.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86: xen: remove the use of VLAIS
  2018-01-08 16:10     ` Peter Zijlstra
@ 2018-01-08 16:20       ` Boris Ostrovsky
  2018-01-08 16:28         ` Juergen Gross
  2018-01-08 16:26       ` Juergen Gross
  1 sibling, 1 reply; 12+ messages in thread
From: Boris Ostrovsky @ 2018-01-08 16:20 UTC (permalink / raw)
  To: Peter Zijlstra, Nick Desaulniers
  Cc: Juergen Gross, ghackmann, mka, kees, srhines, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, x86, xen-devel, linux-kernel

On 01/08/2018 11:10 AM, Peter Zijlstra wrote:
> On Sat, Jan 06, 2018 at 01:39:48PM -0800, Nick Desaulniers wrote:
>> Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
>> frowned upon by others.
>>
>> https://lkml.org/lkml/2013/9/23/500
>>
>> Here, the VLAIS was used because the size of the bitmap returned from
>> xen_mc_entry() depended on possibly (based on kernel configuration)
>> runtime sized data. Rather than declaring args as a VLAIS then calling
>> sizeof on *args, we calculate the appropriate sizeof args manually.
>> Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
>> (thanks to a helpful checkpatch warning from an earlier version of this
>> patch).
>>
>> Suggested-by: Juergen Gross <jgross@suse.com>
>> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
>> ---
>> Changes in v2:
>> * Change mask to us DECLARE_BITMAP instead of pointer, as suggested.
>> * Update commit message to remove mention of pointer.
>> * Update sizeof calculation to work with array rather than pointer.
>>
>>  arch/x86/xen/mmu_pv.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>> index 4d62c07..d850762 100644
>> --- a/arch/x86/xen/mmu_pv.c
>> +++ b/arch/x86/xen/mmu_pv.c
>> @@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
>>  {
>>  	struct {
>>  		struct mmuext_op op;
>> -#ifdef CONFIG_SMP
>> -		DECLARE_BITMAP(mask, num_processors);
>> -#else
>>  		DECLARE_BITMAP(mask, NR_CPUS);
>> -#endif
>>  	} *args;
> Why is it OK for Xen to place this bitmap on-stack in the first place?
> That NR_CPUS thing can be fairly huge.

Err... right. Now it's even worse than it was before, when the array was
potentially smaller. I'll drop this.

-boris

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86: xen: remove the use of VLAIS
  2018-01-08 16:10     ` Peter Zijlstra
  2018-01-08 16:20       ` Boris Ostrovsky
@ 2018-01-08 16:26       ` Juergen Gross
  2018-01-08 18:49         ` Ingo Molnar
  1 sibling, 1 reply; 12+ messages in thread
From: Juergen Gross @ 2018-01-08 16:26 UTC (permalink / raw)
  To: Peter Zijlstra, Nick Desaulniers
  Cc: ghackmann, mka, kees, srhines, Boris Ostrovsky, Thomas Gleixner,
	Ingo Molnar, H. Peter Anvin, x86, xen-devel, linux-kernel

On 08/01/18 17:10, Peter Zijlstra wrote:
> On Sat, Jan 06, 2018 at 01:39:48PM -0800, Nick Desaulniers wrote:
>> Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
>> frowned upon by others.
>>
>> https://lkml.org/lkml/2013/9/23/500
>>
>> Here, the VLAIS was used because the size of the bitmap returned from
>> xen_mc_entry() depended on possibly (based on kernel configuration)
>> runtime sized data. Rather than declaring args as a VLAIS then calling
>> sizeof on *args, we calculate the appropriate sizeof args manually.
>> Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
>> (thanks to a helpful checkpatch warning from an earlier version of this
>> patch).
>>
>> Suggested-by: Juergen Gross <jgross@suse.com>
>> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
>> ---
>> Changes in v2:
>> * Change mask to us DECLARE_BITMAP instead of pointer, as suggested.
>> * Update commit message to remove mention of pointer.
>> * Update sizeof calculation to work with array rather than pointer.
>>
>>  arch/x86/xen/mmu_pv.c | 8 +++-----
>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>> index 4d62c07..d850762 100644
>> --- a/arch/x86/xen/mmu_pv.c
>> +++ b/arch/x86/xen/mmu_pv.c
>> @@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
>>  {
>>  	struct {
>>  		struct mmuext_op op;
>> -#ifdef CONFIG_SMP
>> -		DECLARE_BITMAP(mask, num_processors);
>> -#else
>>  		DECLARE_BITMAP(mask, NR_CPUS);
>> -#endif
>>  	} *args;
> 
> Why is it OK for Xen to place this bitmap on-stack in the first place?
> That NR_CPUS thing can be fairly huge.

This only a pointer to the bitmap.


Juergen

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86: xen: remove the use of VLAIS
  2018-01-08 16:20       ` Boris Ostrovsky
@ 2018-01-08 16:28         ` Juergen Gross
  2018-01-08 16:35           ` [Xen-devel] " Boris Ostrovsky
  0 siblings, 1 reply; 12+ messages in thread
From: Juergen Gross @ 2018-01-08 16:28 UTC (permalink / raw)
  To: Boris Ostrovsky, Peter Zijlstra, Nick Desaulniers
  Cc: ghackmann, mka, kees, srhines, Thomas Gleixner, Ingo Molnar,
	H. Peter Anvin, x86, xen-devel, linux-kernel

On 08/01/18 17:20, Boris Ostrovsky wrote:
> On 01/08/2018 11:10 AM, Peter Zijlstra wrote:
>> On Sat, Jan 06, 2018 at 01:39:48PM -0800, Nick Desaulniers wrote:
>>> Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
>>> frowned upon by others.
>>>
>>> https://lkml.org/lkml/2013/9/23/500
>>>
>>> Here, the VLAIS was used because the size of the bitmap returned from
>>> xen_mc_entry() depended on possibly (based on kernel configuration)
>>> runtime sized data. Rather than declaring args as a VLAIS then calling
>>> sizeof on *args, we calculate the appropriate sizeof args manually.
>>> Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
>>> (thanks to a helpful checkpatch warning from an earlier version of this
>>> patch).
>>>
>>> Suggested-by: Juergen Gross <jgross@suse.com>
>>> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
>>> ---
>>> Changes in v2:
>>> * Change mask to us DECLARE_BITMAP instead of pointer, as suggested.
>>> * Update commit message to remove mention of pointer.
>>> * Update sizeof calculation to work with array rather than pointer.
>>>
>>>  arch/x86/xen/mmu_pv.c | 8 +++-----
>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>>> index 4d62c07..d850762 100644
>>> --- a/arch/x86/xen/mmu_pv.c
>>> +++ b/arch/x86/xen/mmu_pv.c
>>> @@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
>>>  {
>>>  	struct {
>>>  		struct mmuext_op op;
>>> -#ifdef CONFIG_SMP
>>> -		DECLARE_BITMAP(mask, num_processors);
>>> -#else
>>>  		DECLARE_BITMAP(mask, NR_CPUS);
>>> -#endif
>>>  	} *args;
>> Why is it OK for Xen to place this bitmap on-stack in the first place?
>> That NR_CPUS thing can be fairly huge.
> 
> Err... right. Now it's even worse than it was before, when the array was
> potentially smaller. I'll drop this.

No, its only the pointer to the struct, not the struct itself.


Juergen

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [Xen-devel] [PATCH v2] x86: xen: remove the use of VLAIS
  2018-01-08 16:28         ` Juergen Gross
@ 2018-01-08 16:35           ` Boris Ostrovsky
  0 siblings, 0 replies; 12+ messages in thread
From: Boris Ostrovsky @ 2018-01-08 16:35 UTC (permalink / raw)
  To: Juergen Gross, Peter Zijlstra, Nick Desaulniers
  Cc: x86, H. Peter Anvin, linux-kernel, ghackmann, mka, Ingo Molnar,
	srhines, kees, xen-devel, Thomas Gleixner

On 01/08/2018 11:28 AM, Juergen Gross wrote:
> On 08/01/18 17:20, Boris Ostrovsky wrote:
>> On 01/08/2018 11:10 AM, Peter Zijlstra wrote:
>>> On Sat, Jan 06, 2018 at 01:39:48PM -0800, Nick Desaulniers wrote:
>>>> Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
>>>> frowned upon by others.
>>>>
>>>> https://lkml.org/lkml/2013/9/23/500
>>>>
>>>> Here, the VLAIS was used because the size of the bitmap returned from
>>>> xen_mc_entry() depended on possibly (based on kernel configuration)
>>>> runtime sized data. Rather than declaring args as a VLAIS then calling
>>>> sizeof on *args, we calculate the appropriate sizeof args manually.
>>>> Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
>>>> (thanks to a helpful checkpatch warning from an earlier version of this
>>>> patch).
>>>>
>>>> Suggested-by: Juergen Gross <jgross@suse.com>
>>>> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>> * Change mask to us DECLARE_BITMAP instead of pointer, as suggested.
>>>> * Update commit message to remove mention of pointer.
>>>> * Update sizeof calculation to work with array rather than pointer.
>>>>
>>>>  arch/x86/xen/mmu_pv.c | 8 +++-----
>>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>>>> index 4d62c07..d850762 100644
>>>> --- a/arch/x86/xen/mmu_pv.c
>>>> +++ b/arch/x86/xen/mmu_pv.c
>>>> @@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
>>>>  {
>>>>  	struct {
>>>>  		struct mmuext_op op;
>>>> -#ifdef CONFIG_SMP
>>>> -		DECLARE_BITMAP(mask, num_processors);
>>>> -#else
>>>>  		DECLARE_BITMAP(mask, NR_CPUS);
>>>> -#endif
>>>>  	} *args;
>>> Why is it OK for Xen to place this bitmap on-stack in the first place?
>>> That NR_CPUS thing can be fairly huge.
>> Err... right. Now it's even worse than it was before, when the array was
>> potentially smaller. I'll drop this.
> No, its only the pointer to the struct, not the struct itself.
>


It's the full array, isn't it?

#define DECLARE_BITMAP(name,bits) \
        unsigned long name[BITS_TO_LONGS(bits)]

<pause>

OK, it *is* a pointer. Sigh...

-boris

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86: xen: remove the use of VLAIS
  2018-01-08 16:26       ` Juergen Gross
@ 2018-01-08 18:49         ` Ingo Molnar
  2018-01-09  5:35           ` Juergen Gross
  0 siblings, 1 reply; 12+ messages in thread
From: Ingo Molnar @ 2018-01-08 18:49 UTC (permalink / raw)
  To: Juergen Gross
  Cc: Peter Zijlstra, Nick Desaulniers, ghackmann, mka, kees, srhines,
	Boris Ostrovsky, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, xen-devel, linux-kernel


* Juergen Gross <jgross@suse.com> wrote:

> On 08/01/18 17:10, Peter Zijlstra wrote:
> > On Sat, Jan 06, 2018 at 01:39:48PM -0800, Nick Desaulniers wrote:
> >> Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
> >> frowned upon by others.
> >>
> >> https://lkml.org/lkml/2013/9/23/500
> >>
> >> Here, the VLAIS was used because the size of the bitmap returned from
> >> xen_mc_entry() depended on possibly (based on kernel configuration)
> >> runtime sized data. Rather than declaring args as a VLAIS then calling
> >> sizeof on *args, we calculate the appropriate sizeof args manually.
> >> Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
> >> (thanks to a helpful checkpatch warning from an earlier version of this
> >> patch).
> >>
> >> Suggested-by: Juergen Gross <jgross@suse.com>
> >> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
> >> ---
> >> Changes in v2:
> >> * Change mask to us DECLARE_BITMAP instead of pointer, as suggested.
> >> * Update commit message to remove mention of pointer.
> >> * Update sizeof calculation to work with array rather than pointer.
> >>
> >>  arch/x86/xen/mmu_pv.c | 8 +++-----
> >>  1 file changed, 3 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
> >> index 4d62c07..d850762 100644
> >> --- a/arch/x86/xen/mmu_pv.c
> >> +++ b/arch/x86/xen/mmu_pv.c
> >> @@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
> >>  {
> >>  	struct {
> >>  		struct mmuext_op op;
> >> -#ifdef CONFIG_SMP
> >> -		DECLARE_BITMAP(mask, num_processors);
> >> -#else
> >>  		DECLARE_BITMAP(mask, NR_CPUS);
> >> -#endif
> >>  	} *args;
> > 
> > Why is it OK for Xen to place this bitmap on-stack in the first place?
> > That NR_CPUS thing can be fairly huge.
> 
> This only a pointer to the bitmap.

What's the maximum NR_CPUs for configs that can run this code, times 8?

Thanks,

	Ingo

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v2] x86: xen: remove the use of VLAIS
  2018-01-08 18:49         ` Ingo Molnar
@ 2018-01-09  5:35           ` Juergen Gross
  0 siblings, 0 replies; 12+ messages in thread
From: Juergen Gross @ 2018-01-09  5:35 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: Peter Zijlstra, Nick Desaulniers, ghackmann, mka, kees, srhines,
	Boris Ostrovsky, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
	x86, xen-devel, linux-kernel

On 08/01/18 19:49, Ingo Molnar wrote:
> 
> * Juergen Gross <jgross@suse.com> wrote:
> 
>> On 08/01/18 17:10, Peter Zijlstra wrote:
>>> On Sat, Jan 06, 2018 at 01:39:48PM -0800, Nick Desaulniers wrote:
>>>> Variable Length Arrays In Structs (VLAIS) is not supported by Clang, and
>>>> frowned upon by others.
>>>>
>>>> https://lkml.org/lkml/2013/9/23/500
>>>>
>>>> Here, the VLAIS was used because the size of the bitmap returned from
>>>> xen_mc_entry() depended on possibly (based on kernel configuration)
>>>> runtime sized data. Rather than declaring args as a VLAIS then calling
>>>> sizeof on *args, we calculate the appropriate sizeof args manually.
>>>> Further, we can get rid of the #ifdef's and rely on num_possible_cpus()
>>>> (thanks to a helpful checkpatch warning from an earlier version of this
>>>> patch).
>>>>
>>>> Suggested-by: Juergen Gross <jgross@suse.com>
>>>> Signed-off-by: Nick Desaulniers <nick.desaulniers@gmail.com>
>>>> ---
>>>> Changes in v2:
>>>> * Change mask to us DECLARE_BITMAP instead of pointer, as suggested.
>>>> * Update commit message to remove mention of pointer.
>>>> * Update sizeof calculation to work with array rather than pointer.
>>>>
>>>>  arch/x86/xen/mmu_pv.c | 8 +++-----
>>>>  1 file changed, 3 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/arch/x86/xen/mmu_pv.c b/arch/x86/xen/mmu_pv.c
>>>> index 4d62c07..d850762 100644
>>>> --- a/arch/x86/xen/mmu_pv.c
>>>> +++ b/arch/x86/xen/mmu_pv.c
>>>> @@ -1325,20 +1325,18 @@ static void xen_flush_tlb_others(const struct cpumask *cpus,
>>>>  {
>>>>  	struct {
>>>>  		struct mmuext_op op;
>>>> -#ifdef CONFIG_SMP
>>>> -		DECLARE_BITMAP(mask, num_processors);
>>>> -#else
>>>>  		DECLARE_BITMAP(mask, NR_CPUS);
>>>> -#endif
>>>>  	} *args;
>>>
>>> Why is it OK for Xen to place this bitmap on-stack in the first place?
>>> That NR_CPUS thing can be fairly huge.
>>
>> This only a pointer to the bitmap.
> 
> What's the maximum NR_CPUs for configs that can run this code, times 8?

Why does this matter? args is a pointer only, so it occupies 8 bytes of
the stack. The structure is only for type correctness.


Juergen

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2018-01-09  5:35 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-24 18:02 [PATCH] x86: xen: remove the use of VLAIS Nick Desaulniers
2018-01-02  8:18 ` Juergen Gross
2018-01-06 21:39   ` [PATCH v2] " Nick Desaulniers
2018-01-08  6:54     ` Juergen Gross
2018-01-08 15:54     ` Boris Ostrovsky
2018-01-08 16:10     ` Peter Zijlstra
2018-01-08 16:20       ` Boris Ostrovsky
2018-01-08 16:28         ` Juergen Gross
2018-01-08 16:35           ` [Xen-devel] " Boris Ostrovsky
2018-01-08 16:26       ` Juergen Gross
2018-01-08 18:49         ` Ingo Molnar
2018-01-09  5:35           ` Juergen Gross

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).