* [PATCH] pvclock: introduce seqcount-like API
@ 2016-06-09 11:23 Paolo Bonzini
2016-06-09 12:43 ` Roman Kagan
2016-06-12 10:25 ` Minfei Huang
0 siblings, 2 replies; 10+ messages in thread
From: Paolo Bonzini @ 2016-06-09 11:23 UTC (permalink / raw)
To: linux-kernel, kvm; +Cc: mnghuan, luto, rkagan
The version field in struct pvclock_vcpu_time_info basically implements
a seqcount. Wrap it with the usual read_begin and read_retry functions,
and use these APIs instead of peppering the code with smp_rmb()s.
While at it, change it to the more pedantically correct virt_rmb().
With this change, __pvclock_read_cycles can be simplified noticeably.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
arch/x86/entry/vdso/vclock_gettime.c | 25 +++++------------------
arch/x86/include/asm/pvclock.h | 39 +++++++++++++++++++++---------------
arch/x86/kernel/pvclock.c | 17 ++++++----------
3 files changed, 34 insertions(+), 47 deletions(-)
diff --git a/arch/x86/entry/vdso/vclock_gettime.c b/arch/x86/entry/vdso/vclock_gettime.c
index 2f02d23a05ef..94d54d0defa7 100644
--- a/arch/x86/entry/vdso/vclock_gettime.c
+++ b/arch/x86/entry/vdso/vclock_gettime.c
@@ -96,9 +96,8 @@ static notrace cycle_t vread_pvclock(int *mode)
{
const struct pvclock_vcpu_time_info *pvti = &get_pvti0()->pvti;
cycle_t ret;
- u64 tsc, pvti_tsc;
- u64 last, delta, pvti_system_time;
- u32 version, pvti_tsc_to_system_mul, pvti_tsc_shift;
+ u64 last;
+ u32 version;
/*
* Note: The kernel and hypervisor must guarantee that cpu ID
@@ -123,29 +122,15 @@ static notrace cycle_t vread_pvclock(int *mode)
*/
do {
- version = pvti->version;
-
- smp_rmb();
+ version = pvclock_read_begin(pvti);
if (unlikely(!(pvti->flags & PVCLOCK_TSC_STABLE_BIT))) {
*mode = VCLOCK_NONE;
return 0;
}
- tsc = rdtsc_ordered();
- pvti_tsc_to_system_mul = pvti->tsc_to_system_mul;
- pvti_tsc_shift = pvti->tsc_shift;
- pvti_system_time = pvti->system_time;
- pvti_tsc = pvti->tsc_timestamp;
-
- /* Make sure that the version double-check is last. */
- smp_rmb();
- } while (unlikely((version & 1) || version != pvti->version));
-
- delta = tsc - pvti_tsc;
- ret = pvti_system_time +
- pvclock_scale_delta(delta, pvti_tsc_to_system_mul,
- pvti_tsc_shift);
+ ret = __pvclock_read_cycles(pvti);
+ } while (pvclock_read_retry(pvti, version));
/* refer to vread_tsc() comment for rationale */
last = gtod->cycle_last;
diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 7c1c89598688..0ee92db1e9f3 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -25,6 +25,24 @@ void pvclock_resume(void);
void pvclock_touch_watchdogs(void);
+static __always_inline
+unsigned pvclock_read_begin(const struct pvclock_vcpu_time_info *src)
+{
+ unsigned version = src->version & ~1;
+ /* Make sure that the version is read before the data. */
+ virt_rmb();
+ return version;
+}
+
+static __always_inline
+bool pvclock_read_retry(const struct pvclock_vcpu_time_info *src,
+ unsigned version)
+{
+ /* Make sure that the version is re-read after the data. */
+ virt_rmb();
+ return version != src->version;
+}
+
/*
* Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
* yielding a 64-bit result.
@@ -69,23 +87,12 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, int shift)
}
static __always_inline
-unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
- cycle_t *cycles, u8 *flags)
+cycle_t __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src)
{
- unsigned version;
- cycle_t offset;
- u64 delta;
-
- version = src->version;
- /* Make the latest version visible */
- smp_rmb();
-
- delta = rdtsc_ordered() - src->tsc_timestamp;
- offset = pvclock_scale_delta(delta, src->tsc_to_system_mul,
- src->tsc_shift);
- *cycles = src->system_time + offset;
- *flags = src->flags;
- return version;
+ u64 delta = rdtsc_ordered() - src->tsc_timestamp;
+ cycle_t offset = pvclock_scale_delta(delta, src->tsc_to_system_mul,
+ src->tsc_shift);
+ return src->system_time + offset;
}
struct pvclock_vsyscall_time_info {
diff --git a/arch/x86/kernel/pvclock.c b/arch/x86/kernel/pvclock.c
index 06c58ce46762..3599404e3089 100644
--- a/arch/x86/kernel/pvclock.c
+++ b/arch/x86/kernel/pvclock.c
@@ -64,14 +64,9 @@ u8 pvclock_read_flags(struct pvclock_vcpu_time_info *src)
u8 flags;
do {
- version = src->version;
- /* Make the latest version visible */
- smp_rmb();
-
+ version = pvclock_read_begin(src);
flags = src->flags;
- /* Make sure that the version double-check is last. */
- smp_rmb();
- } while ((src->version & 1) || version != src->version);
+ } while (pvclock_read_retry(src, version));
return flags & valid_flags;
}
@@ -84,10 +79,10 @@ cycle_t pvclock_clocksource_read(struct pvclock_vcpu_time_info *src)
u8 flags;
do {
- version = __pvclock_read_cycles(src, &ret, &flags);
- /* Make sure that the version double-check is last. */
- smp_rmb();
- } while ((src->version & 1) || version != src->version);
+ version = pvclock_read_begin(src);
+ ret = __pvclock_read_cycles(src);
+ flags = src->flags;
+ } while (pvclock_read_retry(src, version));
if (unlikely((flags & PVCLOCK_GUEST_STOPPED) != 0)) {
src->flags &= ~PVCLOCK_GUEST_STOPPED;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] pvclock: introduce seqcount-like API
2016-06-09 11:23 [PATCH] pvclock: introduce seqcount-like API Paolo Bonzini
@ 2016-06-09 12:43 ` Roman Kagan
2016-06-09 12:47 ` Paolo Bonzini
2016-06-12 10:25 ` Minfei Huang
1 sibling, 1 reply; 10+ messages in thread
From: Roman Kagan @ 2016-06-09 12:43 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm, mnghuan, luto
On Thu, Jun 09, 2016 at 01:23:23PM +0200, Paolo Bonzini wrote:
> The version field in struct pvclock_vcpu_time_info basically implements
> a seqcount. Wrap it with the usual read_begin and read_retry functions,
> and use these APIs instead of peppering the code with smp_rmb()s.
> While at it, change it to the more pedantically correct virt_rmb().
>
> With this change, __pvclock_read_cycles can be simplified noticeably.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/entry/vdso/vclock_gettime.c | 25 +++++------------------
> arch/x86/include/asm/pvclock.h | 39 +++++++++++++++++++++---------------
> arch/x86/kernel/pvclock.c | 17 ++++++----------
> 3 files changed, 34 insertions(+), 47 deletions(-)
[...]
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
[...]
> @@ -69,23 +87,12 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, int shift)
> }
>
> static __always_inline
> -unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
> - cycle_t *cycles, u8 *flags)
> +cycle_t __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src)
> {
> - unsigned version;
> - cycle_t offset;
> - u64 delta;
> -
> - version = src->version;
> - /* Make the latest version visible */
> - smp_rmb();
This is on top of Minfei's patch, right? It isn't in Linus' tree yet so
I wonder if it makes sense to merge the two patches into one.
Will you post it to stable, too?
Anyway
Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
Roman.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pvclock: introduce seqcount-like API
2016-06-09 12:43 ` Roman Kagan
@ 2016-06-09 12:47 ` Paolo Bonzini
2016-06-09 13:35 ` Roman Kagan
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2016-06-09 12:47 UTC (permalink / raw)
To: Roman Kagan, linux-kernel, kvm, mnghuan, luto
On 09/06/2016 14:43, Roman Kagan wrote:
> On Thu, Jun 09, 2016 at 01:23:23PM +0200, Paolo Bonzini wrote:
>> The version field in struct pvclock_vcpu_time_info basically implements
>> a seqcount. Wrap it with the usual read_begin and read_retry functions,
>> and use these APIs instead of peppering the code with smp_rmb()s.
>> While at it, change it to the more pedantically correct virt_rmb().
>>
>> With this change, __pvclock_read_cycles can be simplified noticeably.
>>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>> arch/x86/entry/vdso/vclock_gettime.c | 25 +++++------------------
>> arch/x86/include/asm/pvclock.h | 39 +++++++++++++++++++++---------------
>> arch/x86/kernel/pvclock.c | 17 ++++++----------
>> 3 files changed, 34 insertions(+), 47 deletions(-)
> [...]
>> --- a/arch/x86/include/asm/pvclock.h
>> +++ b/arch/x86/include/asm/pvclock.h
> [...]
>> @@ -69,23 +87,12 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, int shift)
>> }
>>
>> static __always_inline
>> -unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
>> - cycle_t *cycles, u8 *flags)
>> +cycle_t __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src)
>> {
>> - unsigned version;
>> - cycle_t offset;
>> - u64 delta;
>> -
>> - version = src->version;
>> - /* Make the latest version visible */
>> - smp_rmb();
>
> This is on top of Minfei's patch, right? It isn't in Linus' tree yet so
> I wonder if it makes sense to merge the two patches into one.
>
> Will you post it to stable, too?
Not this one, because Minfei's patch is enough to fix the bug, but I do
plan on including it in 4.7 to simplify the merging (kvm/next has
already been branched off Linus's tree).
Paolo
> Anyway
>
> Reviewed-by: Roman Kagan <rkagan@virtuozzo.com>
>
> Roman.
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pvclock: introduce seqcount-like API
2016-06-09 12:47 ` Paolo Bonzini
@ 2016-06-09 13:35 ` Roman Kagan
2016-06-09 13:45 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Roman Kagan @ 2016-06-09 13:35 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm, mnghuan, luto
On Thu, Jun 09, 2016 at 02:47:54PM +0200, Paolo Bonzini wrote:
> On 09/06/2016 14:43, Roman Kagan wrote:
> > On Thu, Jun 09, 2016 at 01:23:23PM +0200, Paolo Bonzini wrote:
> >> The version field in struct pvclock_vcpu_time_info basically implements
> >> a seqcount. Wrap it with the usual read_begin and read_retry functions,
> >> and use these APIs instead of peppering the code with smp_rmb()s.
> >> While at it, change it to the more pedantically correct virt_rmb().
> >>
> >> With this change, __pvclock_read_cycles can be simplified noticeably.
> >>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> >> ---
> >> arch/x86/entry/vdso/vclock_gettime.c | 25 +++++------------------
> >> arch/x86/include/asm/pvclock.h | 39 +++++++++++++++++++++---------------
> >> arch/x86/kernel/pvclock.c | 17 ++++++----------
> >> 3 files changed, 34 insertions(+), 47 deletions(-)
> > [...]
> >> --- a/arch/x86/include/asm/pvclock.h
> >> +++ b/arch/x86/include/asm/pvclock.h
> > [...]
> >> @@ -69,23 +87,12 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, int shift)
> >> }
> >>
> >> static __always_inline
> >> -unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
> >> - cycle_t *cycles, u8 *flags)
> >> +cycle_t __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src)
> >> {
> >> - unsigned version;
> >> - cycle_t offset;
> >> - u64 delta;
> >> -
> >> - version = src->version;
> >> - /* Make the latest version visible */
> >> - smp_rmb();
> >
> > This is on top of Minfei's patch, right? It isn't in Linus' tree yet so
> > I wonder if it makes sense to merge the two patches into one.
> >
> > Will you post it to stable, too?
>
> Not this one, because Minfei's patch is enough to fix the bug, but I do
Has it landed in any public tree? I'm unable to find any. There
appears to be another version of the patch on the list, so I'm confused.
Roman.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pvclock: introduce seqcount-like API
2016-06-09 13:35 ` Roman Kagan
@ 2016-06-09 13:45 ` Paolo Bonzini
2016-06-09 17:12 ` Andy Lutomirski
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2016-06-09 13:45 UTC (permalink / raw)
To: Roman Kagan, linux-kernel, kvm, mnghuan, luto
On 09/06/2016 15:35, Roman Kagan wrote:
> On Thu, Jun 09, 2016 at 02:47:54PM +0200, Paolo Bonzini wrote:
>> On 09/06/2016 14:43, Roman Kagan wrote:
>>> On Thu, Jun 09, 2016 at 01:23:23PM +0200, Paolo Bonzini wrote:
>>>> The version field in struct pvclock_vcpu_time_info basically implements
>>>> a seqcount. Wrap it with the usual read_begin and read_retry functions,
>>>> and use these APIs instead of peppering the code with smp_rmb()s.
>>>> While at it, change it to the more pedantically correct virt_rmb().
>>>>
>>>> With this change, __pvclock_read_cycles can be simplified noticeably.
>>>>
>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>> ---
>>>> arch/x86/entry/vdso/vclock_gettime.c | 25 +++++------------------
>>>> arch/x86/include/asm/pvclock.h | 39 +++++++++++++++++++++---------------
>>>> arch/x86/kernel/pvclock.c | 17 ++++++----------
>>>> 3 files changed, 34 insertions(+), 47 deletions(-)
>>> [...]
>>>> --- a/arch/x86/include/asm/pvclock.h
>>>> +++ b/arch/x86/include/asm/pvclock.h
>>> [...]
>>>> @@ -69,23 +87,12 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, int shift)
>>>> }
>>>>
>>>> static __always_inline
>>>> -unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
>>>> - cycle_t *cycles, u8 *flags)
>>>> +cycle_t __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src)
>>>> {
>>>> - unsigned version;
>>>> - cycle_t offset;
>>>> - u64 delta;
>>>> -
>>>> - version = src->version;
>>>> - /* Make the latest version visible */
>>>> - smp_rmb();
>>>
>>> This is on top of Minfei's patch, right? It isn't in Linus' tree yet so
>>> I wonder if it makes sense to merge the two patches into one.
>>>
>>> Will you post it to stable, too?
>>
>> Not this one, because Minfei's patch is enough to fix the bug, but I do
>
> Has it landed in any public tree? I'm unable to find any. There
> appears to be another version of the patch on the list, so I'm confused.
I'm about to push it to kvm/master.
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pvclock: introduce seqcount-like API
2016-06-09 13:45 ` Paolo Bonzini
@ 2016-06-09 17:12 ` Andy Lutomirski
2016-06-09 18:03 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Andy Lutomirski @ 2016-06-09 17:12 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Roman Kagan, linux-kernel, kvm list, Minfei Huang, Andrew Lutomirski
On Thu, Jun 9, 2016 at 6:45 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 09/06/2016 15:35, Roman Kagan wrote:
>> On Thu, Jun 09, 2016 at 02:47:54PM +0200, Paolo Bonzini wrote:
>>> On 09/06/2016 14:43, Roman Kagan wrote:
>>>> On Thu, Jun 09, 2016 at 01:23:23PM +0200, Paolo Bonzini wrote:
>>>>> The version field in struct pvclock_vcpu_time_info basically implements
>>>>> a seqcount. Wrap it with the usual read_begin and read_retry functions,
>>>>> and use these APIs instead of peppering the code with smp_rmb()s.
>>>>> While at it, change it to the more pedantically correct virt_rmb().
>>>>>
>>>>> With this change, __pvclock_read_cycles can be simplified noticeably.
>>>>>
>>>>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>>>>> ---
>>>>> arch/x86/entry/vdso/vclock_gettime.c | 25 +++++------------------
>>>>> arch/x86/include/asm/pvclock.h | 39 +++++++++++++++++++++---------------
>>>>> arch/x86/kernel/pvclock.c | 17 ++++++----------
>>>>> 3 files changed, 34 insertions(+), 47 deletions(-)
>>>> [...]
>>>>> --- a/arch/x86/include/asm/pvclock.h
>>>>> +++ b/arch/x86/include/asm/pvclock.h
>>>> [...]
>>>>> @@ -69,23 +87,12 @@ static inline u64 pvclock_scale_delta(u64 delta, u32 mul_frac, int shift)
>>>>> }
>>>>>
>>>>> static __always_inline
>>>>> -unsigned __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src,
>>>>> - cycle_t *cycles, u8 *flags)
>>>>> +cycle_t __pvclock_read_cycles(const struct pvclock_vcpu_time_info *src)
>>>>> {
>>>>> - unsigned version;
>>>>> - cycle_t offset;
>>>>> - u64 delta;
>>>>> -
>>>>> - version = src->version;
>>>>> - /* Make the latest version visible */
>>>>> - smp_rmb();
>>>>
>>>> This is on top of Minfei's patch, right? It isn't in Linus' tree yet so
>>>> I wonder if it makes sense to merge the two patches into one.
>>>>
>>>> Will you post it to stable, too?
>>>
>>> Not this one, because Minfei's patch is enough to fix the bug, but I do
>>
>> Has it landed in any public tree? I'm unable to find any. There
>> appears to be another version of the patch on the list, so I'm confused.
>
> I'm about to push it to kvm/master.
>
Sorry for being slow. I'm catching up. In its current form, I don't
like this patch. Please don't apply it.
The problem is that this makes two significant changes at once:
1. Use the new version helpers. I like that change.
2. Use __pvclock_read_cycles. That should be separate, and it should
come with timing numbers in the changelog.
--Andy
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pvclock: introduce seqcount-like API
2016-06-09 17:12 ` Andy Lutomirski
@ 2016-06-09 18:03 ` Paolo Bonzini
2016-06-09 18:08 ` Andy Lutomirski
0 siblings, 1 reply; 10+ messages in thread
From: Paolo Bonzini @ 2016-06-09 18:03 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Roman Kagan, linux-kernel, kvm list, Minfei Huang, Andrew Lutomirski
On 09/06/2016 19:12, Andy Lutomirski wrote:
> On Thu, Jun 9, 2016 at 6:45 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 09/06/2016 15:35, Roman Kagan wrote:
>>> On Thu, Jun 09, 2016 at 02:47:54PM +0200, Paolo Bonzini wrote:
>>>> On 09/06/2016 14:43, Roman Kagan wrote:
>>> Has it landed in any public tree? I'm unable to find any. There
>>> appears to be another version of the patch on the list, so I'm confused.
>>
>> I'm about to push it to kvm/master.
>
> Sorry for being slow. I'm catching up. In its current form, I don't
> like this patch. Please don't apply it.
Sure, I was talking about Minfei's patches, not this one. :) Of course
I need ack for this one.
> The problem is that this makes two significant changes at once:
>
> 1. Use the new version helpers. I like that change.
>
> 2. Use __pvclock_read_cycles. That should be separate, and it should
> come with timing numbers in the changelog.
__pvclock_read_cycles is pretty much the same as the code that is being
inlined. Thus the only change is that __pvclock_read_cycles is called
inside the loop rather than outside, but the loop really is expected to
never roll so why make a copy in the first place?
I'll split the patch anyway, thanks!
Paolo
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pvclock: introduce seqcount-like API
2016-06-09 18:03 ` Paolo Bonzini
@ 2016-06-09 18:08 ` Andy Lutomirski
2016-06-15 12:17 ` Paolo Bonzini
0 siblings, 1 reply; 10+ messages in thread
From: Andy Lutomirski @ 2016-06-09 18:08 UTC (permalink / raw)
To: Paolo Bonzini
Cc: Roman Kagan, linux-kernel, kvm list, Minfei Huang, Andrew Lutomirski
On Thu, Jun 9, 2016 at 11:03 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 09/06/2016 19:12, Andy Lutomirski wrote:
>> On Thu, Jun 9, 2016 at 6:45 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> On 09/06/2016 15:35, Roman Kagan wrote:
>>>> On Thu, Jun 09, 2016 at 02:47:54PM +0200, Paolo Bonzini wrote:
>>>>> On 09/06/2016 14:43, Roman Kagan wrote:
>>>> Has it landed in any public tree? I'm unable to find any. There
>>>> appears to be another version of the patch on the list, so I'm confused.
>>>
>>> I'm about to push it to kvm/master.
>>
>> Sorry for being slow. I'm catching up. In its current form, I don't
>> like this patch. Please don't apply it.
>
> Sure, I was talking about Minfei's patches, not this one. :) Of course
> I need ack for this one.
>
>> The problem is that this makes two significant changes at once:
>>
>> 1. Use the new version helpers. I like that change.
>>
>> 2. Use __pvclock_read_cycles. That should be separate, and it should
>> come with timing numbers in the changelog.
>
> __pvclock_read_cycles is pretty much the same as the code that is being
> inlined. Thus the only change is that __pvclock_read_cycles is called
> inside the loop rather than outside, but the loop really is expected to
> never roll so why make a copy in the first place?
I feel like I had a reason, but I don't remember what it was.
--Andy
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pvclock: introduce seqcount-like API
2016-06-09 11:23 [PATCH] pvclock: introduce seqcount-like API Paolo Bonzini
2016-06-09 12:43 ` Roman Kagan
@ 2016-06-12 10:25 ` Minfei Huang
1 sibling, 0 replies; 10+ messages in thread
From: Minfei Huang @ 2016-06-12 10:25 UTC (permalink / raw)
To: Paolo Bonzini; +Cc: linux-kernel, kvm, luto, rkagan
On 06/09/16 at 01:23P, Paolo Bonzini wrote:
> The version field in struct pvclock_vcpu_time_info basically implements
> a seqcount. Wrap it with the usual read_begin and read_retry functions,
> and use these APIs instead of peppering the code with smp_rmb()s.
> While at it, change it to the more pedantically correct virt_rmb().
>
> With this change, __pvclock_read_cycles can be simplified noticeably.
Hi, Paolo.
Thanks for accepting my patches in your repo.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
> arch/x86/entry/vdso/vclock_gettime.c | 25 +++++------------------
> arch/x86/include/asm/pvclock.h | 39 +++++++++++++++++++++---------------
> arch/x86/kernel/pvclock.c | 17 ++++++----------
> 3 files changed, 34 insertions(+), 47 deletions(-)
>
> diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
> index 7c1c89598688..0ee92db1e9f3 100644
> --- a/arch/x86/include/asm/pvclock.h
> +++ b/arch/x86/include/asm/pvclock.h
> @@ -25,6 +25,24 @@ void pvclock_resume(void);
>
> void pvclock_touch_watchdogs(void);
>
> +static __always_inline
> +unsigned pvclock_read_begin(const struct pvclock_vcpu_time_info *src)
It's better to use type unsigned int, instead of unsigned which is
complained by script checkpatch.
Thanks
Minfei
> +{
> + unsigned version = src->version & ~1;
Ditto.
> + /* Make sure that the version is read before the data. */
> + virt_rmb();
> + return version;
> +}
> +
> +static __always_inline
> +bool pvclock_read_retry(const struct pvclock_vcpu_time_info *src,
> + unsigned version)
Ditto.
Thanks
Minfei
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] pvclock: introduce seqcount-like API
2016-06-09 18:08 ` Andy Lutomirski
@ 2016-06-15 12:17 ` Paolo Bonzini
0 siblings, 0 replies; 10+ messages in thread
From: Paolo Bonzini @ 2016-06-15 12:17 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Roman Kagan, linux-kernel, kvm list, Minfei Huang, Andrew Lutomirski
On 09/06/2016 20:08, Andy Lutomirski wrote:
> > __pvclock_read_cycles is pretty much the same as the code that is being
> > inlined. Thus the only change is that __pvclock_read_cycles is called
> > inside the loop rather than outside, but the loop really is expected to
> > never roll so why make a copy in the first place?
>
> I feel like I had a reason, but I don't remember what it was.
I cannot see any substantial difference in the generated code with
this patch. On the other hand, if I add this:
diff --git a/arch/x86/include/asm/pvclock.h b/arch/x86/include/asm/pvclock.h
index 0ee92db1e9f3..d019f0cc80ec 100644
--- a/arch/x86/include/asm/pvclock.h
+++ b/arch/x86/include/asm/pvclock.h
@@ -40,7 +40,7 @@ bool pvclock_read_retry(const struct pvclock_vcpu_time_info *src,
{
/* Make sure that the version is re-read after the data. */
virt_rmb();
- return version != src->version;
+ return unlikely(version != src->version);
}
/*
then vread_pvclock is inlined everywhere, which bloats the code somewhat (+80
code bytes). Do you want me to mark vread_pvclock as noinline too?
Paolo
^ permalink raw reply related [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-06-15 12:17 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-09 11:23 [PATCH] pvclock: introduce seqcount-like API Paolo Bonzini
2016-06-09 12:43 ` Roman Kagan
2016-06-09 12:47 ` Paolo Bonzini
2016-06-09 13:35 ` Roman Kagan
2016-06-09 13:45 ` Paolo Bonzini
2016-06-09 17:12 ` Andy Lutomirski
2016-06-09 18:03 ` Paolo Bonzini
2016-06-09 18:08 ` Andy Lutomirski
2016-06-15 12:17 ` Paolo Bonzini
2016-06-12 10:25 ` Minfei Huang
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).