Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On Friday 08 March 2013, Thomas Gleixner wrote: > On Fri, 8 Mar 2013, Daniel Lezcano wrote: > > On 03/06/2013 10:48 AM, Thomas Gleixner wrote: > > I was wondering if it would be possible to take the 3/4 and 4/4 > > otherwise the flag dependency will prevent to send those to the > > maintainer's tree until they gain visibility on it. > > I can take them with the ack of arm soc folks. Sounds good, Acked-by: Arnd Bergmann -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On Friday 08 March 2013, Thomas Gleixner wrote: On Fri, 8 Mar 2013, Daniel Lezcano wrote: On 03/06/2013 10:48 AM, Thomas Gleixner wrote: I was wondering if it would be possible to take the 3/4 and 4/4 otherwise the flag dependency will prevent to send those to the maintainer's tree until they gain visibility on it. I can take them with the ack of arm soc folks. Sounds good, Acked-by: Arnd Bergmann a...@arndb.de -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On Fri, 8 Mar 2013, Daniel Lezcano wrote: > On 03/06/2013 10:48 AM, Thomas Gleixner wrote: > I was wondering if it would be possible to take the 3/4 and 4/4 > otherwise the flag dependency will prevent to send those to the > maintainer's tree until they gain visibility on it. I can take them with the ack of arm soc folks. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On 03/06/2013 10:48 AM, Thomas Gleixner wrote:
> On Wed, 6 Mar 2013, Daniel Lezcano wrote:
>> On 03/05/2013 09:40 PM, Thomas Gleixner wrote:
>>> On Sat, 2 Mar 2013, Daniel Lezcano wrote:
When a cpu goes to a deep idle state where its local timer is shutdown,
it notifies the time frame work to use the broadcast timer instead.
Unfortunately, the broadcast device could wake up any CPU, including an
idle one which is not concerned by the wake up at all.
This implies, in the worst case, an idle CPU will wake up to send an IPI
to another idle cpu.
This patch solves this by setting the irq affinity to the cpu concerned
by the nearest timer event, by this way, the CPU which is wake up is
guarantee to be the one concerned by the next event and we are safe with
unnecessary wakeup for another idle CPU.
As the irq affinity is not supported by all the archs, a flag is needed
to specify which clocksource can handle it : CLOCK_EVT_FEAT_DYNIRQ
Signed-off-by: Daniel Lezcano
---
include/linux/clockchips.h |5 +
kernel/time/tick-broadcast.c | 40
+---
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 6634652..c93e2a6 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -55,6 +55,11 @@ enum clock_event_nofitiers {
#define CLOCK_EVT_FEAT_C3STOP 0x08
#define CLOCK_EVT_FEAT_DUMMY 0x10
+/*
+ * Clock event device can set its irq affinity dynamically
+ */
+#define CLOCK_EVT_FEAT_DYNIRQ 0x20
+
/**
* struct clock_event_device - clock event device descriptor
* @event_handler:Assigned by the framework to be called by the
low
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 6197ac0..9ca8ff5 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -406,13 +406,37 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void)
return to_cpumask(tick_broadcast_oneshot_mask);
}
-static int tick_broadcast_set_event(struct clock_event_device *bc,
+/*
+ * Set broadcast interrupt affinity
+ */
+static void tick_broadcast_set_affinity(struct clock_event_device *bc,
+ const struct cpumask *cpumask)
+{
+ if (!(bc->features & CLOCK_EVT_FEAT_DYNIRQ))
+ return;
+
+ if (cpumask_equal(bc->cpumask, cpumask))
+ return;
+
+ bc->cpumask = cpumask;
>>>
>>> This breaks with CONFIG_CPUMASK_OFFSTACK=y. cpumask_copy() is your friend!
>>
>> This instruction copies the pointer, not the cpumask content.
>>
>> bc->cpumask is defined as a const struct cpumask * and is used to copy a
>> cpumask pointer not the content.
>>
>> The cpumask parameter is a pointer to a global cpumask provided by the
>> cpumask_of macro.
>>
>> But to be in the safe side, I compiled tested with
>> CONFIG_CPUMASK_OFFSTACK=y without problem.
Hi Thomas,
thanks for merging the patch 1 and 2.
I was wondering if it would be possible to take the 3/4 and 4/4
otherwise the flag dependency will prevent to send those to the
maintainer's tree until they gain visibility on it.
--
Facebook |
Twitter |
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On 03/06/2013 10:48 AM, Thomas Gleixner wrote:
On Wed, 6 Mar 2013, Daniel Lezcano wrote:
On 03/05/2013 09:40 PM, Thomas Gleixner wrote:
On Sat, 2 Mar 2013, Daniel Lezcano wrote:
When a cpu goes to a deep idle state where its local timer is shutdown,
it notifies the time frame work to use the broadcast timer instead.
Unfortunately, the broadcast device could wake up any CPU, including an
idle one which is not concerned by the wake up at all.
This implies, in the worst case, an idle CPU will wake up to send an IPI
to another idle cpu.
This patch solves this by setting the irq affinity to the cpu concerned
by the nearest timer event, by this way, the CPU which is wake up is
guarantee to be the one concerned by the next event and we are safe with
unnecessary wakeup for another idle CPU.
As the irq affinity is not supported by all the archs, a flag is needed
to specify which clocksource can handle it : CLOCK_EVT_FEAT_DYNIRQ
Signed-off-by: Daniel Lezcano daniel.lezc...@linaro.org
---
include/linux/clockchips.h |5 +
kernel/time/tick-broadcast.c | 40
+---
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 6634652..c93e2a6 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -55,6 +55,11 @@ enum clock_event_nofitiers {
#define CLOCK_EVT_FEAT_C3STOP 0x08
#define CLOCK_EVT_FEAT_DUMMY 0x10
+/*
+ * Clock event device can set its irq affinity dynamically
+ */
+#define CLOCK_EVT_FEAT_DYNIRQ 0x20
+
/**
* struct clock_event_device - clock event device descriptor
* @event_handler:Assigned by the framework to be called by the
low
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 6197ac0..9ca8ff5 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -406,13 +406,37 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void)
return to_cpumask(tick_broadcast_oneshot_mask);
}
-static int tick_broadcast_set_event(struct clock_event_device *bc,
+/*
+ * Set broadcast interrupt affinity
+ */
+static void tick_broadcast_set_affinity(struct clock_event_device *bc,
+ const struct cpumask *cpumask)
+{
+ if (!(bc-features CLOCK_EVT_FEAT_DYNIRQ))
+ return;
+
+ if (cpumask_equal(bc-cpumask, cpumask))
+ return;
+
+ bc-cpumask = cpumask;
This breaks with CONFIG_CPUMASK_OFFSTACK=y. cpumask_copy() is your friend!
This instruction copies the pointer, not the cpumask content.
bc-cpumask is defined as a const struct cpumask * and is used to copy a
cpumask pointer not the content.
The cpumask parameter is a pointer to a global cpumask provided by the
cpumask_of macro.
But to be in the safe side, I compiled tested with
CONFIG_CPUMASK_OFFSTACK=y without problem.
Hi Thomas,
thanks for merging the patch 1 and 2.
I was wondering if it would be possible to take the 3/4 and 4/4
otherwise the flag dependency will prevent to send those to the
maintainer's tree until they gain visibility on it.
--
http://www.linaro.org/ Linaro.org │ Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro Facebook |
http://twitter.com/#!/linaroorg Twitter |
http://www.linaro.org/linaro-blog/ Blog
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On Fri, 8 Mar 2013, Daniel Lezcano wrote: On 03/06/2013 10:48 AM, Thomas Gleixner wrote: I was wondering if it would be possible to take the 3/4 and 4/4 otherwise the flag dependency will prevent to send those to the maintainer's tree until they gain visibility on it. I can take them with the ack of arm soc folks. Thanks, tglx -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On 03/06/2013 10:48 AM, Thomas Gleixner wrote:
> On Wed, 6 Mar 2013, Daniel Lezcano wrote:
>> On 03/05/2013 09:40 PM, Thomas Gleixner wrote:
>>> On Sat, 2 Mar 2013, Daniel Lezcano wrote:
When a cpu goes to a deep idle state where its local timer is shutdown,
it notifies the time frame work to use the broadcast timer instead.
Unfortunately, the broadcast device could wake up any CPU, including an
idle one which is not concerned by the wake up at all.
This implies, in the worst case, an idle CPU will wake up to send an IPI
to another idle cpu.
This patch solves this by setting the irq affinity to the cpu concerned
by the nearest timer event, by this way, the CPU which is wake up is
guarantee to be the one concerned by the next event and we are safe with
unnecessary wakeup for another idle CPU.
As the irq affinity is not supported by all the archs, a flag is needed
to specify which clocksource can handle it : CLOCK_EVT_FEAT_DYNIRQ
Signed-off-by: Daniel Lezcano
---
include/linux/clockchips.h |5 +
kernel/time/tick-broadcast.c | 40
+---
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 6634652..c93e2a6 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -55,6 +55,11 @@ enum clock_event_nofitiers {
#define CLOCK_EVT_FEAT_C3STOP 0x08
#define CLOCK_EVT_FEAT_DUMMY 0x10
+/*
+ * Clock event device can set its irq affinity dynamically
+ */
+#define CLOCK_EVT_FEAT_DYNIRQ 0x20
+
/**
* struct clock_event_device - clock event device descriptor
* @event_handler:Assigned by the framework to be called by the
low
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 6197ac0..9ca8ff5 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -406,13 +406,37 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void)
return to_cpumask(tick_broadcast_oneshot_mask);
}
-static int tick_broadcast_set_event(struct clock_event_device *bc,
+/*
+ * Set broadcast interrupt affinity
+ */
+static void tick_broadcast_set_affinity(struct clock_event_device *bc,
+ const struct cpumask *cpumask)
+{
+ if (!(bc->features & CLOCK_EVT_FEAT_DYNIRQ))
+ return;
+
+ if (cpumask_equal(bc->cpumask, cpumask))
+ return;
+
+ bc->cpumask = cpumask;
>>>
>>> This breaks with CONFIG_CPUMASK_OFFSTACK=y. cpumask_copy() is your friend!
>>
>> This instruction copies the pointer, not the cpumask content.
>>
>> bc->cpumask is defined as a const struct cpumask * and is used to copy a
>> cpumask pointer not the content.
>>
>> The cpumask parameter is a pointer to a global cpumask provided by the
>> cpumask_of macro.
>>
>> But to be in the safe side, I compiled tested with
>> CONFIG_CPUMASK_OFFSTACK=y without problem.
>>
>> Did I missed something ?
>
> No, I misinterpreted the patch. Assigning a pointer is safe.
Ok, thanks anyway for reviewing the patch.
Do you think it is acceptable for upstreaming ?
-- Daniel
Facebook |
Twitter |
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On Wed, 6 Mar 2013, Daniel Lezcano wrote:
> On 03/05/2013 09:40 PM, Thomas Gleixner wrote:
> > On Sat, 2 Mar 2013, Daniel Lezcano wrote:
> >> When a cpu goes to a deep idle state where its local timer is shutdown,
> >> it notifies the time frame work to use the broadcast timer instead.
> >>
> >> Unfortunately, the broadcast device could wake up any CPU, including an
> >> idle one which is not concerned by the wake up at all.
> >>
> >> This implies, in the worst case, an idle CPU will wake up to send an IPI
> >> to another idle cpu.
> >>
> >> This patch solves this by setting the irq affinity to the cpu concerned
> >> by the nearest timer event, by this way, the CPU which is wake up is
> >> guarantee to be the one concerned by the next event and we are safe with
> >> unnecessary wakeup for another idle CPU.
> >>
> >> As the irq affinity is not supported by all the archs, a flag is needed
> >> to specify which clocksource can handle it : CLOCK_EVT_FEAT_DYNIRQ
> >>
> >> Signed-off-by: Daniel Lezcano
> >> ---
> >> include/linux/clockchips.h |5 +
> >> kernel/time/tick-broadcast.c | 40
> >> +---
> >> 2 files changed, 38 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
> >> index 6634652..c93e2a6 100644
> >> --- a/include/linux/clockchips.h
> >> +++ b/include/linux/clockchips.h
> >> @@ -55,6 +55,11 @@ enum clock_event_nofitiers {
> >> #define CLOCK_EVT_FEAT_C3STOP 0x08
> >> #define CLOCK_EVT_FEAT_DUMMY 0x10
> >>
> >> +/*
> >> + * Clock event device can set its irq affinity dynamically
> >> + */
> >> +#define CLOCK_EVT_FEAT_DYNIRQ 0x20
> >> +
> >> /**
> >> * struct clock_event_device - clock event device descriptor
> >> * @event_handler:Assigned by the framework to be called by the
> >> low
> >> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> >> index 6197ac0..9ca8ff5 100644
> >> --- a/kernel/time/tick-broadcast.c
> >> +++ b/kernel/time/tick-broadcast.c
> >> @@ -406,13 +406,37 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void)
> >>return to_cpumask(tick_broadcast_oneshot_mask);
> >> }
> >>
> >> -static int tick_broadcast_set_event(struct clock_event_device *bc,
> >> +/*
> >> + * Set broadcast interrupt affinity
> >> + */
> >> +static void tick_broadcast_set_affinity(struct clock_event_device *bc,
> >> + const struct cpumask *cpumask)
> >> +{
> >> + if (!(bc->features & CLOCK_EVT_FEAT_DYNIRQ))
> >> + return;
> >> +
> >> + if (cpumask_equal(bc->cpumask, cpumask))
> >> + return;
> >> +
> >> + bc->cpumask = cpumask;
> >
> > This breaks with CONFIG_CPUMASK_OFFSTACK=y. cpumask_copy() is your friend!
>
> This instruction copies the pointer, not the cpumask content.
>
> bc->cpumask is defined as a const struct cpumask * and is used to copy a
> cpumask pointer not the content.
>
> The cpumask parameter is a pointer to a global cpumask provided by the
> cpumask_of macro.
>
> But to be in the safe side, I compiled tested with
> CONFIG_CPUMASK_OFFSTACK=y without problem.
>
> Did I missed something ?
No, I misinterpreted the patch. Assigning a pointer is safe.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On 03/05/2013 09:40 PM, Thomas Gleixner wrote:
> On Sat, 2 Mar 2013, Daniel Lezcano wrote:
>> When a cpu goes to a deep idle state where its local timer is shutdown,
>> it notifies the time frame work to use the broadcast timer instead.
>>
>> Unfortunately, the broadcast device could wake up any CPU, including an
>> idle one which is not concerned by the wake up at all.
>>
>> This implies, in the worst case, an idle CPU will wake up to send an IPI
>> to another idle cpu.
>>
>> This patch solves this by setting the irq affinity to the cpu concerned
>> by the nearest timer event, by this way, the CPU which is wake up is
>> guarantee to be the one concerned by the next event and we are safe with
>> unnecessary wakeup for another idle CPU.
>>
>> As the irq affinity is not supported by all the archs, a flag is needed
>> to specify which clocksource can handle it : CLOCK_EVT_FEAT_DYNIRQ
>>
>> Signed-off-by: Daniel Lezcano
>> ---
>> include/linux/clockchips.h |5 +
>> kernel/time/tick-broadcast.c | 40 +---
>> 2 files changed, 38 insertions(+), 7 deletions(-)
>>
>> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
>> index 6634652..c93e2a6 100644
>> --- a/include/linux/clockchips.h
>> +++ b/include/linux/clockchips.h
>> @@ -55,6 +55,11 @@ enum clock_event_nofitiers {
>> #define CLOCK_EVT_FEAT_C3STOP 0x08
>> #define CLOCK_EVT_FEAT_DUMMY0x10
>>
>> +/*
>> + * Clock event device can set its irq affinity dynamically
>> + */
>> +#define CLOCK_EVT_FEAT_DYNIRQ 0x20
>> +
>> /**
>> * struct clock_event_device - clock event device descriptor
>> * @event_handler: Assigned by the framework to be called by the low
>> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
>> index 6197ac0..9ca8ff5 100644
>> --- a/kernel/time/tick-broadcast.c
>> +++ b/kernel/time/tick-broadcast.c
>> @@ -406,13 +406,37 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void)
>> return to_cpumask(tick_broadcast_oneshot_mask);
>> }
>>
>> -static int tick_broadcast_set_event(struct clock_event_device *bc,
>> +/*
>> + * Set broadcast interrupt affinity
>> + */
>> +static void tick_broadcast_set_affinity(struct clock_event_device *bc,
>> +const struct cpumask *cpumask)
>> +{
>> +if (!(bc->features & CLOCK_EVT_FEAT_DYNIRQ))
>> +return;
>> +
>> +if (cpumask_equal(bc->cpumask, cpumask))
>> +return;
>> +
>> +bc->cpumask = cpumask;
>
> This breaks with CONFIG_CPUMASK_OFFSTACK=y. cpumask_copy() is your friend!
This instruction copies the pointer, not the cpumask content.
bc->cpumask is defined as a const struct cpumask * and is used to copy a
cpumask pointer not the content.
The cpumask parameter is a pointer to a global cpumask provided by the
cpumask_of macro.
But to be in the safe side, I compiled tested with
CONFIG_CPUMASK_OFFSTACK=y without problem.
Did I missed something ?
Thanks
-- Daniel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pm" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Facebook |
Twitter |
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On 03/05/2013 09:40 PM, Thomas Gleixner wrote:
On Sat, 2 Mar 2013, Daniel Lezcano wrote:
When a cpu goes to a deep idle state where its local timer is shutdown,
it notifies the time frame work to use the broadcast timer instead.
Unfortunately, the broadcast device could wake up any CPU, including an
idle one which is not concerned by the wake up at all.
This implies, in the worst case, an idle CPU will wake up to send an IPI
to another idle cpu.
This patch solves this by setting the irq affinity to the cpu concerned
by the nearest timer event, by this way, the CPU which is wake up is
guarantee to be the one concerned by the next event and we are safe with
unnecessary wakeup for another idle CPU.
As the irq affinity is not supported by all the archs, a flag is needed
to specify which clocksource can handle it : CLOCK_EVT_FEAT_DYNIRQ
Signed-off-by: Daniel Lezcano daniel.lezc...@linaro.org
---
include/linux/clockchips.h |5 +
kernel/time/tick-broadcast.c | 40 +---
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 6634652..c93e2a6 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -55,6 +55,11 @@ enum clock_event_nofitiers {
#define CLOCK_EVT_FEAT_C3STOP 0x08
#define CLOCK_EVT_FEAT_DUMMY0x10
+/*
+ * Clock event device can set its irq affinity dynamically
+ */
+#define CLOCK_EVT_FEAT_DYNIRQ 0x20
+
/**
* struct clock_event_device - clock event device descriptor
* @event_handler: Assigned by the framework to be called by the low
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 6197ac0..9ca8ff5 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -406,13 +406,37 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void)
return to_cpumask(tick_broadcast_oneshot_mask);
}
-static int tick_broadcast_set_event(struct clock_event_device *bc,
+/*
+ * Set broadcast interrupt affinity
+ */
+static void tick_broadcast_set_affinity(struct clock_event_device *bc,
+const struct cpumask *cpumask)
+{
+if (!(bc-features CLOCK_EVT_FEAT_DYNIRQ))
+return;
+
+if (cpumask_equal(bc-cpumask, cpumask))
+return;
+
+bc-cpumask = cpumask;
This breaks with CONFIG_CPUMASK_OFFSTACK=y. cpumask_copy() is your friend!
This instruction copies the pointer, not the cpumask content.
bc-cpumask is defined as a const struct cpumask * and is used to copy a
cpumask pointer not the content.
The cpumask parameter is a pointer to a global cpumask provided by the
cpumask_of macro.
But to be in the safe side, I compiled tested with
CONFIG_CPUMASK_OFFSTACK=y without problem.
Did I missed something ?
Thanks
-- Daniel
--
To unsubscribe from this list: send the line unsubscribe linux-pm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
http://www.linaro.org/ Linaro.org │ Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro Facebook |
http://twitter.com/#!/linaroorg Twitter |
http://www.linaro.org/linaro-blog/ Blog
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On Wed, 6 Mar 2013, Daniel Lezcano wrote:
On 03/05/2013 09:40 PM, Thomas Gleixner wrote:
On Sat, 2 Mar 2013, Daniel Lezcano wrote:
When a cpu goes to a deep idle state where its local timer is shutdown,
it notifies the time frame work to use the broadcast timer instead.
Unfortunately, the broadcast device could wake up any CPU, including an
idle one which is not concerned by the wake up at all.
This implies, in the worst case, an idle CPU will wake up to send an IPI
to another idle cpu.
This patch solves this by setting the irq affinity to the cpu concerned
by the nearest timer event, by this way, the CPU which is wake up is
guarantee to be the one concerned by the next event and we are safe with
unnecessary wakeup for another idle CPU.
As the irq affinity is not supported by all the archs, a flag is needed
to specify which clocksource can handle it : CLOCK_EVT_FEAT_DYNIRQ
Signed-off-by: Daniel Lezcano daniel.lezc...@linaro.org
---
include/linux/clockchips.h |5 +
kernel/time/tick-broadcast.c | 40
+---
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 6634652..c93e2a6 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -55,6 +55,11 @@ enum clock_event_nofitiers {
#define CLOCK_EVT_FEAT_C3STOP 0x08
#define CLOCK_EVT_FEAT_DUMMY 0x10
+/*
+ * Clock event device can set its irq affinity dynamically
+ */
+#define CLOCK_EVT_FEAT_DYNIRQ 0x20
+
/**
* struct clock_event_device - clock event device descriptor
* @event_handler:Assigned by the framework to be called by the
low
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 6197ac0..9ca8ff5 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -406,13 +406,37 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void)
return to_cpumask(tick_broadcast_oneshot_mask);
}
-static int tick_broadcast_set_event(struct clock_event_device *bc,
+/*
+ * Set broadcast interrupt affinity
+ */
+static void tick_broadcast_set_affinity(struct clock_event_device *bc,
+ const struct cpumask *cpumask)
+{
+ if (!(bc-features CLOCK_EVT_FEAT_DYNIRQ))
+ return;
+
+ if (cpumask_equal(bc-cpumask, cpumask))
+ return;
+
+ bc-cpumask = cpumask;
This breaks with CONFIG_CPUMASK_OFFSTACK=y. cpumask_copy() is your friend!
This instruction copies the pointer, not the cpumask content.
bc-cpumask is defined as a const struct cpumask * and is used to copy a
cpumask pointer not the content.
The cpumask parameter is a pointer to a global cpumask provided by the
cpumask_of macro.
But to be in the safe side, I compiled tested with
CONFIG_CPUMASK_OFFSTACK=y without problem.
Did I missed something ?
No, I misinterpreted the patch. Assigning a pointer is safe.
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On 03/06/2013 10:48 AM, Thomas Gleixner wrote:
On Wed, 6 Mar 2013, Daniel Lezcano wrote:
On 03/05/2013 09:40 PM, Thomas Gleixner wrote:
On Sat, 2 Mar 2013, Daniel Lezcano wrote:
When a cpu goes to a deep idle state where its local timer is shutdown,
it notifies the time frame work to use the broadcast timer instead.
Unfortunately, the broadcast device could wake up any CPU, including an
idle one which is not concerned by the wake up at all.
This implies, in the worst case, an idle CPU will wake up to send an IPI
to another idle cpu.
This patch solves this by setting the irq affinity to the cpu concerned
by the nearest timer event, by this way, the CPU which is wake up is
guarantee to be the one concerned by the next event and we are safe with
unnecessary wakeup for another idle CPU.
As the irq affinity is not supported by all the archs, a flag is needed
to specify which clocksource can handle it : CLOCK_EVT_FEAT_DYNIRQ
Signed-off-by: Daniel Lezcano daniel.lezc...@linaro.org
---
include/linux/clockchips.h |5 +
kernel/time/tick-broadcast.c | 40
+---
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 6634652..c93e2a6 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -55,6 +55,11 @@ enum clock_event_nofitiers {
#define CLOCK_EVT_FEAT_C3STOP 0x08
#define CLOCK_EVT_FEAT_DUMMY 0x10
+/*
+ * Clock event device can set its irq affinity dynamically
+ */
+#define CLOCK_EVT_FEAT_DYNIRQ 0x20
+
/**
* struct clock_event_device - clock event device descriptor
* @event_handler:Assigned by the framework to be called by the
low
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 6197ac0..9ca8ff5 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -406,13 +406,37 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void)
return to_cpumask(tick_broadcast_oneshot_mask);
}
-static int tick_broadcast_set_event(struct clock_event_device *bc,
+/*
+ * Set broadcast interrupt affinity
+ */
+static void tick_broadcast_set_affinity(struct clock_event_device *bc,
+ const struct cpumask *cpumask)
+{
+ if (!(bc-features CLOCK_EVT_FEAT_DYNIRQ))
+ return;
+
+ if (cpumask_equal(bc-cpumask, cpumask))
+ return;
+
+ bc-cpumask = cpumask;
This breaks with CONFIG_CPUMASK_OFFSTACK=y. cpumask_copy() is your friend!
This instruction copies the pointer, not the cpumask content.
bc-cpumask is defined as a const struct cpumask * and is used to copy a
cpumask pointer not the content.
The cpumask parameter is a pointer to a global cpumask provided by the
cpumask_of macro.
But to be in the safe side, I compiled tested with
CONFIG_CPUMASK_OFFSTACK=y without problem.
Did I missed something ?
No, I misinterpreted the patch. Assigning a pointer is safe.
Ok, thanks anyway for reviewing the patch.
Do you think it is acceptable for upstreaming ?
-- Daniel
http://www.linaro.org/ Linaro.org │ Open source software for ARM SoCs
Follow Linaro: http://www.facebook.com/pages/Linaro Facebook |
http://twitter.com/#!/linaroorg Twitter |
http://www.linaro.org/linaro-blog/ Blog
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On Sat, 2 Mar 2013, Daniel Lezcano wrote:
> When a cpu goes to a deep idle state where its local timer is shutdown,
> it notifies the time frame work to use the broadcast timer instead.
>
> Unfortunately, the broadcast device could wake up any CPU, including an
> idle one which is not concerned by the wake up at all.
>
> This implies, in the worst case, an idle CPU will wake up to send an IPI
> to another idle cpu.
>
> This patch solves this by setting the irq affinity to the cpu concerned
> by the nearest timer event, by this way, the CPU which is wake up is
> guarantee to be the one concerned by the next event and we are safe with
> unnecessary wakeup for another idle CPU.
>
> As the irq affinity is not supported by all the archs, a flag is needed
> to specify which clocksource can handle it : CLOCK_EVT_FEAT_DYNIRQ
>
> Signed-off-by: Daniel Lezcano
> ---
> include/linux/clockchips.h |5 +
> kernel/time/tick-broadcast.c | 40 +---
> 2 files changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
> index 6634652..c93e2a6 100644
> --- a/include/linux/clockchips.h
> +++ b/include/linux/clockchips.h
> @@ -55,6 +55,11 @@ enum clock_event_nofitiers {
> #define CLOCK_EVT_FEAT_C3STOP0x08
> #define CLOCK_EVT_FEAT_DUMMY 0x10
>
> +/*
> + * Clock event device can set its irq affinity dynamically
> + */
> +#define CLOCK_EVT_FEAT_DYNIRQ0x20
> +
> /**
> * struct clock_event_device - clock event device descriptor
> * @event_handler: Assigned by the framework to be called by the low
> diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
> index 6197ac0..9ca8ff5 100644
> --- a/kernel/time/tick-broadcast.c
> +++ b/kernel/time/tick-broadcast.c
> @@ -406,13 +406,37 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void)
> return to_cpumask(tick_broadcast_oneshot_mask);
> }
>
> -static int tick_broadcast_set_event(struct clock_event_device *bc,
> +/*
> + * Set broadcast interrupt affinity
> + */
> +static void tick_broadcast_set_affinity(struct clock_event_device *bc,
> + const struct cpumask *cpumask)
> +{
> + if (!(bc->features & CLOCK_EVT_FEAT_DYNIRQ))
> + return;
> +
> + if (cpumask_equal(bc->cpumask, cpumask))
> + return;
> +
> + bc->cpumask = cpumask;
This breaks with CONFIG_CPUMASK_OFFSTACK=y. cpumask_copy() is your friend!
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 2/4][V2] time : set broadcast irq affinity
On Sat, 2 Mar 2013, Daniel Lezcano wrote:
When a cpu goes to a deep idle state where its local timer is shutdown,
it notifies the time frame work to use the broadcast timer instead.
Unfortunately, the broadcast device could wake up any CPU, including an
idle one which is not concerned by the wake up at all.
This implies, in the worst case, an idle CPU will wake up to send an IPI
to another idle cpu.
This patch solves this by setting the irq affinity to the cpu concerned
by the nearest timer event, by this way, the CPU which is wake up is
guarantee to be the one concerned by the next event and we are safe with
unnecessary wakeup for another idle CPU.
As the irq affinity is not supported by all the archs, a flag is needed
to specify which clocksource can handle it : CLOCK_EVT_FEAT_DYNIRQ
Signed-off-by: Daniel Lezcano daniel.lezc...@linaro.org
---
include/linux/clockchips.h |5 +
kernel/time/tick-broadcast.c | 40 +---
2 files changed, 38 insertions(+), 7 deletions(-)
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 6634652..c93e2a6 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -55,6 +55,11 @@ enum clock_event_nofitiers {
#define CLOCK_EVT_FEAT_C3STOP0x08
#define CLOCK_EVT_FEAT_DUMMY 0x10
+/*
+ * Clock event device can set its irq affinity dynamically
+ */
+#define CLOCK_EVT_FEAT_DYNIRQ0x20
+
/**
* struct clock_event_device - clock event device descriptor
* @event_handler: Assigned by the framework to be called by the low
diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c
index 6197ac0..9ca8ff5 100644
--- a/kernel/time/tick-broadcast.c
+++ b/kernel/time/tick-broadcast.c
@@ -406,13 +406,37 @@ struct cpumask *tick_get_broadcast_oneshot_mask(void)
return to_cpumask(tick_broadcast_oneshot_mask);
}
-static int tick_broadcast_set_event(struct clock_event_device *bc,
+/*
+ * Set broadcast interrupt affinity
+ */
+static void tick_broadcast_set_affinity(struct clock_event_device *bc,
+ const struct cpumask *cpumask)
+{
+ if (!(bc-features CLOCK_EVT_FEAT_DYNIRQ))
+ return;
+
+ if (cpumask_equal(bc-cpumask, cpumask))
+ return;
+
+ bc-cpumask = cpumask;
This breaks with CONFIG_CPUMASK_OFFSTACK=y. cpumask_copy() is your friend!
Thanks,
tglx
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
