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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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);
>
>
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 <[email protected]>
Signed-off-by: Nick Desaulniers <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
Reviewed-by: Juergen Gross <[email protected]>
Juergen
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 <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
Applied to for-linus-4.15.
-boris
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 <[email protected]>
> Signed-off-by: Nick Desaulniers <[email protected]>
> ---
> 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.
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 <[email protected]>
>> Signed-off-by: Nick Desaulniers <[email protected]>
>> ---
>> 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
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 <[email protected]>
>> Signed-off-by: Nick Desaulniers <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> Signed-off-by: Nick Desaulniers <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>>>> Signed-off-by: Nick Desaulniers <[email protected]>
>>>> ---
>>>> 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
* Juergen Gross <[email protected]> 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 <[email protected]>
> >> Signed-off-by: Nick Desaulniers <[email protected]>
> >> ---
> >> 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
On 08/01/18 19:49, Ingo Molnar wrote:
>
> * Juergen Gross <[email protected]> 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 <[email protected]>
>>>> Signed-off-by: Nick Desaulniers <[email protected]>
>>>> ---
>>>> 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