diff mbox

[Xen-devel] xen/arm: minor improvement in smp_send_call_function_mask()

Message ID 1408423704-15059-1-git-send-email-anup.patel@linaro.org
State New
Headers show

Commit Message

Anup Patel Aug. 19, 2014, 4:48 a.m. UTC 
Currently, smp_send_call_function_mask() function implemented
by xen arm/arm64 will use IPI to call function on current CPU.

This means that current smp_send_call_function_mask() will do
the following on current CPU:
Trigger SGI for current CPU => Xen takes interrupt on current CPU
=> IPI interrupt handler will call smp_call_function_interrupt()

This patch improves the above by straight away calling
smp_call_function_interrupt() for current CPU. This is very
similar to smp_send_call_function_mask() implemented by Xen x86.

Signed-off-by: Anup Patel <anup.patel@linaro.org>
Signed-off-by: Pranavkumar Sawargaonkar <pranavkumar@linaro.org>
---
 xen/arch/arm/smp.c |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

Comments

Julien Grall Aug. 19, 2014, 9:06 p.m. UTC | #1 
Hi Anup,

On 18/08/14 23:48, Anup Patel wrote:
>   xen/arch/arm/smp.c |   14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
> index 30203b8..c80c068 100644
> --- a/xen/arch/arm/smp.c
> +++ b/xen/arch/arm/smp.c
> @@ -19,7 +19,19 @@ void smp_send_event_check_mask(const cpumask_t *mask)
>
>   void smp_send_call_function_mask(const cpumask_t *mask)
>   {
> -    send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION);
> +    cpumask_t target_mask;
> +
> +    cpumask_andnot(&target_mask, mask, cpumask_of(smp_processor_id()));
> +
> +    if ( cpumask_weight(&target_mask) )

Is it necessary? What happen if Xen tries to send an SGI with an empty mask?

AFAIU, the function cpumask_weight is complex so if we can avoid it, it 
would be better.
Anup Patel Aug. 20, 2014, 6:14 a.m. UTC | #2 
On 20 August 2014 02:36, Julien Grall <julien.grall@linaro.org> wrote:
> Hi Anup,
>
>
> On 18/08/14 23:48, Anup Patel wrote:
>>
>>   xen/arch/arm/smp.c |   14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
>> index 30203b8..c80c068 100644
>> --- a/xen/arch/arm/smp.c
>> +++ b/xen/arch/arm/smp.c
>> @@ -19,7 +19,19 @@ void smp_send_event_check_mask(const cpumask_t *mask)
>>
>>   void smp_send_call_function_mask(const cpumask_t *mask)
>>   {
>> -    send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION);
>> +    cpumask_t target_mask;
>> +
>> +    cpumask_andnot(&target_mask, mask, cpumask_of(smp_processor_id()));
>> +
>> +    if ( cpumask_weight(&target_mask) )
>
>
> Is it necessary? What happen if Xen tries to send an SGI with an empty mask?

If Xen tries to send SGI with empty mask then target_mask will be empty
hence cpumask_weight(&target_mask) will return 0

>
> AFAIU, the function cpumask_weight is complex so if we can avoid it, it
> would be better.

Can you explain more about how cpumask_weight is complex ??

Other alternative is to use "cpumask_first(&target_mask) != NR_CPUS".

>
> --
> Julien Grall

--
Anup
Julien Grall Aug. 20, 2014, 3:07 p.m. UTC | #3 
On 20/08/14 01:14, Anup Patel wrote:
>> Is it necessary? What happen if Xen tries to send an SGI with an empty mask?
>
> If Xen tries to send SGI with empty mask then target_mask will be empty
> hence cpumask_weight(&target_mask) will return 0

The GIC documentation says:
"If this field is 0x00 when TargetListFilter is 0b00 , the Distributor 
does not forward the interrupt to any
CPU interface."

TargetListFilter == 0b00 => Forward interrupt to a specific list of CPUs.

>>
>> AFAIU, the function cpumask_weight is complex so if we can avoid it, it
>> would be better.
>
> Can you explain more about how cpumask_weight is complex ??

cpumask_weight contains a loop and multiple addition. I doubt the case 
where the mask only contains the current cpu happens often.

As the GIC won't forward the interrupt if the list of CPUs is empty, I 
don't think it's worth to add this check.

> Other alternative is to use "cpumask_first(&target_mask) != NR_CPUS".

The best alternative would be cpumask_empty.

Regards,
Anup Patel Aug. 21, 2014, 11:04 a.m. UTC | #4 
On 20 August 2014 20:37, Julien Grall <julien.grall@linaro.org> wrote:
>
>
> On 20/08/14 01:14, Anup Patel wrote:
>>>
>>> Is it necessary? What happen if Xen tries to send an SGI with an empty
>>> mask?
>>
>>
>> If Xen tries to send SGI with empty mask then target_mask will be empty
>> hence cpumask_weight(&target_mask) will return 0
>
>
> The GIC documentation says:
> "If this field is 0x00 when TargetListFilter is 0b00 , the Distributor does
> not forward the interrupt to any
> CPU interface."
>
> TargetListFilter == 0b00 => Forward interrupt to a specific list of CPUs.
>
>
>>>
>>> AFAIU, the function cpumask_weight is complex so if we can avoid it, it
>>> would be better.
>>
>>
>> Can you explain more about how cpumask_weight is complex ??
>
>
> cpumask_weight contains a loop and multiple addition. I doubt the case where
> the mask only contains the current cpu happens often.
>
> As the GIC won't forward the interrupt if the list of CPUs is empty, I don't
> think it's worth to add this check.
>
>
>> Other alternative is to use "cpumask_first(&target_mask) != NR_CPUS".
>
>
> The best alternative would be cpumask_empty.

All three cpumask_empty(), cpumask_first(), and cpumask_weight()
are O(N) where N is number of bits in cpumask.

It really does not make much difference which of these operation
is chosen.

Since empty target list is fine with GIC Distributor, I will drop the check.

--
Anup

>
> Regards,
>
> --
> Julien Grall
Julien Grall Aug. 21, 2014, 4:54 p.m. UTC | #5 
Hi Anup,

On 21/08/14 06:04, Anup Patel wrote:
>> The best alternative would be cpumask_empty.
>
> All three cpumask_empty(), cpumask_first(), and cpumask_weight()
> are O(N) where N is number of bits in cpumask.
> It really does not make much difference which of these operation
> is chosen.

Hmmm right. That the worst case for all, and always the case for 
cpumask_weight. Anyway...

> Since empty target list is fine with GIC Distributor, I will drop the check.

Assuming you only remove the check in the next version:

Acked-by: Julien Grall <julien.grall@linaro.org>

Regards,
Andrew Cooper Aug. 21, 2014, 5:22 p.m. UTC | #6 
On 21/08/14 17:54, Julien Grall wrote:
> Hi Anup,
>
> On 21/08/14 06:04, Anup Patel wrote:
>>> The best alternative would be cpumask_empty.
>>
>> All three cpumask_empty(), cpumask_first(), and cpumask_weight()
>> are O(N) where N is number of bits in cpumask.
>> It really does not make much difference which of these operation
>> is chosen.

They are all O(N), but O() notation hides lesser factors.

cpumask_empty() is slightly cheaper than cpumask_first(), which are both
substantially cheaper than cpumask_weight().

There is no fastpath for calculating the hamming weight of 0, resulting
in a lot of dependent shift/mask operations.

~Andrew
diff mbox

Patch

diff --git a/xen/arch/arm/smp.c b/xen/arch/arm/smp.c
index 30203b8..c80c068 100644
--- a/xen/arch/arm/smp.c
+++ b/xen/arch/arm/smp.c
@@ -19,7 +19,19 @@  void smp_send_event_check_mask(const cpumask_t *mask)
 
 void smp_send_call_function_mask(const cpumask_t *mask)
 {
-    send_SGI_mask(mask, GIC_SGI_CALL_FUNCTION);
+    cpumask_t target_mask;
+
+    cpumask_andnot(&target_mask, mask, cpumask_of(smp_processor_id()));
+
+    if ( cpumask_weight(&target_mask) )
+        send_SGI_mask(&target_mask, GIC_SGI_CALL_FUNCTION);
+
+    if ( cpumask_test_cpu(smp_processor_id(), mask) )
+    {
+        local_irq_disable();
+        smp_call_function_interrupt();
+        local_irq_enable();
+    }
 }
 
 /*