From: Raghavendra K T <[email protected]>
Changelog:
Reordering in header files and adding declarations to enable
spinlock header to use jump label technique.
Signed-off-by: Raghavendra K T <[email protected]>
---
I was re-basing Jermey patches (https://lkml.org/lkml/2011/10/12/496), while working
on paravirtualized ticket spinlock (3.3.-rc3).
Currently <jump_label.h> includes <workqueue.h> (commit: b202952075f62603bea9bfb6ebc6b0420db11949)
So we get following error when we try to include jump_label.h from
arch/x86/include/asm/spinlock.h because of cyclic dependency
<spinlock.h> -> <jumplabe.h> -> <workque.h> -> ... <seqlock.h> -> <spinlock.h>
CHK include/linux/version.h
CHK include/generated/utsrelease.h
CC arch/x86/kernel/asm-offsets.s
In file included from include/linux/time.h:8:0,
from include/linux/ktime.h:24,
from include/linux/timer.h:5,
from include/linux/workqueue.h:8,
from include/linux/jump_label.h:6,
from /home/raghu/linux-3.3-rc3/arch/x86/include/asm/spinlock.h:4,
from include/linux/spinlock.h:87,
from include/linux/mmzone.h:7,
from include/linux/gfp.h:4,
from include/linux/slab.h:12,
from include/linux/crypto.h:23,
from arch/x86/kernel/asm-offsets.c:8:
include/linux/seqlock.h: In function ‘write_seqlock’:
include/linux/seqlock.h:60:2: error: implicit declaration of function ‘spin_lock’ [-Werror=implicit-function-declaration]
include/linux/seqlock.h: In function ‘write_sequnlock’:
include/linux/seqlock.h:69:2: error: implicit declaration of function ‘spin_unlock’ [-Werror=implicit-function-declaration]
include/linux/seqlock.h: In function ‘write_tryseqlock’:
include/linux/seqlock.h:74:2: error: implicit declaration of function ‘spin_trylock’ [-Werror=implicit-function-declaration]
In file included from include/linux/mmzone.h:7:0,
from include/linux/gfp.h:4,
from include/linux/slab.h:12,
from include/linux/crypto.h:23,
from arch/x86/kernel/asm-offsets.c:8:
include/linux/spinlock.h: At top level:
include/linux/spinlock.h:283:20: warning: conflicting types for ‘spin_lock’ [enabled by default]
include/linux/spinlock.h:283:20: error: static declaration of ‘spin_lock’ follows non-static declaration
include/linux/seqlock.h:60:2: note: previous implicit declaration of ‘spin_lock’ was here
include/linux/spinlock.h:293:19: error: static declaration of ‘spin_trylock’ follows non-static declaration
include/linux/seqlock.h:74:12: note: previous implicit declaration of ‘spin_trylock’ was here
include/linux/spinlock.h:323:20: warning: conflicting types for ‘spin_unlock’ [enabled by default]
include/linux/spinlock.h:323:20: error: static declaration of ‘spin_unlock’ follows non-static declaration
include/linux/seqlock.h:69:2: note: previous implicit declaration of ‘spin_unlock’ was here
cc1: some warnings being treated as errors
make[1]: *** [arch/x86/kernel/asm-offsets.s] Error 1
make: *** [prepare0] Error 2
The patch below fixes it. (Is there any alternative? But this patch is needed before paravirt
ticket-spinlock can go in :( ).
Tesing:
Compile tested with i386 defconfig and x86_64 defconfig. Below result is for x86_64
size Before the patch
================
text data bss dec hex filename
9901730 937168 1146880 11985778 b6e372 vmlinux
size After the patch
================
text data bss dec hex filename
9901730 937168 1146880 11985778 b6e372 vmlinux
include/linux/ktime.h | 2 ++
include/linux/seqlock.h | 4 ++++
include/linux/time.h | 20 +++++++++++---------
include/linux/timer.h | 4 ++--
include/linux/timex.h | 11 ++++++-----
include/linux/workqueue.h | 3 +++
6 files changed, 28 insertions(+), 16 deletions(-)
diff --git a/include/linux/ktime.h b/include/linux/ktime.h
index 603bec2..90547eb 100644
--- a/include/linux/ktime.h
+++ b/include/linux/ktime.h
@@ -289,6 +289,8 @@ static inline int ktime_equal(const ktime_t cmp1, const ktime_t cmp2)
return cmp1.tv64 == cmp2.tv64;
}
+extern struct timeval ns_to_timeval(const s64 nsec);
+
static inline s64 ktime_to_us(const ktime_t kt)
{
struct timeval tv = ktime_to_timeval(kt);
diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
index c6db9fb..5b4a9e9 100644
--- a/include/linux/seqlock.h
+++ b/include/linux/seqlock.h
@@ -51,6 +51,10 @@ typedef struct {
#define DEFINE_SEQLOCK(x) \
seqlock_t x = __SEQLOCK_UNLOCKED(x)
+static inline void spin_lock(spinlock_t *lock);
+static inline int spin_trylock(spinlock_t *lock);
+static inline void spin_unlock(spinlock_t *lock);
+
/* Lock out other writers and update the count.
* Acts like a normal spin_lock/unlock.
* Don't need preempt_disable() because that is in the spin_lock already.
diff --git a/include/linux/time.h b/include/linux/time.h
index b306178..b3ce366 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -3,12 +3,6 @@
#include <linux/types.h>
-#ifdef __KERNEL__
-# include <linux/cache.h>
-# include <linux/seqlock.h>
-# include <linux/math64.h>
-#endif
-
#ifndef _STRUCT_TIMESPEC
#define _STRUCT_TIMESPEC
struct timespec {
@@ -28,9 +22,6 @@ struct timezone {
};
#ifdef __KERNEL__
-
-extern struct timezone sys_tz;
-
/* Parameters used to convert the timespec values: */
#define MSEC_PER_SEC 1000L
#define USEC_PER_MSEC 1000L
@@ -42,6 +33,17 @@ extern struct timezone sys_tz;
#define TIME_T_MAX (time_t)((1UL << ((sizeof(time_t) << 3) - 1)) - 1)
+# include <linux/cache.h>
+# include <linux/seqlock.h>
+# include <linux/math64.h>
+#endif
+
+
+#ifdef __KERNEL__
+
+extern struct timezone sys_tz;
+
+
static inline int timespec_equal(const struct timespec *a,
const struct timespec *b)
{
diff --git a/include/linux/timer.h b/include/linux/timer.h
index 6abd913..25bd863 100644
--- a/include/linux/timer.h
+++ b/include/linux/timer.h
@@ -2,9 +2,7 @@
#define _LINUX_TIMER_H
#include <linux/list.h>
-#include <linux/ktime.h>
#include <linux/stddef.h>
-#include <linux/debugobjects.h>
#include <linux/stringify.h>
struct tvec_base;
@@ -33,6 +31,8 @@ struct timer_list {
#endif
};
+#include <linux/ktime.h>
+#include <linux/debugobjects.h>
extern struct tvec_base boot_tvec_bases;
#ifdef CONFIG_LOCKDEP
diff --git a/include/linux/timex.h b/include/linux/timex.h
index aa60fe7..91ef0fa 100644
--- a/include/linux/timex.h
+++ b/include/linux/timex.h
@@ -53,6 +53,12 @@
#ifndef _LINUX_TIMEX_H
#define _LINUX_TIMEX_H
+#ifdef __KERNEL__
+/* The clock frequency of the i8253/i8254 PIT */
+#define PIT_TICK_RATE 1193182ul
+#include <asm/timex.h>
+#endif /* __KERNEL__ */
+
#include <linux/time.h>
#define NTP_API 4 /* NTP API version */
@@ -171,8 +177,6 @@ struct timex {
#include <linux/types.h>
#include <linux/param.h>
-#include <asm/timex.h>
-
/*
* SHIFT_PLL is used as a dampening factor to define how much we
* adjust the frequency correction for a given offset in PLL mode.
@@ -273,9 +277,6 @@ extern void hardpps(const struct timespec *, const struct timespec *);
int read_current_timer(unsigned long *timer_val);
-/* The clock frequency of the i8253/i8254 PIT */
-#define PIT_TICK_RATE 1193182ul
-
#endif /* KERNEL */
#endif /* LINUX_TIMEX_H */
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index eb8b9f1..ba1588e 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -397,6 +397,9 @@ extern bool workqueue_congested(unsigned int cpu, struct workqueue_struct *wq);
extern unsigned int work_cpu(struct work_struct *work);
extern unsigned int work_busy(struct work_struct *work);
+extern int del_timer_sync(struct timer_list *timer);
+extern int del_timer(struct timer_list *timer);
+
/*
* Kill off a pending schedule_delayed_work(). Note that the work callback
* function may still be running on return from cancel_delayed_work(), unless
On 02/17/2012 12:25 AM, Raghavendra K T wrote:
> From: Raghavendra K T <[email protected]>
>
> Changelog:
> Reordering in header files and adding declarations to enable
> spinlock header to use jump label technique.
>
> Signed-off-by: Raghavendra K T <[email protected]>
> ---
> I was re-basing Jermey patches (https://lkml.org/lkml/2011/10/12/496), while working
> on paravirtualized ticket spinlock (3.3.-rc3).
>
> Currently <jump_label.h> includes <workqueue.h> (commit: b202952075f62603bea9bfb6ebc6b0420db11949)
>
> So we get following error when we try to include jump_label.h from
> arch/x86/include/asm/spinlock.h because of cyclic dependency
> <spinlock.h> -> <jumplabe.h> -> <workque.h> -> ... <seqlock.h> -> <spinlock.h>
What about splitting the jump_label_key_deferred stuff into a separate
jump_label_deferred.h, and just include that where it's needed?
J
>
> CHK include/linux/version.h
> CHK include/generated/utsrelease.h
> CC arch/x86/kernel/asm-offsets.s
> In file included from include/linux/time.h:8:0,
> from include/linux/ktime.h:24,
> from include/linux/timer.h:5,
> from include/linux/workqueue.h:8,
> from include/linux/jump_label.h:6,
> from /home/raghu/linux-3.3-rc3/arch/x86/include/asm/spinlock.h:4,
> from include/linux/spinlock.h:87,
> from include/linux/mmzone.h:7,
> from include/linux/gfp.h:4,
> from include/linux/slab.h:12,
> from include/linux/crypto.h:23,
> from arch/x86/kernel/asm-offsets.c:8:
> include/linux/seqlock.h: In function ‘write_seqlock’:
> include/linux/seqlock.h:60:2: error: implicit declaration of function ‘spin_lock’ [-Werror=implicit-function-declaration]
> include/linux/seqlock.h: In function ‘write_sequnlock’:
> include/linux/seqlock.h:69:2: error: implicit declaration of function ‘spin_unlock’ [-Werror=implicit-function-declaration]
> include/linux/seqlock.h: In function ‘write_tryseqlock’:
> include/linux/seqlock.h:74:2: error: implicit declaration of function ‘spin_trylock’ [-Werror=implicit-function-declaration]
> In file included from include/linux/mmzone.h:7:0,
> from include/linux/gfp.h:4,
> from include/linux/slab.h:12,
> from include/linux/crypto.h:23,
> from arch/x86/kernel/asm-offsets.c:8:
> include/linux/spinlock.h: At top level:
> include/linux/spinlock.h:283:20: warning: conflicting types for ‘spin_lock’ [enabled by default]
> include/linux/spinlock.h:283:20: error: static declaration of ‘spin_lock’ follows non-static declaration
> include/linux/seqlock.h:60:2: note: previous implicit declaration of ‘spin_lock’ was here
> include/linux/spinlock.h:293:19: error: static declaration of ‘spin_trylock’ follows non-static declaration
> include/linux/seqlock.h:74:12: note: previous implicit declaration of ‘spin_trylock’ was here
> include/linux/spinlock.h:323:20: warning: conflicting types for ‘spin_unlock’ [enabled by default]
> include/linux/spinlock.h:323:20: error: static declaration of ‘spin_unlock’ follows non-static declaration
> include/linux/seqlock.h:69:2: note: previous implicit declaration of ‘spin_unlock’ was here
> cc1: some warnings being treated as errors
>
> make[1]: *** [arch/x86/kernel/asm-offsets.s] Error 1
> make: *** [prepare0] Error 2
>
> The patch below fixes it. (Is there any alternative? But this patch is needed before paravirt
> ticket-spinlock can go in :( ).
>
> Tesing:
> Compile tested with i386 defconfig and x86_64 defconfig. Below result is for x86_64
>
> size Before the patch
> ================
> text data bss dec hex filename
> 9901730 937168 1146880 11985778 b6e372 vmlinux
>
> size After the patch
> ================
> text data bss dec hex filename
> 9901730 937168 1146880 11985778 b6e372 vmlinux
>
> include/linux/ktime.h | 2 ++
> include/linux/seqlock.h | 4 ++++
> include/linux/time.h | 20 +++++++++++---------
> include/linux/timer.h | 4 ++--
> include/linux/timex.h | 11 ++++++-----
> include/linux/workqueue.h | 3 +++
> 6 files changed, 28 insertions(+), 16 deletions(-)
>
> diff --git a/include/linux/ktime.h b/include/linux/ktime.h
> index 603bec2..90547eb 100644
> --- a/include/linux/ktime.h
> +++ b/include/linux/ktime.h
> @@ -289,6 +289,8 @@ static inline int ktime_equal(const ktime_t cmp1, const ktime_t cmp2)
> return cmp1.tv64 == cmp2.tv64;
> }
>
> +extern struct timeval ns_to_timeval(const s64 nsec);
> +
> static inline s64 ktime_to_us(const ktime_t kt)
> {
> struct timeval tv = ktime_to_timeval(kt);
> diff --git a/include/linux/seqlock.h b/include/linux/seqlock.h
> index c6db9fb..5b4a9e9 100644
> --- a/include/linux/seqlock.h
> +++ b/include/linux/seqlock.h
> @@ -51,6 +51,10 @@ typedef struct {
> #define DEFINE_SEQLOCK(x) \
> seqlock_t x = __SEQLOCK_UNLOCKED(x)
>
> +static inline void spin_lock(spinlock_t *lock);
> +static inline int spin_trylock(spinlock_t *lock);
> +static inline void spin_unlock(spinlock_t *lock);
> +
> /* Lock out other writers and update the count.
> * Acts like a normal spin_lock/unlock.
> * Don't need preempt_disable() because that is in the spin_lock already.
> diff --git a/include/linux/time.h b/include/linux/time.h
> index b306178..b3ce366 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -3,12 +3,6 @@
>
> #include <linux/types.h>
>
> -#ifdef __KERNEL__
> -# include <linux/cache.h>
> -# include <linux/seqlock.h>
> -# include <linux/math64.h>
> -#endif
> -
> #ifndef _STRUCT_TIMESPEC
> #define _STRUCT_TIMESPEC
> struct timespec {
> @@ -28,9 +22,6 @@ struct timezone {
> };
>
> #ifdef __KERNEL__
> -
> -extern struct timezone sys_tz;
> -
> /* Parameters used to convert the timespec values: */
> #define MSEC_PER_SEC 1000L
> #define USEC_PER_MSEC 1000L
> @@ -42,6 +33,17 @@ extern struct timezone sys_tz;
>
> #define TIME_T_MAX (time_t)((1UL << ((sizeof(time_t) << 3) - 1)) - 1)
>
> +# include <linux/cache.h>
> +# include <linux/seqlock.h>
> +# include <linux/math64.h>
> +#endif
> +
> +
> +#ifdef __KERNEL__
> +
> +extern struct timezone sys_tz;
> +
> +
> static inline int timespec_equal(const struct timespec *a,
> const struct timespec *b)
> {
> diff --git a/include/linux/timer.h b/include/linux/timer.h
> index 6abd913..25bd863 100644
> --- a/include/linux/timer.h
> +++ b/include/linux/timer.h
> @@ -2,9 +2,7 @@
> #define _LINUX_TIMER_H
>
> #include <linux/list.h>
> -#include <linux/ktime.h>
> #include <linux/stddef.h>
> -#include <linux/debugobjects.h>
> #include <linux/stringify.h>
>
> struct tvec_base;
> @@ -33,6 +31,8 @@ struct timer_list {
> #endif
> };
>
> +#include <linux/ktime.h>
> +#include <linux/debugobjects.h>
> extern struct tvec_base boot_tvec_bases;
>
> #ifdef CONFIG_LOCKDEP
> diff --git a/include/linux/timex.h b/include/linux/timex.h
> index aa60fe7..91ef0fa 100644
> --- a/include/linux/timex.h
> +++ b/include/linux/timex.h
> @@ -53,6 +53,12 @@
> #ifndef _LINUX_TIMEX_H
> #define _LINUX_TIMEX_H
>
> +#ifdef __KERNEL__
> +/* The clock frequency of the i8253/i8254 PIT */
> +#define PIT_TICK_RATE 1193182ul
> +#include <asm/timex.h>
> +#endif /* __KERNEL__ */
> +
> #include <linux/time.h>
>
> #define NTP_API 4 /* NTP API version */
> @@ -171,8 +177,6 @@ struct timex {
> #include <linux/types.h>
> #include <linux/param.h>
>
> -#include <asm/timex.h>
> -
> /*
> * SHIFT_PLL is used as a dampening factor to define how much we
> * adjust the frequency correction for a given offset in PLL mode.
> @@ -273,9 +277,6 @@ extern void hardpps(const struct timespec *, const struct timespec *);
>
> int read_current_timer(unsigned long *timer_val);
>
> -/* The clock frequency of the i8253/i8254 PIT */
> -#define PIT_TICK_RATE 1193182ul
> -
> #endif /* KERNEL */
>
> #endif /* LINUX_TIMEX_H */
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index eb8b9f1..ba1588e 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -397,6 +397,9 @@ extern bool workqueue_congested(unsigned int cpu, struct workqueue_struct *wq);
> extern unsigned int work_cpu(struct work_struct *work);
> extern unsigned int work_busy(struct work_struct *work);
>
> +extern int del_timer_sync(struct timer_list *timer);
> +extern int del_timer(struct timer_list *timer);
> +
> /*
> * Kill off a pending schedule_delayed_work(). Note that the work callback
> * function may still be running on return from cancel_delayed_work(), unless
>
On Sat, Feb 18, 2012 at 03:21:12PM -0800, Jeremy Fitzhardinge wrote:
> On 02/17/2012 12:25 AM, Raghavendra K T wrote:
> > From: Raghavendra K T <[email protected]>
> >
> > Changelog:
> > Reordering in header files and adding declarations to enable
> > spinlock header to use jump label technique.
> >
> > Signed-off-by: Raghavendra K T <[email protected]>
> > ---
> > I was re-basing Jermey patches (https://lkml.org/lkml/2011/10/12/496), while working
> > on paravirtualized ticket spinlock (3.3.-rc3).
> >
> > Currently <jump_label.h> includes <workqueue.h> (commit: b202952075f62603bea9bfb6ebc6b0420db11949)
> >
> > So we get following error when we try to include jump_label.h from
> > arch/x86/include/asm/spinlock.h because of cyclic dependency
> > <spinlock.h> -> <jumplabe.h> -> <workque.h> -> ... <seqlock.h> -> <spinlock.h>
>
> What about splitting the jump_label_key_deferred stuff into a separate
> jump_label_deferred.h, and just include that where it's needed?
>
Andrew Jones did exactly that (CCed). But does pvlock have to use jump
label? I looked at the code and it is used like paravirt patching. Meaning
it is patched only once on a boot up when XEN is detected. May be use
paravirt patching instead of jump label? What if jump label will want
to use spinlock for some reason in the future (it uses mutex currently)?
--
Gleb.
On 02/19/2012 01:24 AM, Gleb Natapov wrote:
> On Sat, Feb 18, 2012 at 03:21:12PM -0800, Jeremy Fitzhardinge wrote:
>> On 02/17/2012 12:25 AM, Raghavendra K T wrote:
>>> From: Raghavendra K T <[email protected]>
>>>
>>> Changelog:
>>> Reordering in header files and adding declarations to enable
>>> spinlock header to use jump label technique.
>>>
>>> Signed-off-by: Raghavendra K T <[email protected]>
>>> ---
>>> I was re-basing Jermey patches (https://lkml.org/lkml/2011/10/12/496), while working
>>> on paravirtualized ticket spinlock (3.3.-rc3).
>>>
>>> Currently <jump_label.h> includes <workqueue.h> (commit: b202952075f62603bea9bfb6ebc6b0420db11949)
>>>
>>> So we get following error when we try to include jump_label.h from
>>> arch/x86/include/asm/spinlock.h because of cyclic dependency
>>> <spinlock.h> -> <jumplabe.h> -> <workque.h> -> ... <seqlock.h> -> <spinlock.h>
>> What about splitting the jump_label_key_deferred stuff into a separate
>> jump_label_deferred.h, and just include that where it's needed?
>>
> Andrew Jones did exactly that (CCed). But does pvlock have to use jump
> label? I looked at the code and it is used like paravirt patching. Meaning
> it is patched only once on a boot up when XEN is detected. May be use
> paravirt patching instead of jump label? What if jump label will want
> to use spinlock for some reason in the future (it uses mutex currently)?
The point of the pv ticketlocks is to avoid any pvop calls on the
lock/unlock fastpath, relegating them to only the slow path.
Unfortunately, the pv unlock case can't be identical with the non-pv
unlock, and jump_labels are lighter weight and more efficient than pvops.
It doesn't matter if jump_labels start using spinlocks; all we need the
jump_label machinery to do is patch the jump sites in the code so that
one of two execution paths can be selected. Since all the ticketlock
jump_label patching happens before SMP is enabled, there's no problem
with changing a lock while a cpu is executing the code.
J
On 02/20/2012 10:46 AM, Jeremy Fitzhardinge wrote:
[...]
>>>> So we get following error when we try to include jump_label.h from
>>>> arch/x86/include/asm/spinlock.h because of cyclic dependency
>>>> <spinlock.h> -> <jumplabe.h> -> <workque.h> -> ...<seqlock.h> -> <spinlock.h>
>>> What about splitting the jump_label_key_deferred stuff into a separate
>>> jump_label_deferred.h, and just include that where it's needed?
>>>
>> Andrew Jones did exactly that (CCed).
Sorry, did not get it. Tried to search the patch. Is it similar
work or same work?. Could you please point. shall try both the way
(current way and jump_label_deferred way). So whichever makes maintainer
happy, let that go :)
But does pvlock have to use jump
>> label? I looked at the code and it is used like paravirt patching. Meaning
>> it is patched only once on a boot up when XEN is detected. May be use
>> paravirt patching instead of jump label? What if jump label will want
>> to use spinlock for some reason in the future (it uses mutex currently)?
>
> The point of the pv ticketlocks is to avoid any pvop calls on the
> lock/unlock fastpath, relegating them to only the slow path.
> Unfortunately, the pv unlock case can't be identical with the non-pv
> unlock, and jump_labels are lighter weight and more efficient than pvops.
>
> It doesn't matter if jump_labels start using spinlocks; all we need the
> jump_label machinery to do is patch the jump sites in the code so that
> one of two execution paths can be selected. Since all the ticketlock
> jump_label patching happens before SMP is enabled, there's no problem
> with changing a lock while a cpu is executing the code.
>
I also felt agreeing with Jeremy. seemed to me that latter is more
performance friendly. no?.
(Hmm. Thinking.. By the way is it not that Jeremy's earlier version
had implementation similar to what Gleb asked ?)
On Mon, Feb 20, 2012 at 11:44:25AM +0530, Raghavendra K T wrote:
> On 02/20/2012 10:46 AM, Jeremy Fitzhardinge wrote:
> [...]
> >>>> So we get following error when we try to include jump_label.h from
> >>>>arch/x86/include/asm/spinlock.h because of cyclic dependency
> >>>><spinlock.h> -> <jumplabe.h> -> <workque.h> -> ...<seqlock.h> -> <spinlock.h>
> >>>What about splitting the jump_label_key_deferred stuff into a separate
> >>>jump_label_deferred.h, and just include that where it's needed?
> >>>
> >>Andrew Jones did exactly that (CCed).
>
> Sorry, did not get it. Tried to search the patch. Is it similar
> work or same work?. Could you please point. shall try both the way
> (current way and jump_label_deferred way). So whichever makes
> maintainer happy, let that go :)
>
It was not CCed to any ML. I CCed Andrew so he can chime in.
> But does pvlock have to use jump
> >>label? I looked at the code and it is used like paravirt patching. Meaning
> >>it is patched only once on a boot up when XEN is detected. May be use
> >>paravirt patching instead of jump label? What if jump label will want
> >>to use spinlock for some reason in the future (it uses mutex currently)?
> >
> >The point of the pv ticketlocks is to avoid any pvop calls on the
> >lock/unlock fastpath, relegating them to only the slow path.
> >Unfortunately, the pv unlock case can't be identical with the non-pv
> >unlock, and jump_labels are lighter weight and more efficient than pvops.
> >
> >It doesn't matter if jump_labels start using spinlocks; all we need the
> >jump_label machinery to do is patch the jump sites in the code so that
> >one of two execution paths can be selected. Since all the ticketlock
> >jump_label patching happens before SMP is enabled, there's no problem
> >with changing a lock while a cpu is executing the code.
> >
>
> I also felt agreeing with Jeremy. seemed to me that latter is more
> performance friendly. no?.
>
I thought not about pvop, but about alternative(). jump_labels is used
by spinlock to patch out jump into nops It can be done via alternative()
too I think.
> (Hmm. Thinking.. By the way is it not that Jeremy's earlier version
> had implementation similar to what Gleb asked ?)
--
Gleb.
----- Original Message -----
> On Mon, Feb 20, 2012 at 11:44:25AM +0530, Raghavendra K T wrote:
> > On 02/20/2012 10:46 AM, Jeremy Fitzhardinge wrote:
> > [...]
> > >>>> So we get following error when we try to include jump_label.h
> > >>>> from
> > >>>>arch/x86/include/asm/spinlock.h because of cyclic dependency
> > >>>><spinlock.h> -> <jumplabe.h> -> <workque.h> ->
> > >>>> ...<seqlock.h> -> <spinlock.h>
> > >>>What about splitting the jump_label_key_deferred stuff into a
> > >>>separate
> > >>>jump_label_deferred.h, and just include that where it's needed?
> > >>>
> > >>Andrew Jones did exactly that (CCed).
> >
> > Sorry, did not get it. Tried to search the patch. Is it similar
> > work or same work?. Could you please point. shall try both the way
> > (current way and jump_label_deferred way). So whichever makes
> > maintainer happy, let that go :)
> >
> It was not CCed to any ML. I CCed Andrew so he can chime in.
Hi Raghavendra,
sorry I didn't get around to starting this discussion myself when
I first touched the issue. I also bumped into it when attempting
to rebase pvticketlocks, so I pinged Gleb with the idea to split
the header. As he's said again now though, he wasn't sure if jump
labels were necessary in the pvticketlock implementation. In the
meantime I got busy with other stuff, and didn't get a chance to
continue/expand the discussion.
I actually think we could consider both of the issues separately
though. Should we split jump_label.h to make sure we can include
it where it could otherwise get the cyclic dependencies due to
workqueue.h? The only argument I can see against that is that
jumplabels may change again, and then we may hit another issue,
then again, etc., but handling them like this case by case isn't
very clean. Perhaps we need jump_label.h to define a "minimal
jump label", and then we can create an "extended jump label",
which has rate limiting and other capabilities.
Drew
On 02/20/2012 08:30 PM, Andrew Jones wrote:
>
>
> ----- Original Message -----
>> On Mon, Feb 20, 2012 at 11:44:25AM +0530, Raghavendra K T wrote:
>>> On 02/20/2012 10:46 AM, Jeremy Fitzhardinge wrote:
>>> [...]
Hi Drew,
Thanks for the reply
> Hi Raghavendra,
> sorry I didn't get around to starting this discussion myself when
> I first touched the issue. I also bumped into it when attempting
> to rebase pvticketlocks, so I pinged Gleb with the idea to split
> the header. As he's said again now though, he wasn't sure if jump
> labels were necessary in the pvticketlock implementation. In the
> meantime I got busy with other stuff, and didn't get a chance to
> continue/expand the discussion.
>
> I actually think we could consider both of the issues separately
> though.
Right.
Should we split jump_label.h to make sure we can include
> it where it could otherwise get the cyclic dependencies due to
> workqueue.h? The only argument I can see against that is that
> jumplabels may change again, and then we may hit another issue,
> then again, etc., but handling them like this case by case isn't
> very clean.
Hmm..Handling the way it is handled by me sometime looks ugly to me.
The patch I have posted is only important set of changes. But for
complete yesconfig to work, it still would require change something like
below I am attaching, which (needs 24 file changes) makes it more ugly.
Only point is I can continue testing paravirt spinlock on tip.
Perhaps we need jump_label.h to define a "minimal
> jump label", and then we can create an "extended jump label",
> which has rate limiting and other capabilities.
>
I completely agree. seeing that, you have not started that, it seems
it's good idea for me to take a look at that option, (but may be at
slower pace considering some changes I require (TODO) for kvm paravirt
spinlock).
> Drew
>
>
--->8---
---
On Mon, Feb 20, 2012 at 11:21:16PM +0530, Raghavendra K T wrote:
> Perhaps we need jump_label.h to define a "minimal
> >jump label", and then we can create an "extended jump label",
> >which has rate limiting and other capabilities.
> >
>
> I completely agree. seeing that, you have not started that, it seems
> it's good idea for me to take a look at that option, (but may be at
> slower pace considering some changes I require (TODO) for kvm paravirt
> spinlock).
>
Below is the patch I wrote when I first proposed the idea to
Gleb. This patch isn't exactly what I stated above, because
it's splitting original and ratelimit, rather than minimal and
extended. Maybe this is sufficient for now? Or maybe we don't
want to split at all?
Drew
---
From: Andrew Jones <[email protected]>
Date: Thu, 26 Jan 2012 13:59:30 +0100
Subject: [PATCH] split out rate limiting from jump_label.h
Commit b202952075f62603bea9bfb6ebc6b0420db11949 introduced rate limiting
for jump label disabling. The changes were made in the jump label code
in order to be more widely available and to keep things tidier. This is
all fine, except now jump_label.h includes linux/workqueue.h, which
makes it impossible to include jump_label.h from anything that
workqueue.h needs. For example, it's now impossible to include
jump_label.h from asm/spinlock.h, which was done in Jeremy's proposed
pv-ticketlock patches. This patch splits out the rate limiting related
changes from jump_label.h into a new file, jump_label_ratelimit.h, to
resolve the issue.
Signed-off-by: Andrew Jones <[email protected]>
---
include/linux/jump_label.h | 24 ------------------------
include/linux/jump_label_ratelimit.h | 32 ++++++++++++++++++++++++++++++++
include/linux/perf_event.h | 2 +-
kernel/jump_label.c | 2 +-
4 files changed, 34 insertions(+), 26 deletions(-)
create mode 100644 include/linux/jump_label_ratelimit.h
diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
index 5ce8b14..b15954a 100644
--- a/include/linux/jump_label.h
+++ b/include/linux/jump_label.h
@@ -3,7 +3,6 @@
#include <linux/types.h>
#include <linux/compiler.h>
-#include <linux/workqueue.h>
#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
@@ -15,12 +14,6 @@ struct jump_label_key {
#endif
};
-struct jump_label_key_deferred {
- struct jump_label_key key;
- unsigned long timeout;
- struct delayed_work work;
-};
-
# include <asm/jump_label.h>
# define HAVE_JUMP_LABEL
#endif /* CC_HAVE_ASM_GOTO && CONFIG_JUMP_LABEL */
@@ -58,11 +51,8 @@ extern void arch_jump_label_transform_static(struct jump_entry *entry,
extern int jump_label_text_reserved(void *start, void *end);
extern void jump_label_inc(struct jump_label_key *key);
extern void jump_label_dec(struct jump_label_key *key);
-extern void jump_label_dec_deferred(struct jump_label_key_deferred *key);
extern bool jump_label_enabled(struct jump_label_key *key);
extern void jump_label_apply_nops(struct module *mod);
-extern void jump_label_rate_limit(struct jump_label_key_deferred *key,
- unsigned long rl);
#else /* !HAVE_JUMP_LABEL */
@@ -78,10 +68,6 @@ static __always_inline void jump_label_init(void)
{
}
-struct jump_label_key_deferred {
- struct jump_label_key key;
-};
-
static __always_inline bool static_branch(struct jump_label_key *key)
{
if (unlikely(atomic_read(&key->enabled)))
@@ -99,11 +85,6 @@ static inline void jump_label_dec(struct jump_label_key *key)
atomic_dec(&key->enabled);
}
-static inline void jump_label_dec_deferred(struct jump_label_key_deferred *key)
-{
- jump_label_dec(&key->key);
-}
-
static inline int jump_label_text_reserved(void *start, void *end)
{
return 0;
@@ -121,11 +102,6 @@ static inline int jump_label_apply_nops(struct module *mod)
{
return 0;
}
-
-static inline void jump_label_rate_limit(struct jump_label_key_deferred *key,
- unsigned long rl)
-{
-}
#endif /* HAVE_JUMP_LABEL */
#define jump_label_key_enabled ((struct jump_label_key){ .enabled = ATOMIC_INIT(1), })
diff --git a/include/linux/jump_label_ratelimit.h b/include/linux/jump_label_ratelimit.h
new file mode 100644
index 0000000..2cf61b9
--- /dev/null
+++ b/include/linux/jump_label_ratelimit.h
@@ -0,0 +1,32 @@
+#ifndef _LINUX_JUMP_LABEL_RATELIMIT_H
+#define _LINUX_JUMP_LABEL_RATELIMIT_H
+
+#include <linux/jump_label.h>
+#include <linux/workqueue.h>
+
+#if defined(CC_HAVE_ASM_GOTO) && defined(CONFIG_JUMP_LABEL)
+struct jump_label_key_deferred {
+ struct jump_label_key key;
+ unsigned long timeout;
+ struct delayed_work work;
+};
+#endif
+
+#ifdef HAVE_JUMP_LABEL
+extern void jump_label_dec_deferred(struct jump_label_key_deferred *key);
+extern void jump_label_rate_limit(struct jump_label_key_deferred *key,
+ unsigned long rl);
+#else
+struct jump_label_key_deferred {
+ struct jump_label_key key;
+};
+static inline void jump_label_dec_deferred(struct jump_label_key_deferred *key)
+{
+ jump_label_dec(&key->key);
+}
+static inline void jump_label_rate_limit(struct jump_label_key_deferred *key,
+ unsigned long rl)
+{
+}
+#endif
+#endif /* _LINUX_JUMP_LABEL_RATELIMIT_H */
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 0885561..7ae33d9 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -512,7 +512,7 @@ struct perf_guest_info_callbacks {
#include <linux/ftrace.h>
#include <linux/cpu.h>
#include <linux/irq_work.h>
-#include <linux/jump_label.h>
+#include <linux/jump_label_ratelimit.h>
#include <linux/atomic.h>
#include <asm/local.h>
diff --git a/kernel/jump_label.c b/kernel/jump_label.c
index 01d3b70..f736308 100644
--- a/kernel/jump_label.c
+++ b/kernel/jump_label.c
@@ -12,7 +12,7 @@
#include <linux/slab.h>
#include <linux/sort.h>
#include <linux/err.h>
-#include <linux/jump_label.h>
+#include <linux/jump_label_ratelimit.h>
#ifdef HAVE_JUMP_LABEL
--
1.7.7.6
On 02/20/2012 03:03 PM, Gleb Natapov wrote:
> On Mon, Feb 20, 2012 at 11:44:25AM +0530, Raghavendra K T wrote:
>> On 02/20/2012 10:46 AM, Jeremy Fitzhardinge wrote:
[...]
>> But does pvlock have to use jump
>>>> label? I looked at the code and it is used like paravirt patching. Meaning
>>>> it is patched only once on a boot up when XEN is detected. May be use
>>>> paravirt patching instead of jump label? What if jump label will want
>>>> to use spinlock for some reason in the future (it uses mutex currently)?
>>>
>>> The point of the pv ticketlocks is to avoid any pvop calls on the
>>> lock/unlock fastpath, relegating them to only the slow path.
>>> Unfortunately, the pv unlock case can't be identical with the non-pv
>>> unlock, and jump_labels are lighter weight and more efficient than pvops.
>>>
>>> It doesn't matter if jump_labels start using spinlocks; all we need the
>>> jump_label machinery to do is patch the jump sites in the code so that
>>> one of two execution paths can be selected. Since all the ticketlock
>>> jump_label patching happens before SMP is enabled, there's no problem
>>> with changing a lock while a cpu is executing the code.
>>>
>>
>> I also felt agreeing with Jeremy. seemed to me that latter is more
>> performance friendly. no?.
>>
>
> I thought not about pvop, but about alternative(). jump_labels is used
> by spinlock to patch out jump into nops It can be done via alternative()
> too I think.
I had remembered that this discussion already happened with Jeremy's V5
of ticketlock patches. pulling out link :
https://lkml.org/lkml/2011/10/13/384
On 02/21/2012 08:53 PM, Andrew Jones wrote:
[Including people in perf_event and jumplabel also]
> On Mon, Feb 20, 2012 at 11:21:16PM +0530, Raghavendra K T wrote:
>
> Below is the patch I wrote when I first proposed the idea to
> Gleb. This patch isn't exactly what I stated above, because
> it's splitting original and ratelimit, rather than minimal and
> extended. Maybe this is sufficient for now? Or maybe we don't
> want to split at all?
>
IMHO, This patch is sufficient and makes lot more sense than my patches.
I verified and compile tested this with rebased patches of Jeremy
(yesconfig).
Can add
Reviewed-By: Raghavendra K T<[email protected]>
Peter, Jason, Ingo, please let's know if you have any objection with
below patch..
> Drew
>
> ---
>
> From: Andrew Jones<[email protected]>
> Date: Thu, 26 Jan 2012 13:59:30 +0100
> Subject: [PATCH] split out rate limiting from jump_label.h
>
> Commit b202952075f62603bea9bfb6ebc6b0420db11949 introduced rate limiting
> for jump label disabling. The changes were made in the jump label code
> in order to be more widely available and to keep things tidier. This is
> all fine, except now jump_label.h includes linux/workqueue.h, which
> makes it impossible to include jump_label.h from anything that
> workqueue.h needs. For example, it's now impossible to include
> jump_label.h from asm/spinlock.h, which was done in Jeremy's proposed
> pv-ticketlock patches. This patch splits out the rate limiting related
> changes from jump_label.h into a new file, jump_label_ratelimit.h, to
> resolve the issue.
>
> Signed-off-by: Andrew Jones<[email protected]>
> ---
> include/linux/jump_label.h | 24 ------------------------
> include/linux/jump_label_ratelimit.h | 32 ++++++++++++++++++++++++++++++++
> include/linux/perf_event.h | 2 +-
> kernel/jump_label.c | 2 +-
> 4 files changed, 34 insertions(+), 26 deletions(-)
> create mode 100644 include/linux/jump_label_ratelimit.h
>
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index 5ce8b14..b15954a 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -3,7 +3,6 @@
>
> #include<linux/types.h>
> #include<linux/compiler.h>
> -#include<linux/workqueue.h>
>
> #if defined(CC_HAVE_ASM_GOTO)&& defined(CONFIG_JUMP_LABEL)
>
> @@ -15,12 +14,6 @@ struct jump_label_key {
> #endif
> };
>
> -struct jump_label_key_deferred {
> - struct jump_label_key key;
> - unsigned long timeout;
> - struct delayed_work work;
> -};
> -
> # include<asm/jump_label.h>
> # define HAVE_JUMP_LABEL
> #endif /* CC_HAVE_ASM_GOTO&& CONFIG_JUMP_LABEL */
> @@ -58,11 +51,8 @@ extern void arch_jump_label_transform_static(struct jump_entry *entry,
> extern int jump_label_text_reserved(void *start, void *end);
> extern void jump_label_inc(struct jump_label_key *key);
> extern void jump_label_dec(struct jump_label_key *key);
> -extern void jump_label_dec_deferred(struct jump_label_key_deferred *key);
> extern bool jump_label_enabled(struct jump_label_key *key);
> extern void jump_label_apply_nops(struct module *mod);
> -extern void jump_label_rate_limit(struct jump_label_key_deferred *key,
> - unsigned long rl);
>
> #else /* !HAVE_JUMP_LABEL */
>
> @@ -78,10 +68,6 @@ static __always_inline void jump_label_init(void)
> {
> }
>
> -struct jump_label_key_deferred {
> - struct jump_label_key key;
> -};
> -
> static __always_inline bool static_branch(struct jump_label_key *key)
> {
> if (unlikely(atomic_read(&key->enabled)))
> @@ -99,11 +85,6 @@ static inline void jump_label_dec(struct jump_label_key *key)
> atomic_dec(&key->enabled);
> }
>
> -static inline void jump_label_dec_deferred(struct jump_label_key_deferred *key)
> -{
> - jump_label_dec(&key->key);
> -}
> -
> static inline int jump_label_text_reserved(void *start, void *end)
> {
> return 0;
> @@ -121,11 +102,6 @@ static inline int jump_label_apply_nops(struct module *mod)
> {
> return 0;
> }
> -
> -static inline void jump_label_rate_limit(struct jump_label_key_deferred *key,
> - unsigned long rl)
> -{
> -}
> #endif /* HAVE_JUMP_LABEL */
>
> #define jump_label_key_enabled ((struct jump_label_key){ .enabled = ATOMIC_INIT(1), })
> diff --git a/include/linux/jump_label_ratelimit.h b/include/linux/jump_label_ratelimit.h
> new file mode 100644
> index 0000000..2cf61b9
> --- /dev/null
> +++ b/include/linux/jump_label_ratelimit.h
> @@ -0,0 +1,32 @@
> +#ifndef _LINUX_JUMP_LABEL_RATELIMIT_H
> +#define _LINUX_JUMP_LABEL_RATELIMIT_H
> +
> +#include<linux/jump_label.h>
> +#include<linux/workqueue.h>
> +
> +#if defined(CC_HAVE_ASM_GOTO)&& defined(CONFIG_JUMP_LABEL)
> +struct jump_label_key_deferred {
> + struct jump_label_key key;
> + unsigned long timeout;
> + struct delayed_work work;
> +};
> +#endif
> +
> +#ifdef HAVE_JUMP_LABEL
> +extern void jump_label_dec_deferred(struct jump_label_key_deferred *key);
> +extern void jump_label_rate_limit(struct jump_label_key_deferred *key,
> + unsigned long rl);
> +#else
> +struct jump_label_key_deferred {
> + struct jump_label_key key;
> +};
> +static inline void jump_label_dec_deferred(struct jump_label_key_deferred *key)
> +{
> + jump_label_dec(&key->key);
> +}
> +static inline void jump_label_rate_limit(struct jump_label_key_deferred *key,
> + unsigned long rl)
> +{
> +}
> +#endif
> +#endif /* _LINUX_JUMP_LABEL_RATELIMIT_H */
> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index 0885561..7ae33d9 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -512,7 +512,7 @@ struct perf_guest_info_callbacks {
> #include<linux/ftrace.h>
> #include<linux/cpu.h>
> #include<linux/irq_work.h>
> -#include<linux/jump_label.h>
> +#include<linux/jump_label_ratelimit.h>
> #include<linux/atomic.h>
> #include<asm/local.h>
>
> diff --git a/kernel/jump_label.c b/kernel/jump_label.c
> index 01d3b70..f736308 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -12,7 +12,7 @@
> #include<linux/slab.h>
> #include<linux/sort.h>
> #include<linux/err.h>
> -#include<linux/jump_label.h>
> +#include<linux/jump_label_ratelimit.h>
>
> #ifdef HAVE_JUMP_LABEL
>