Subject: Re: [RFC PATCHv2 1/3] hrtimer: Add notifer for clock_was_set

From: Daniel Borkmann <[email-masked]>
Date: Wed, 07 Aug 2013 11:35:02 +0200

On 08/07/2013 11:31 AM, Fan Du wrote:
> On 2013年08月07日 17:20, Daniel Borkmann wrote:
>> On 08/07/2013 11:04 AM, Fan Du wrote:
>>> When clock_was_set is called in case of system wall time change
>>> or host resume from suspend state, use this notifier for places
>>> where interested in this action.
>>
>> (Only minor commenting on this one ...)
>>
>>> Signed-off-by: Fan Du <[email-masked]>
>>> ---
>>> kernel/hrtimer.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
>>> index 383319b..b7c62a9 100644
>>> --- a/kernel/hrtimer.c
>>> +++ b/kernel/hrtimer.c
>>> @@ -755,6 +755,9 @@ static inline void retrigger_next_event(void *arg) { }
>>>
>>> #endif /* CONFIG_HIGH_RES_TIMERS */
>>>
>>> +ATOMIC_NOTIFIER_HEAD(clock_change_notifier_list);
>>> +EXPORT_SYMBOL(clock_change_notifier_list);
>>
>> This should be static and hidden from other modules, e.g. have a look at
>> netevent_notif_chain (net/core/netevent.c).
>>
>> Instead, this should be accessed via registration/un-registration handlers
>> for notifier blocks, and those can then be exported as EXPORT_SYMBOL_GPL
>> as this is core area.
>>
>>> +
>>> /*
>>> * Clock realtime was set
>>> *
>>> @@ -773,6 +776,7 @@ void clock_was_set(void)
>>> on_each_cpu(retrigger_next_event, NULL, 1);
>>> #endif
>>> timerfd_clock_was_set();
>>> + atomic_notifier_call_chain(&clock_change_notifier_list, 0, 0);
>>
>> Also here a small one-line handler call_clock_change_notifiers() would be
>> better.
>
> Thanks for your attention.
> After taking a look at netevent_notif_chain, you mean I should do it in below style:
>
> static ATOMIC_NOTIFIER_HEAD(clock_change_notifier_list);
>
> int un/register_clock_change_notifier(struct notifier_block *nb)
> {
>          int err;
>
>          err = atomic_notifier_chain_un/register(&clock_change_notifier_list, nb);
>          return err;

return atomic_notifier_chain_un/register(&clock_change_notifier_list, nb);

> }
> EXPORT_SYMBOL_GPL(clock_change_notifier_list);
>
> int call_clock_change_notifiers(unsigned long val, void *v)
> {
>          return atomic_notifier_call_chain(&clock_change_notifier, val, v);
> }
> EXPORT_SYMBOL_GPL(call_clock_change_notifiers);
>
> Will do it in next version after others comment rest of patches.

Yes, sounds good to me (you might also want to cc Thomas Gleixner on this one).