Re: [Xen-devel] [PATCH 2/3] timekeeping: Fix potential lost pv notification of time change

[John Stultz <john.stultz@xxxxxxxxxx>]

On 12/18/2013 10:44 AM, John Stultz wrote:
> On 12/18/2013 02:08 AM, Ingo Molnar wrote:
>> * John Stultz <john.stultz@xxxxxxxxxx> wrote:
>>
>>> In 780427f0e11 (Indicate that clock was set in the pvclock
>>> gtod notifier), logic was added to pass a CLOCK_WAS_SET
>>> notification to the pvclock notifier chain.
>>>
>>> While that patch added a action flag returned from
>>> accumulate_nsecs_to_secs(), it only uses the returned value
>>> in one location, and not in the logarithmic accumulation.
>>>
>>> This means if a leap second triggered during the logarithmic
>>> accumulation (which is most likely where it would happen),
>>> the notification that the clock was set would not make it to
>>> the pv notifiers.
>>>
>>> This patch extends the logarithmic_accumulation pass down
>>> that action flag so proper notification will occur.
>>>
>>> Cc: Sasha Levin <sasha.levin@xxxxxxxxxx>
>>> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>>> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
>>> Cc: David Vrabel <david.vrabel@xxxxxxxxxx>
>>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>
>>> Cc: Prarit Bhargava <prarit@xxxxxxxxxx>
>>> Cc: Richard Cochran <richardcochran@xxxxxxxxx>
>>> Cc: <xen-devel@xxxxxxxxxxxxx>
>>> Cc: stable <stable@xxxxxxxxxxxxxxx> #3.11+
>>> Signed-off-by: John Stultz <john.stultz@xxxxxxxxxx>
>>> ---
>>>  kernel/time/timekeeping.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
>>> index 6bad3d9..998ec751 100644
>>> --- a/kernel/time/timekeeping.c
>>> +++ b/kernel/time/timekeeping.c
>>> @@ -1295,7 +1295,7 @@ static inline unsigned int 
>>> accumulate_nsecs_to_secs(struct timekeeper *tk)
>>>   * Returns the unconsumed cycles.
>>>   */
>>>  static cycle_t logarithmic_accumulation(struct timekeeper *tk, cycle_t 
>>> offset,
>>> -                                           u32 shift)
>>> +                                           u32 shift, unsigned int *action)
>> I have two complaints about this patch:
>>
>> 1)
>>
>> I think the 'action' name sucks because it's too obfuscated. It's only 
>> ever set to TK_CLOCK_WAS_SET, so why not name it more descriptively, 
>> i.e. 'clock_was_set'?
> Sure, I was reusing the existing variables, but no issue changing the
> name here too.
>
>
>> 2)
>>
>> Secondly, the proliferation of parameters passed around I think calls 
>> for a helper structure which would carry the (offset, shift, 
>> clock_was_set) triple:
>>
>>      struct acc_params {
>>              cycle_t         offset;
>>              u32             shift;
>>              bool            clock_was_set;
>>      };
>>
>> And then passed down like this:
>>
>>>  static cycle_t logarithmic_accumulation(struct timekeeper *tk, struct 
>>> acc_params *params)
>> Agreed?
> Huh. Ok, I don't see the parameters structure likely being reused, so
> this would be a special struct only for the logarithmic_accumulation() call?

The other downside to this is it really doesn't improve the read-ability
of the update_wall_clock function, since
we end either end up changing all the offset, shift, action references
to param.* (which besides being a bit ugly, feels like way too much
change for an urgent patch). The alternative is to only pack the
structure right before we call which is ok, but seems like extra overhead.

Also passing the structure to the function makes it a little more
difficult to understand, as you lose the clarity of which values are
input-only and which values are modified or set by the function.

I do agree that some of recent changes (particularly the
shadow-timekeeping logic) has caused the code to feel a bit hairy and
has caused a number of these warts where we're passing extra arguments.
I've not had the time to take a deep look and see how things could be
re-factored to be a bit cleaner. But the code definitely needs some cleanup.


I'll re-factor the action->clock_was_set bit for this (urgent) patch
series, but I may want to push back on trying to do the logarithmic
accumulation parameters structure change for 3.14.

thanks
-john


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel