* PARAVIRT_SAVE_FLAGS_IRQ_DISABLE composite callsite
@ 2007-02-25 9:40 Jeremy Fitzhardinge
2007-02-25 22:07 ` Rusty Russell
0 siblings, 1 reply; 6+ messages in thread
From: Jeremy Fitzhardinge @ 2007-02-25 9:40 UTC (permalink / raw)
To: Rusty Russell, Zachary Amsden, Chris Wright; +Cc: Virtualization Mailing List
What was the point of this again? Was it that these two operations are
used so commonly together that its worth having a special type for them,
or is there some correctness issue here?
It seems to me that having it adds a fair amount of fiddley complexity,
and it doesn't gain very much because patching will make each operation
individually fairly efficient.
J
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PARAVIRT_SAVE_FLAGS_IRQ_DISABLE composite callsite
2007-02-25 9:40 PARAVIRT_SAVE_FLAGS_IRQ_DISABLE composite callsite Jeremy Fitzhardinge
@ 2007-02-25 22:07 ` Rusty Russell
2007-02-26 0:51 ` Jeremy Fitzhardinge
0 siblings, 1 reply; 6+ messages in thread
From: Rusty Russell @ 2007-02-25 22:07 UTC (permalink / raw)
To: Jeremy Fitzhardinge; +Cc: Chris Wright, Virtualization Mailing List
On Sun, 2007-02-25 at 01:40 -0800, Jeremy Fitzhardinge wrote:
> What was the point of this again? Was it that these two operations are
> used so commonly together that its worth having a special type for them,
> or is there some correctness issue here?
>
> It seems to me that having it adds a fair amount of fiddley complexity,
> and it doesn't gain very much because patching will make each operation
> individually fairly efficient.
Yes, the combo was *so* common we went for one patch site rather than
two.
Rusty.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PARAVIRT_SAVE_FLAGS_IRQ_DISABLE composite callsite
2007-02-25 22:07 ` Rusty Russell
@ 2007-02-26 0:51 ` Jeremy Fitzhardinge
2007-02-26 2:43 ` Rusty Russell
0 siblings, 1 reply; 6+ messages in thread
From: Jeremy Fitzhardinge @ 2007-02-26 0:51 UTC (permalink / raw)
To: Rusty Russell; +Cc: Chris Wright, Virtualization Mailing List
Rusty Russell wrote:
> Yes, the combo was *so* common we went for one patch site rather than
> two.
>
Is there any disadvantage to having two adjacent patch sites? For
native the same instruction sequence will be generated, but with a few
more nops.
J
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PARAVIRT_SAVE_FLAGS_IRQ_DISABLE composite callsite
2007-02-26 0:51 ` Jeremy Fitzhardinge
@ 2007-02-26 2:43 ` Rusty Russell
2007-02-26 5:49 ` Jeremy Fitzhardinge
0 siblings, 1 reply; 6+ messages in thread
From: Rusty Russell @ 2007-02-26 2:43 UTC (permalink / raw)
To: Jeremy Fitzhardinge; +Cc: Chris Wright, Virtualization Mailing List
On Sun, 2007-02-25 at 16:51 -0800, Jeremy Fitzhardinge wrote:
> Rusty Russell wrote:
> > Yes, the combo was *so* common we went for one patch site rather than
> > two.
> >
>
> Is there any disadvantage to having two adjacent patch sites? For
> native the same instruction sequence will be generated, but with a few
> more nops.
Originally it was just 24 bytes vs 12 bytes, it's probably less now.
But as I said, it's *really* popular. I don't have the numbers on me,
but it's almost worth making it the default and implementing cli / sti
in terms of save & restore 8)
Rusty.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: PARAVIRT_SAVE_FLAGS_IRQ_DISABLE composite callsite
2007-02-26 2:43 ` Rusty Russell
@ 2007-02-26 5:49 ` Jeremy Fitzhardinge
2007-02-26 23:28 ` Rusty Russell
0 siblings, 1 reply; 6+ messages in thread
From: Jeremy Fitzhardinge @ 2007-02-26 5:49 UTC (permalink / raw)
To: Rusty Russell; +Cc: Chris Wright, Virtualization Mailing List
Rusty Russell wrote:
> Originally it was just 24 bytes vs 12 bytes, it's probably less now.
> But as I said, it's *really* popular. I don't have the numbers on me,
> but it's almost worth making it the default and implementing cli / sti
> in terms of save & restore 8)
Do you mean statically or dynamically popular. I did a quick look at
the static usage in a PAE kernel:
(site id -> count)
1391 patch sites:
82 -> 126 - pmd_val
29 -> 117 - restore_fl
91 -> 103 - save_fl+irq_disable
57 -> 97 - apic_write
28 -> 90 - save_fl
35 -> 85 - read_msr
...
so you're right, safe_fl+irq_disable is very common in the static
count. Each safe_fl+irq_disable site is 18 bytes vs 10 each for save_fl
and irq_disable on their own, so not fusing them would cost about 200
bytes of kernel text. If we set the clobber for irq_disable to none,
then there would be no need to explicitly save/restore %eax over it.
The upside for removing it would be:
* simpler having a 1:1 relationship between pv_ops functions and
call site types
* better patched code for less work - separate callsites can be
patched by the generic patcher, but the combined callsite type
will always need special casing
Well, that's it really, simpler. I just had a bug as a result of its
special type, and from the comments in vmi.c it doesn't seem too popular
over there either.
J
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: PARAVIRT_SAVE_FLAGS_IRQ_DISABLE composite callsite
2007-02-26 5:49 ` Jeremy Fitzhardinge
@ 2007-02-26 23:28 ` Rusty Russell
0 siblings, 0 replies; 6+ messages in thread
From: Rusty Russell @ 2007-02-26 23:28 UTC (permalink / raw)
To: Jeremy Fitzhardinge; +Cc: Chris Wright, Virtualization Mailing List
On Sun, 2007-02-25 at 21:49 -0800, Jeremy Fitzhardinge wrote:
> Rusty Russell wrote:
> > Originally it was just 24 bytes vs 12 bytes, it's probably less now.
> > But as I said, it's *really* popular. I don't have the numbers on me,
> > but it's almost worth making it the default and implementing cli / sti
> > in terms of save & restore 8)
>
> Do you mean statically or dynamically popular.
Both 8(
Undo it and see if you can measure it on lmbench?
Rusty.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2007-02-26 23:28 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-02-25 9:40 PARAVIRT_SAVE_FLAGS_IRQ_DISABLE composite callsite Jeremy Fitzhardinge
2007-02-25 22:07 ` Rusty Russell
2007-02-26 0:51 ` Jeremy Fitzhardinge
2007-02-26 2:43 ` Rusty Russell
2007-02-26 5:49 ` Jeremy Fitzhardinge
2007-02-26 23:28 ` Rusty Russell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).