* [PATCH] x86: Reduce duplicated code in the x86_64 context switch path.
@ 2013-05-13 5:14 Joe Damato
2013-05-13 6:03 ` H. Peter Anvin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Joe Damato @ 2013-05-13 5:14 UTC (permalink / raw)
To: x86, linux-kernel; +Cc: mingo, hpa, akpm, Joe Damato
Signed-off-by: Joe Damato <ice799@gmail.com>
---
arch/x86/include/asm/switch_to.h | 30 ++++++++++++++++++++++++++++++
arch/x86/kernel/process_64.c | 29 ++---------------------------
2 files changed, 32 insertions(+), 27 deletions(-)
diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index 4ec45b3..a322cc6 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -124,6 +124,36 @@ do { \
__switch_canary_iparam \
: "memory", "cc" __EXTRA_CLOBBER)
+#define loadsegment_fs(fs, index) \
+ loadsegment(fs, index)
+
+#define loadsegment_gs(gs, index) \
+ load_gs_index(index)
+
+#define switch_segment(prev, next, index, seg, msr) \
+ do { \
+ /* \
+ * Segment register != 0 always requires a reload. Also \
+ * reload when it has changed. When prev process used 64bit \
+ * base always reload to avoid an information leak. \
+ */ \
+ if (unlikely(index | next->index | prev->seg)) { \
+ loadsegment_##seg(seg, next->index); \
+ /* \
+ * Check if the user used a selector != 0; if yes \
+ * clear 64bit base, since overloaded base is always \
+ * mapped to the Null selector \
+ */ \
+ if (index) \
+ prev->seg = 0; \
+ } \
+ \
+ /* when next process has a 64bit base use it */ \
+ if (next->seg) \
+ wrmsrl(msr, next->seg); \
+ prev->index = index; \
+ } while (0)
+
#endif /* CONFIG_X86_32 */
#endif /* _ASM_X86_SWITCH_TO_H */
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 355ae06..f41d026 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -318,34 +318,9 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
/*
* Switch FS and GS.
- *
- * Segment register != 0 always requires a reload. Also
- * reload when it has changed. When prev process used 64bit
- * base always reload to avoid an information leak.
*/
- if (unlikely(fsindex | next->fsindex | prev->fs)) {
- loadsegment(fs, next->fsindex);
- /*
- * Check if the user used a selector != 0; if yes
- * clear 64bit base, since overloaded base is always
- * mapped to the Null selector
- */
- if (fsindex)
- prev->fs = 0;
- }
- /* when next process has a 64bit base use it */
- if (next->fs)
- wrmsrl(MSR_FS_BASE, next->fs);
- prev->fsindex = fsindex;
-
- if (unlikely(gsindex | next->gsindex | prev->gs)) {
- load_gs_index(next->gsindex);
- if (gsindex)
- prev->gs = 0;
- }
- if (next->gs)
- wrmsrl(MSR_KERNEL_GS_BASE, next->gs);
- prev->gsindex = gsindex;
+ switch_segment(prev, next, fsindex, fs, MSR_FS_BASE);
+ switch_segment(prev, next, gsindex, gs, MSR_KERNEL_GS_BASE);
switch_fpu_finish(next_p, fpu);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Reduce duplicated code in the x86_64 context switch path.
2013-05-13 5:14 [PATCH] x86: Reduce duplicated code in the x86_64 context switch path Joe Damato
@ 2013-05-13 6:03 ` H. Peter Anvin
2013-05-13 7:26 ` Joe Damato
2013-05-13 7:20 ` [PATCH v2] " Joe Damato
2013-05-13 16:31 ` [PATCH] " Guenter Roeck
2 siblings, 1 reply; 6+ messages in thread
From: H. Peter Anvin @ 2013-05-13 6:03 UTC (permalink / raw)
To: Joe Damato, x86, linux-kernel; +Cc: mingo, akpm
This is a net addition in code, and send to only make it harder to understand what is happening. As such I don't think this is a good idea.
Joe Damato <ice799@gmail.com> wrote:
>Signed-off-by: Joe Damato <ice799@gmail.com>
>---
> arch/x86/include/asm/switch_to.h | 30 ++++++++++++++++++++++++++++++
> arch/x86/kernel/process_64.c | 29 ++---------------------------
> 2 files changed, 32 insertions(+), 27 deletions(-)
>
>diff --git a/arch/x86/include/asm/switch_to.h
>b/arch/x86/include/asm/switch_to.h
>index 4ec45b3..a322cc6 100644
>--- a/arch/x86/include/asm/switch_to.h
>+++ b/arch/x86/include/asm/switch_to.h
>@@ -124,6 +124,36 @@ do { \
> __switch_canary_iparam \
> : "memory", "cc" __EXTRA_CLOBBER)
>
>+#define loadsegment_fs(fs, index) \
>+ loadsegment(fs, index)
>+
>+#define loadsegment_gs(gs, index) \
>+ load_gs_index(index)
>+
>+#define switch_segment(prev, next, index, seg, msr) \
>+ do { \
>+ /* \
>+ * Segment register != 0 always requires a reload. Also \
>+ * reload when it has changed. When prev process used 64bit \
>+ * base always reload to avoid an information leak. \
>+ */ \
>+ if (unlikely(index | next->index | prev->seg)) { \
>+ loadsegment_##seg(seg, next->index); \
>+ /* \
>+ * Check if the user used a selector != 0; if yes \
>+ * clear 64bit base, since overloaded base is always \
>+ * mapped to the Null selector \
>+ */ \
>+ if (index) \
>+ prev->seg = 0; \
>+ } \
>+ \
>+ /* when next process has a 64bit base use it */ \
>+ if (next->seg) \
>+ wrmsrl(msr, next->seg); \
>+ prev->index = index; \
>+ } while (0)
>+
> #endif /* CONFIG_X86_32 */
>
> #endif /* _ASM_X86_SWITCH_TO_H */
>diff --git a/arch/x86/kernel/process_64.c
>b/arch/x86/kernel/process_64.c
>index 355ae06..f41d026 100644
>--- a/arch/x86/kernel/process_64.c
>+++ b/arch/x86/kernel/process_64.c
>@@ -318,34 +318,9 @@ __switch_to(struct task_struct *prev_p, struct
>task_struct *next_p)
>
> /*
> * Switch FS and GS.
>- *
>- * Segment register != 0 always requires a reload. Also
>- * reload when it has changed. When prev process used 64bit
>- * base always reload to avoid an information leak.
> */
>- if (unlikely(fsindex | next->fsindex | prev->fs)) {
>- loadsegment(fs, next->fsindex);
>- /*
>- * Check if the user used a selector != 0; if yes
>- * clear 64bit base, since overloaded base is always
>- * mapped to the Null selector
>- */
>- if (fsindex)
>- prev->fs = 0;
>- }
>- /* when next process has a 64bit base use it */
>- if (next->fs)
>- wrmsrl(MSR_FS_BASE, next->fs);
>- prev->fsindex = fsindex;
>-
>- if (unlikely(gsindex | next->gsindex | prev->gs)) {
>- load_gs_index(next->gsindex);
>- if (gsindex)
>- prev->gs = 0;
>- }
>- if (next->gs)
>- wrmsrl(MSR_KERNEL_GS_BASE, next->gs);
>- prev->gsindex = gsindex;
>+ switch_segment(prev, next, fsindex, fs, MSR_FS_BASE);
>+ switch_segment(prev, next, gsindex, gs, MSR_KERNEL_GS_BASE);
>
> switch_fpu_finish(next_p, fpu);
>
--
Sent from my mobile phone. Please excuse brevity and lack of formatting.
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2] x86: Reduce duplicated code in the x86_64 context switch path.
2013-05-13 5:14 [PATCH] x86: Reduce duplicated code in the x86_64 context switch path Joe Damato
2013-05-13 6:03 ` H. Peter Anvin
@ 2013-05-13 7:20 ` Joe Damato
2013-05-13 16:31 ` [PATCH] " Guenter Roeck
2 siblings, 0 replies; 6+ messages in thread
From: Joe Damato @ 2013-05-13 7:20 UTC (permalink / raw)
To: x86, linux-kernel; +Cc: mingo, hpa, akpm, Joe Damato
Signed-off-by: Joe Damato <ice799@gmail.com>
---
arch/x86/include/asm/switch_to.h | 25 +++++++++++++++++++++++++
arch/x86/kernel/process_64.c | 29 ++---------------------------
2 files changed, 27 insertions(+), 27 deletions(-)
diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
index 4ec45b3..5fd0267 100644
--- a/arch/x86/include/asm/switch_to.h
+++ b/arch/x86/include/asm/switch_to.h
@@ -124,6 +124,31 @@ do { \
__switch_canary_iparam \
: "memory", "cc" __EXTRA_CLOBBER)
+#define load_fs_index(index) \
+ loadsegment(fs, index)
+
+#define switch_segment(prev, next, index, seg, msr) \
+ do { \
+ /* \
+ * Segment register != 0 always requires a reload. Also \
+ * reload when it has changed. When prev process used 64bit \
+ * base always reload to avoid an information leak. \
+ */ \
+ if (unlikely(index | next->index | prev->seg)) { \
+ load_##seg##_index(next->index); \
+ /* \
+ * Check if the user used a selector != 0; if yes \
+ * clear 64bit base, since overloaded base is always \
+ * mapped to the Null selector \
+ */ \
+ if (index) \
+ prev->seg = 0; \
+ } \
+ /* when next process has a 64bit base use it */ \
+ if (next->seg) \
+ wrmsrl(msr, next->seg); \
+ prev->index = index; \
+ } while (0)
#endif /* CONFIG_X86_32 */
#endif /* _ASM_X86_SWITCH_TO_H */
diff --git a/arch/x86/kernel/process_64.c b/arch/x86/kernel/process_64.c
index 355ae06..f41d026 100644
--- a/arch/x86/kernel/process_64.c
+++ b/arch/x86/kernel/process_64.c
@@ -318,34 +318,9 @@ __switch_to(struct task_struct *prev_p, struct task_struct *next_p)
/*
* Switch FS and GS.
- *
- * Segment register != 0 always requires a reload. Also
- * reload when it has changed. When prev process used 64bit
- * base always reload to avoid an information leak.
*/
- if (unlikely(fsindex | next->fsindex | prev->fs)) {
- loadsegment(fs, next->fsindex);
- /*
- * Check if the user used a selector != 0; if yes
- * clear 64bit base, since overloaded base is always
- * mapped to the Null selector
- */
- if (fsindex)
- prev->fs = 0;
- }
- /* when next process has a 64bit base use it */
- if (next->fs)
- wrmsrl(MSR_FS_BASE, next->fs);
- prev->fsindex = fsindex;
-
- if (unlikely(gsindex | next->gsindex | prev->gs)) {
- load_gs_index(next->gsindex);
- if (gsindex)
- prev->gs = 0;
- }
- if (next->gs)
- wrmsrl(MSR_KERNEL_GS_BASE, next->gs);
- prev->gsindex = gsindex;
+ switch_segment(prev, next, fsindex, fs, MSR_FS_BASE);
+ switch_segment(prev, next, gsindex, gs, MSR_KERNEL_GS_BASE);
switch_fpu_finish(next_p, fpu);
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Reduce duplicated code in the x86_64 context switch path.
2013-05-13 6:03 ` H. Peter Anvin
@ 2013-05-13 7:26 ` Joe Damato
0 siblings, 0 replies; 6+ messages in thread
From: Joe Damato @ 2013-05-13 7:26 UTC (permalink / raw)
To: H. Peter Anvin; +Cc: x86, LKML, Ingo Molnar, akpm
On Sun, May 12, 2013 at 11:03 PM, H. Peter Anvin <hpa@zytor.com> wrote:
> This is a net addition in code, and send to only make it harder to understand what is happening. As such I don't think this is a good idea.
OK, I tweaked it slightly to break even on lines of code sent a v2. I
do think it would be nice to remove the duplicated code and (IMHO) it
makes __switch_to easier to read.
> Joe Damato <ice799@gmail.com> wrote:
>
>>Signed-off-by: Joe Damato <ice799@gmail.com>
>>---
>> arch/x86/include/asm/switch_to.h | 30 ++++++++++++++++++++++++++++++
>> arch/x86/kernel/process_64.c | 29 ++---------------------------
>> 2 files changed, 32 insertions(+), 27 deletions(-)
>>
>>diff --git a/arch/x86/include/asm/switch_to.h
>>b/arch/x86/include/asm/switch_to.h
>>index 4ec45b3..a322cc6 100644
>>--- a/arch/x86/include/asm/switch_to.h
>>+++ b/arch/x86/include/asm/switch_to.h
>>@@ -124,6 +124,36 @@ do { \
>> __switch_canary_iparam \
>> : "memory", "cc" __EXTRA_CLOBBER)
>>
>>+#define loadsegment_fs(fs, index) \
>>+ loadsegment(fs, index)
>>+
>>+#define loadsegment_gs(gs, index) \
>>+ load_gs_index(index)
>>+
>>+#define switch_segment(prev, next, index, seg, msr) \
>>+ do { \
>>+ /* \
>>+ * Segment register != 0 always requires a reload. Also \
>>+ * reload when it has changed. When prev process used 64bit \
>>+ * base always reload to avoid an information leak. \
>>+ */ \
>>+ if (unlikely(index | next->index | prev->seg)) { \
>>+ loadsegment_##seg(seg, next->index); \
>>+ /* \
>>+ * Check if the user used a selector != 0; if yes \
>>+ * clear 64bit base, since overloaded base is always \
>>+ * mapped to the Null selector \
>>+ */ \
>>+ if (index) \
>>+ prev->seg = 0; \
>>+ } \
>>+ \
>>+ /* when next process has a 64bit base use it */ \
>>+ if (next->seg) \
>>+ wrmsrl(msr, next->seg); \
>>+ prev->index = index; \
>>+ } while (0)
>>+
>> #endif /* CONFIG_X86_32 */
>>
>> #endif /* _ASM_X86_SWITCH_TO_H */
>>diff --git a/arch/x86/kernel/process_64.c
>>b/arch/x86/kernel/process_64.c
>>index 355ae06..f41d026 100644
>>--- a/arch/x86/kernel/process_64.c
>>+++ b/arch/x86/kernel/process_64.c
>>@@ -318,34 +318,9 @@ __switch_to(struct task_struct *prev_p, struct
>>task_struct *next_p)
>>
>> /*
>> * Switch FS and GS.
>>- *
>>- * Segment register != 0 always requires a reload. Also
>>- * reload when it has changed. When prev process used 64bit
>>- * base always reload to avoid an information leak.
>> */
>>- if (unlikely(fsindex | next->fsindex | prev->fs)) {
>>- loadsegment(fs, next->fsindex);
>>- /*
>>- * Check if the user used a selector != 0; if yes
>>- * clear 64bit base, since overloaded base is always
>>- * mapped to the Null selector
>>- */
>>- if (fsindex)
>>- prev->fs = 0;
>>- }
>>- /* when next process has a 64bit base use it */
>>- if (next->fs)
>>- wrmsrl(MSR_FS_BASE, next->fs);
>>- prev->fsindex = fsindex;
>>-
>>- if (unlikely(gsindex | next->gsindex | prev->gs)) {
>>- load_gs_index(next->gsindex);
>>- if (gsindex)
>>- prev->gs = 0;
>>- }
>>- if (next->gs)
>>- wrmsrl(MSR_KERNEL_GS_BASE, next->gs);
>>- prev->gsindex = gsindex;
>>+ switch_segment(prev, next, fsindex, fs, MSR_FS_BASE);
>>+ switch_segment(prev, next, gsindex, gs, MSR_KERNEL_GS_BASE);
>>
>> switch_fpu_finish(next_p, fpu);
>>
>
> --
> Sent from my mobile phone. Please excuse brevity and lack of formatting.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Reduce duplicated code in the x86_64 context switch path.
2013-05-13 5:14 [PATCH] x86: Reduce duplicated code in the x86_64 context switch path Joe Damato
2013-05-13 6:03 ` H. Peter Anvin
2013-05-13 7:20 ` [PATCH v2] " Joe Damato
@ 2013-05-13 16:31 ` Guenter Roeck
2013-05-13 17:30 ` Ingo Molnar
2 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2013-05-13 16:31 UTC (permalink / raw)
To: Joe Damato; +Cc: x86, linux-kernel, mingo, hpa, akpm
On Sun, May 12, 2013 at 10:14:51PM -0700, Joe Damato wrote:
> Signed-off-by: Joe Damato <ice799@gmail.com>
> ---
> arch/x86/include/asm/switch_to.h | 30 ++++++++++++++++++++++++++++++
> arch/x86/kernel/process_64.c | 29 ++---------------------------
> 2 files changed, 32 insertions(+), 27 deletions(-)
>
> diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
> index 4ec45b3..a322cc6 100644
> --- a/arch/x86/include/asm/switch_to.h
> +++ b/arch/x86/include/asm/switch_to.h
> @@ -124,6 +124,36 @@ do { \
> __switch_canary_iparam \
> : "memory", "cc" __EXTRA_CLOBBER)
>
> +#define loadsegment_fs(fs, index) \
> + loadsegment(fs, index)
> +
> +#define loadsegment_gs(gs, index) \
> + load_gs_index(index)
> +
> +#define switch_segment(prev, next, index, seg, msr) \
> + do { \
> + /* \
> + * Segment register != 0 always requires a reload. Also \
> + * reload when it has changed. When prev process used 64bit \
> + * base always reload to avoid an information leak. \
> + */ \
> + if (unlikely(index | next->index | prev->seg)) { \
> + loadsegment_##seg(seg, next->index); \
> + /* \
> + * Check if the user used a selector != 0; if yes \
> + * clear 64bit base, since overloaded base is always \
> + * mapped to the Null selector \
> + */ \
> + if (index) \
> + prev->seg = 0; \
> + } \
> + \
> + /* when next process has a 64bit base use it */ \
> + if (next->seg) \
> + wrmsrl(msr, next->seg); \
> + prev->index = index; \
> + } while (0)
> +
> #endif /* CONFIG_X86_32 */
>
For my part I'll never understand how code written as macros is supposed to
improve anything. I always find it confusing and risky, as it is very easy
to introduce side effects. Also, while it may reduce the source code size,
it often results in increased object size.
My take: If you can not write it as inline function(s), don't bother.
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] x86: Reduce duplicated code in the x86_64 context switch path.
2013-05-13 16:31 ` [PATCH] " Guenter Roeck
@ 2013-05-13 17:30 ` Ingo Molnar
0 siblings, 0 replies; 6+ messages in thread
From: Ingo Molnar @ 2013-05-13 17:30 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Joe Damato, x86, linux-kernel, mingo, hpa, akpm
* Guenter Roeck <linux@roeck-us.net> wrote:
> On Sun, May 12, 2013 at 10:14:51PM -0700, Joe Damato wrote:
> > Signed-off-by: Joe Damato <ice799@gmail.com>
> > ---
> > arch/x86/include/asm/switch_to.h | 30 ++++++++++++++++++++++++++++++
> > arch/x86/kernel/process_64.c | 29 ++---------------------------
> > 2 files changed, 32 insertions(+), 27 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
> > index 4ec45b3..a322cc6 100644
> > --- a/arch/x86/include/asm/switch_to.h
> > +++ b/arch/x86/include/asm/switch_to.h
> > @@ -124,6 +124,36 @@ do { \
> > __switch_canary_iparam \
> > : "memory", "cc" __EXTRA_CLOBBER)
> >
> > +#define loadsegment_fs(fs, index) \
> > + loadsegment(fs, index)
> > +
> > +#define loadsegment_gs(gs, index) \
> > + load_gs_index(index)
> > +
> > +#define switch_segment(prev, next, index, seg, msr) \
> > + do { \
> > + /* \
> > + * Segment register != 0 always requires a reload. Also \
> > + * reload when it has changed. When prev process used 64bit \
> > + * base always reload to avoid an information leak. \
> > + */ \
> > + if (unlikely(index | next->index | prev->seg)) { \
> > + loadsegment_##seg(seg, next->index); \
> > + /* \
> > + * Check if the user used a selector != 0; if yes \
> > + * clear 64bit base, since overloaded base is always \
> > + * mapped to the Null selector \
> > + */ \
> > + if (index) \
> > + prev->seg = 0; \
> > + } \
> > + \
> > + /* when next process has a 64bit base use it */ \
> > + if (next->seg) \
> > + wrmsrl(msr, next->seg); \
> > + prev->index = index; \
> > + } while (0)
> > +
> > #endif /* CONFIG_X86_32 */
> >
> For my part I'll never understand how code written as macros is supposed to
> improve anything. I always find it confusing and risky, as it is very easy
> to introduce side effects. Also, while it may reduce the source code size,
> it often results in increased object size.
>
> My take: If you can not write it as inline function(s), don't bother.
Indeed.
Thanks,
Ingo
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-05-13 17:30 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-13 5:14 [PATCH] x86: Reduce duplicated code in the x86_64 context switch path Joe Damato
2013-05-13 6:03 ` H. Peter Anvin
2013-05-13 7:26 ` Joe Damato
2013-05-13 7:20 ` [PATCH v2] " Joe Damato
2013-05-13 16:31 ` [PATCH] " Guenter Roeck
2013-05-13 17:30 ` Ingo Molnar
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox