[RFC PATCHv2 0/3] xfrm: Refactor xfrm_state timer management

[RFC PATCHv2 0/3] xfrm: Refactor xfrm_state timer management

[Fan Du]

The first version of "refactor xfrm_state timer management" has been
flushed into toilet since nobody but only me like it.

Anyway new approach here is updating SAs lifetime timeout whenever
clock_was_set is called, iow, system clock changed or host resume from
suspend state. Rule is simple, force soft expire for any SAs which has
not reach their soft expire limit and hard expire for those has experienced
soft expire timeout but wait for hard expire timeout to come.(If I undestand
Dave advice clearly)

Locking issue:
 - holding rtnl_lock when iterate on all net namespace.
 - holding xfrm_state_lock when iterate all xfrm_state in this net.
 - holding state->lock when changing xfrm_state.

I'm not aware of any other locks other than above ones, so if I'm missing
something obviously, please tell me.

Thanks!

Fan Du (3):
  hrtimer: Add notifer for clock_was_set
  xfrm: Update xfrm_state lifetime expire after clock_was_set
  xfrm: Revert "Fix unexpected SA hard expiration after changing date"

 include/net/xfrm.h    |    4 ----
 kernel/hrtimer.c      |    4 ++++
 net/xfrm/xfrm_state.c |   61 +++++++++++++++++++++++++++++++++++--------------
 3 files changed, 48 insertions(+), 21 deletions(-)

-- 
1.7.9.5

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

[Fan Du]

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.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 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);
+
 /*
  * 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);
 }
 
 /*
-- 
1.7.9.5

[RFC PATCHv2 2/3] xfrm: Update xfrm_state lifetime expire after clock_was_set

[Fan Du]

After clock_was_set called to set new time or host resume from suspend
state. Notify IKED with soft timeout for SAs which haven't reach its
soft timeout limit. For those dying SAs, arrange them to hard expire.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 net/xfrm/xfrm_state.c |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 78f66fa..25e808d 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2002,6 +2002,44 @@ int xfrm_init_state(struct xfrm_state *x)
 
 EXPORT_SYMBOL(xfrm_init_state);
 
+extern struct atomic_notifier_head clock_change_notifier_list;
+static int clock_change_callback(struct notifier_block *nb,
+                               unsigned long reason, void *arg)
+{
+	struct xfrm_state_walk *walk;
+	struct xfrm_state *state;
+	struct net *net;
+	long next;
+
+	rtnl_lock();
+	for_each_net(net) {
+		spin_lock_bh(&xfrm_state_lock);
+		list_for_each_entry(walk, &net->xfrm.state_all, all) {
+			state = container_of(walk, struct xfrm_state, km);
+			spin_lock(&state->lock);
+			if(state->km.dying) {
+				next = 0;
+			} else {
+				state->km.dying = 1;
+				km_state_expired(state, 0, 0);
+				next = state->lft.hard_add_expires_seconds -
+					state->lft.soft_add_expires_seconds;
+			}
+			state->km.state = XFRM_STATE_EXPIRED;
+			tasklet_hrtimer_start(&state->mtimer, ktime_set(next,0), HRTIMER_MODE_REL);
+			spin_unlock(&state->lock);
+		}
+		spin_unlock_bh(&xfrm_state_lock);
+	}
+	rtnl_unlock();
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block clock_change_notifier = {
+        .notifier_call = clock_change_callback,
+};
+
 int __net_init xfrm_state_init(struct net *net)
 {
 	unsigned int sz;
@@ -2026,6 +2064,8 @@ int __net_init xfrm_state_init(struct net *net)
 	INIT_HLIST_HEAD(&net->xfrm.state_gc_list);
 	INIT_WORK(&net->xfrm.state_gc_work, xfrm_state_gc_task);
 	init_waitqueue_head(&net->xfrm.km_waitq);
+	atomic_notifier_chain_register(&clock_change_notifier_list,
+					&clock_change_notifier);
 	return 0;
 
 out_byspi:
-- 
1.7.9.5

[RFC PATCHv2 3/3] xfrm: Revert "Fix unexpected SA hard expiration after changing date"

[Fan Du]

Since SAs lifetime timeout has been updated whenever clock_was_set is
called. So commit: e3c0d04750751389d5116267f8cf4687444d9a50
("Fix unexpected SA hard expiration after changing date") is not needed
anymore.

Signed-off-by: Fan Du <fan.du@windriver.com>
---
 include/net/xfrm.h    |    4 ----
 net/xfrm/xfrm_state.c |   21 ++++-----------------
 2 files changed, 4 insertions(+), 21 deletions(-)

diff --git a/include/net/xfrm.h b/include/net/xfrm.h
index 94ce082..b9df23f 100644
--- a/include/net/xfrm.h
+++ b/include/net/xfrm.h
@@ -214,9 +214,6 @@ struct xfrm_state {
 	struct xfrm_lifetime_cur curlft;
 	struct tasklet_hrtimer	mtimer;
 
-	/* used to fix curlft->add_time when changing date */
-	long		saved_tmo;
-
 	/* Last used time */
 	unsigned long		lastused;
 
@@ -242,7 +239,6 @@ static inline struct net *xs_net(struct xfrm_state *x)
 
 /* xflags - make enum if more show up */
 #define XFRM_TIME_DEFER	1
-#define XFRM_SOFT_EXPIRE 2
 
 enum {
 	XFRM_STATE_VOID,
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 25e808d..310985d 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -407,17 +407,8 @@ static enum hrtimer_restart xfrm_timer_handler(struct hrtimer * me)
 	if (x->lft.hard_add_expires_seconds) {
 		long tmo = x->lft.hard_add_expires_seconds +
 			x->curlft.add_time - now;
-		if (tmo <= 0) {
-			if (x->xflags & XFRM_SOFT_EXPIRE) {
-				/* enter hard expire without soft expire first?!
-				 * setting a new date could trigger this.
-				 * workarbound: fix x->curflt.add_time by below:
-				 */
-				x->curlft.add_time = now - x->saved_tmo - 1;
-				tmo = x->lft.hard_add_expires_seconds - x->saved_tmo;
-			} else
-				goto expired;
-		}
+		if (tmo <= 0)
+			goto expired;
 		if (tmo < next)
 			next = tmo;
 	}
@@ -434,14 +425,10 @@ static enum hrtimer_restart xfrm_timer_handler(struct hrtimer * me)
 	if (x->lft.soft_add_expires_seconds) {
 		long tmo = x->lft.soft_add_expires_seconds +
 			x->curlft.add_time - now;
-		if (tmo <= 0) {
+		if (tmo <= 0)
 			warn = 1;
-			x->xflags &= ~XFRM_SOFT_EXPIRE;
-		} else if (tmo < next) {
+		else if (tmo < next)
 			next = tmo;
-			x->xflags |= XFRM_SOFT_EXPIRE;
-			x->saved_tmo = tmo;
-		}
 	}
 	if (x->lft.soft_use_expires_seconds) {
 		long tmo = x->lft.soft_use_expires_seconds +
-- 
1.7.9.5

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

[Daniel Borkmann]

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 <fan.du@windriver.com>
> ---
>   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.

>   }
>
>   /*
>

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

[Fan Du]

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 <fan.du@windriver.com>
>> ---
>> 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;
}
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.

Thanks Daniel.

>> }
>>
>> /*
>>
>

-- 
浮沉随浪只记今朝笑

--fan

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

[Daniel Borkmann]

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 <fan.du@windriver.com>
>>> ---
>>> 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).

[PATCHv3 net-next ] xfrm: Update xfrm_state lifetime expire after clock_was_set

[Fan Du]

On 2013年08月07日 17:04, Fan Du wrote:
> After clock_was_set called to set new time or host resume from suspend
> state. Notify IKED with soft timeout for SAs which haven't reach its
> soft timeout limit. For those dying SAs, arrange them to hard expire.
>
> Signed-off-by: Fan Du<fan.du@windriver.com>
> ---
>   net/xfrm/xfrm_state.c |   40 ++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 40 insertions(+)
>
> diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
> index 78f66fa..25e808d 100644
> --- a/net/xfrm/xfrm_state.c
> +++ b/net/xfrm/xfrm_state.c
> @@ -2002,6 +2002,44 @@ int xfrm_init_state(struct xfrm_state *x)
>
>   EXPORT_SYMBOL(xfrm_init_state);
>
> +extern struct atomic_notifier_head clock_change_notifier_list;
> +static int clock_change_callback(struct notifier_block *nb,
> +                               unsigned long reason, void *arg)
> +{
> +	struct xfrm_state_walk *walk;
> +	struct xfrm_state *state;
> +	struct net *net;
> +	long next;
> +
> +	rtnl_lock();
> +	for_each_net(net) {
> +		spin_lock_bh(&xfrm_state_lock);
> +		list_for_each_entry(walk,&net->xfrm.state_all, all) {
> +			state = container_of(walk, struct xfrm_state, km);
> +			spin_lock(&state->lock);
                              ^^^^^^^^^^^^^
LOCKDEP complaint here about trying acquiring xfrm_state_lock while holding state->lock.
So fix this with below modifications.


 From bf282199fd172d1bbc6dcd0e38d1b8fc2192e5f5 Mon Sep 17 00:00:00 2001
From: Fan Du <fan.du@windriver.com>
Date: Wed, 14 Aug 2013 12:59:11 +0800
Subject: [PATCHv3 net-next ] xfrm: Update xfrm_state lifetime expire after clock_was_set

After clock_was_set called to set new time or host resume from suspend
state. Notify IKED with soft timeout for SAs which haven't reach its
soft timeout limit. For those dying SAs, arrange them to hard expire.

This modification is characterized by SA is sensible to any degree of
clock changes while as SA lifetime is marked by second.

Another point is clock_was_set is traversing all net name space to
update SA time while holding rtnl_lock, it may not scale very well.

Signed-off-by: Fan Du <fan.du@windriver.com>

v3:
   - Fix lockdep complaint about circular locking with trying to acquire
     state->clock while holding xfrm_state_lock.
v2:
   - Use notifier when clock was set, and then update SA lifetime accordingly.
---
  net/xfrm/xfrm_state.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
  1 file changed, 44 insertions(+)

diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 1df4436..50af80c 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -2002,6 +2002,48 @@ int xfrm_init_state(struct xfrm_state *x)

  EXPORT_SYMBOL(xfrm_init_state);

+static int clock_change_callback(struct notifier_block *nb,
+				unsigned long reason, void *arg)
+{
+	struct xfrm_state_walk *walk;
+	struct xfrm_state *state;
+	struct net *net;
+	long next;
+
+	rtnl_lock();
+	for_each_net(net) {
+		spin_lock_bh(&xfrm_state_lock);
+		list_for_each_entry(walk, &net->xfrm.state_all, all) {
+			state = container_of(walk, struct xfrm_state, km);
+			xfrm_state_hold(state);
+			spin_unlock_bh(&xfrm_state_lock);
+
+			spin_lock_bh(&state->lock);
+			if (state->km.dying) {
+				next = 0;
+			} else {
+				state->km.dying = 1;
+				km_state_expired(state, 0, 0);
+				next = state->lft.hard_add_expires_seconds -
+					state->lft.soft_add_expires_seconds;
+			}
+			state->km.state = XFRM_STATE_EXPIRED;
+			tasklet_hrtimer_start(&state->mtimer, ktime_set(next, 0), HRTIMER_MODE_REL);
+			spin_unlock_bh(&state->lock);
+			xfrm_state_put(state);
+			spin_lock_bh(&xfrm_state_lock);
+		}
+		spin_unlock_bh(&xfrm_state_lock);
+	}
+	rtnl_unlock();
+
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block clock_change_notifier = {
+	.notifier_call = clock_change_callback,
+};
+
  int __net_init xfrm_state_init(struct net *net)
  {
  	unsigned int sz;
@@ -2026,6 +2068,7 @@ int __net_init xfrm_state_init(struct net *net)
  	INIT_HLIST_HEAD(&net->xfrm.state_gc_list);
  	INIT_WORK(&net->xfrm.state_gc_work, xfrm_state_gc_task);
  	init_waitqueue_head(&net->xfrm.km_waitq);
+	register_clock_change_notifier(&clock_change_notifier);
  	return 0;

  out_byspi:
@@ -2057,6 +2100,7 @@ void xfrm_state_fini(struct net *net)
  	xfrm_hash_free(net->xfrm.state_bysrc, sz);
  	WARN_ON(!hlist_empty(net->xfrm.state_bydst));
  	xfrm_hash_free(net->xfrm.state_bydst, sz);
+	unregister_clock_change_notifier(&clock_change_notifier);
  }

  #ifdef CONFIG_AUDITSYSCALL
-- 
1.7.9.5


-- 
浮沉随浪只记今朝笑

--fan

[PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Fan Du]

 From e3929d4fdfad5b40fd8cad0e217597670d1aef54 Mon Sep 17 00:00:00 2001
From: Fan Du <fan.du@windriver.com>
Date: Wed, 14 Aug 2013 16:39:23 +0800
Subject: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

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, e.g Ipsec SA lifetime management.

Signed-off-by: Fan Du <fan.du@windriver.com>

v3:
   -Beautify notifier with register/unregister API exported for other subsystem.

---
  include/linux/hrtimer.h |    3 +++
  kernel/hrtimer.c        |   19 +++++++++++++++++++
  2 files changed, 22 insertions(+)

diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index d19a5c2..f0404e4 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -461,4 +461,7 @@ extern u64 ktime_divns(const ktime_t kt, s64 div);
  /* Show pending timers: */
  extern void sysrq_timer_list_show(void);

+extern int register_clock_change_notifier(struct notifier_block *nb);
+extern int unregister_clock_change_notifier(struct notifier_block *nb);
+
  #endif
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index 383319b..c6e6405 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -755,6 +755,24 @@ static inline void retrigger_next_event(void *arg) { }

  #endif /* CONFIG_HIGH_RES_TIMERS */

+static ATOMIC_NOTIFIER_HEAD(clock_change_notifier_list);
+static int call_clock_change_notifiers(void)
+{
+	return atomic_notifier_call_chain(&clock_change_notifier_list, 0, 0);
+}
+
+int register_clock_change_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_register(&clock_change_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(register_clock_change_notifier);
+
+int unregister_clock_change_notifier(struct notifier_block *nb)
+{
+	return atomic_notifier_chain_unregister(&clock_change_notifier_list, nb);
+}
+EXPORT_SYMBOL_GPL(unregister_clock_change_notifier);
+
  /*
   * Clock realtime was set
   *
@@ -773,6 +791,7 @@ void clock_was_set(void)
  	on_each_cpu(retrigger_next_event, NULL, 1);
  #endif
  	timerfd_clock_was_set();
+	call_clock_change_notifiers();
  }

  /*
-- 
1.7.9.5


-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv3 net-next ] xfrm: Update xfrm_state lifetime expire after clock_was_set

[Steffen Klassert]

On Wed, Aug 14, 2013 at 01:26:02PM +0800, Fan Du wrote:
> 
> From bf282199fd172d1bbc6dcd0e38d1b8fc2192e5f5 Mon Sep 17 00:00:00 2001
> From: Fan Du <fan.du@windriver.com>
> Date: Wed, 14 Aug 2013 12:59:11 +0800
> Subject: [PATCHv3 net-next ] xfrm: Update xfrm_state lifetime expire after clock_was_set
> 
> After clock_was_set called to set new time or host resume from suspend
> state. Notify IKED with soft timeout for SAs which haven't reach its
> soft timeout limit. For those dying SAs, arrange them to hard expire.
> 
> This modification is characterized by SA is sensible to any degree of
> clock changes while as SA lifetime is marked by second.
> 
> Another point is clock_was_set is traversing all net name space to
> update SA time while holding rtnl_lock, it may not scale very well.
> 
> Signed-off-by: Fan Du <fan.du@windriver.com>
> 
> v3:
>   - Fix lockdep complaint about circular locking with trying to acquire
>     state->clock while holding xfrm_state_lock.

Fan, please resubmit the whole patchset and not single patches from a
patchset. Also, please Cc Thomas Gleixner for review of the hrtimer
changes when you resubmit.

Thanks!

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Sergei Shtylyov]

Hello.

On 08/14/2013 12:52 PM, Fan Du wrote:

>  From e3929d4fdfad5b40fd8cad0e217597670d1aef54 Mon Sep 17 00:00:00 2001
> From: Fan Du <fan.du@windriver.com>
> Date: Wed, 14 Aug 2013 16:39:23 +0800
> Subject: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

    This header is not needed.

> 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, e.g Ipsec SA lifetime management.

> Signed-off-by: Fan Du <fan.du@windriver.com>

> v3:
>    -Beautify notifier with register/unregister API exported for other subsystem.

[...]

> diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
> index d19a5c2..f0404e4 100644
> --- a/include/linux/hrtimer.h
> +++ b/include/linux/hrtimer.h
> @@ -461,4 +461,7 @@ extern u64 ktime_divns(const ktime_t kt, s64 div);
>   /* Show pending timers: */
>   extern void sysrq_timer_list_show(void);
>
> +extern int register_clock_change_notifier(struct notifier_block *nb);
> +extern int unregister_clock_change_notifier(struct notifier_block *nb);
> +
>   #endif
> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index 383319b..c6e6405 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -755,6 +755,24 @@ static inline void retrigger_next_event(void *arg) { }
>
>   #endif /* CONFIG_HIGH_RES_TIMERS */
>
> +static ATOMIC_NOTIFIER_HEAD(clock_change_notifier_list);
> +static int call_clock_change_notifiers(void)
> +{
> +    return atomic_notifier_call_chain(&clock_change_notifier_list, 0, 0);
> +}
> +
> +int register_clock_change_notifier(struct notifier_block *nb)
> +{
> +    return atomic_notifier_chain_register(&clock_change_notifier_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(register_clock_change_notifier);
> +
> +int unregister_clock_change_notifier(struct notifier_block *nb)
> +{
> +    return atomic_notifier_chain_unregister(&clock_change_notifier_list, nb);
> +}
> +EXPORT_SYMBOL_GPL(unregister_clock_change_notifier);
> +
>   /*
>    * Clock realtime was set
>    *
> @@ -773,6 +791,7 @@ void clock_was_set(void)
>       on_each_cpu(retrigger_next_event, NULL, 1);
>   #endif
>       timerfd_clock_was_set();
> +    call_clock_change_notifiers();
>   }

    Your patch seems whitepsace damaged, i.e. a space is added before each 
line starting with space.

WBR, Sergei

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Thomas Gleixner]

On Wed, 14 Aug 2013, Fan Du wrote:

> From e3929d4fdfad5b40fd8cad0e217597670d1aef54 Mon Sep 17 00:00:00 2001
> From: Fan Du <fan.du@windriver.com>
> Date: Wed, 14 Aug 2013 16:39:23 +0800
> Subject: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was
> called
> 
> 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, e.g Ipsec SA lifetime management.

Sigh. These notifiers have been proposed in the past and we always
rejected them. Why do you need this? There should be nothing except
the core timekeeping code which needs to know about clock_was_set.

Can you please explain what kind of users you have in mind and WHY
they need to know about it.

Thanks,

	tglx

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Fan Du]

On 2013年08月18日 17:05, Thomas Gleixner wrote:
> On Wed, 14 Aug 2013, Fan Du wrote:
>
>>  From e3929d4fdfad5b40fd8cad0e217597670d1aef54 Mon Sep 17 00:00:00 2001
>> From: Fan Du<fan.du@windriver.com>
>> Date: Wed, 14 Aug 2013 16:39:23 +0800
>> Subject: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was
>> called
>>
>> 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, e.g Ipsec SA lifetime management.
>
> Sigh. These notifiers have been proposed in the past and we always
> rejected them. Why do you need this? There should be nothing except
> the core timekeeping code which needs to know about clock_was_set.
>
> Can you please explain what kind of users you have in mind and WHY
> they need to know about it.

Hi, Thomas


Thanks for your patience. Please let me take a few seconds try to
explain this.

Current xfrm layers has *one* hrtimer to guard Ipsec keys timeout,
The timeout could be measured in either of below two ways:

  (1) The timer is started once the keys is created, but this
      key is not necessary actually used right now. In detail,
      record the get_seconds() when this key is created.

  (2) Starting the timer when this key is actually used, e.g when
      an IP packet need to be encrypted. In details, recored the
      get_seconds() when this key is first used.

So in the hrtimer handler, the code get current get_seconds, and
check against with what saved in (1)or(2), and notify the timeout
up to user land.

So the pitfall is using one hrtimer for two timeout events,
most importantly using get_seconds to check timeout, once system
clock is changed by user intentionally, the key timeout could
misbehave wildly.

A refractor has been proposed to get rid of depending on system wall
clock by cleaning up the hrtimer handler. Unfortunately David frowned
on it in (3), and suggest once system clock is changed, adjust the
timeout of the key.


(3): http://www.spinics.net/lists/netdev/msg245169.html


> Thanks,
>
> 	tglx
>

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Thomas Gleixner]

On Tue, 20 Aug 2013, Fan Du wrote:
> Thanks for your patience. Please let me take a few seconds try to
> explain this.

Sorry for the late reply.
 
> Current xfrm layers has *one* hrtimer to guard Ipsec keys timeout,
> The timeout could be measured in either of below two ways:
> 
>  (1) The timer is started once the keys is created, but this
>      key is not necessary actually used right now. In detail,
>      record the get_seconds() when this key is created.
> 
>  (2) Starting the timer when this key is actually used, e.g when
>      an IP packet need to be encrypted. In details, recored the
>      get_seconds() when this key is first used.
> 
> So in the hrtimer handler, the code get current get_seconds, and
> check against with what saved in (1)or(2), and notify the timeout
> up to user land.
> 
> So the pitfall is using one hrtimer for two timeout events,
> most importantly using get_seconds to check timeout, once system
> clock is changed by user intentionally, the key timeout could
> misbehave wildly.
> 
> A refractor has been proposed to get rid of depending on system wall
> clock by cleaning up the hrtimer handler. Unfortunately David frowned
> on it in (3), and suggest once system clock is changed, adjust the
> timeout of the key.
> 
> 
> (3): http://www.spinics.net/lists/netdev/msg245169.html

Thanks for the explanation so far.

What's still unclear to me is why these timeouts are bound to wall
time in the first place.

Is there any real reason why the key life time can't simply be
expressed in monotonic time, e.g. N seconds after creation or M
seconds after usage? Looking at the relevant RFCs I can't find any
requirement for binding the life time to wall time. 

A life time of 10 minutes does not change when the wall clock is
adjusted for whatever reasons. It's still 10 minutes and not some
random result of the wall clock adjustments. But I might be wrong as
usual :)

Thanks,

	tglx

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Herbert Xu]

On Thu, Sep 12, 2013 at 03:21:24PM +0200, Thomas Gleixner wrote:
>
> > (3): http://www.spinics.net/lists/netdev/msg245169.html
> 
> Thanks for the explanation so far.
> 
> What's still unclear to me is why these timeouts are bound to wall
> time in the first place.
> 
> Is there any real reason why the key life time can't simply be
> expressed in monotonic time, e.g. N seconds after creation or M
> seconds after usage? Looking at the relevant RFCs I can't find any
> requirement for binding the life time to wall time. 
> 
> A life time of 10 minutes does not change when the wall clock is
> adjusted for whatever reasons. It's still 10 minutes and not some
> random result of the wall clock adjustments. But I might be wrong as
> usual :)

Well we started out with straight timers.  It was changed because
people wanted IPsec SAs to expire after a suspect/resume which
AFAIK does not touch normal timers.

Of course, this brought with it a new set of problems when the
system time is stepped which now cause SAs to expire even though
they probably shouldn't.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Herbert Xu]

On Thu, Sep 12, 2013 at 03:21:24PM +0200, Thomas Gleixner wrote:
>
> > (3): http://www.spinics.net/lists/netdev/msg245169.html
> 
> Thanks for the explanation so far.
> 
> What's still unclear to me is why these timeouts are bound to wall
> time in the first place.
> 
> Is there any real reason why the key life time can't simply be
> expressed in monotonic time, e.g. N seconds after creation or M
> seconds after usage? Looking at the relevant RFCs I can't find any
> requirement for binding the life time to wall time. 
> 
> A life time of 10 minutes does not change when the wall clock is
> adjusted for whatever reasons. It's still 10 minutes and not some
> random result of the wall clock adjustments. But I might be wrong as
> usual :)

Well we started out with straight timers.  It was changed because
people wanted IPsec SAs to expire after a suspect/resume which
AFAIK does not touch normal timers.

Of course, this brought with it a new set of problems when the
system time is stepped which now cause SAs to expire even though
they probably shouldn't.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Thomas Gleixner]

On Thu, 12 Sep 2013, Herbert Xu wrote:
> On Thu, Sep 12, 2013 at 03:21:24PM +0200, Thomas Gleixner wrote:
> >
> > > (3): http://www.spinics.net/lists/netdev/msg245169.html
> > 
> > Thanks for the explanation so far.
> > 
> > What's still unclear to me is why these timeouts are bound to wall
> > time in the first place.
> > 
> > Is there any real reason why the key life time can't simply be
> > expressed in monotonic time, e.g. N seconds after creation or M
> > seconds after usage? Looking at the relevant RFCs I can't find any
> > requirement for binding the life time to wall time. 
> > 
> > A life time of 10 minutes does not change when the wall clock is
> > adjusted for whatever reasons. It's still 10 minutes and not some
> > random result of the wall clock adjustments. But I might be wrong as
> > usual :)
> 
> Well we started out with straight timers.  It was changed because
> people wanted IPsec SAs to expire after a suspect/resume which

Right suspend is the usual suspect :)

> AFAIK does not touch normal timers.
> 
> Of course, this brought with it a new set of problems when the
> system time is stepped which now cause SAs to expire even though
> they probably shouldn't.

Right. That's what I guessed. So your problem is that the timer_list
timers which are the proper mechanism for this (the life time has a 1
second granularity, so hrtimers are complete overkill) are not
expiring after a suspend/resume cycle.

So what about going back to timer_list timers and simply utilize
register_pm_notifier(), which will tell you that the system resumed?

Thanks,

	tglx

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Thomas Gleixner]

On Thu, 12 Sep 2013, Herbert Xu wrote:
> On Thu, Sep 12, 2013 at 03:21:24PM +0200, Thomas Gleixner wrote:
> >
> > > (3): http://www.spinics.net/lists/netdev/msg245169.html
> > 
> > Thanks for the explanation so far.
> > 
> > What's still unclear to me is why these timeouts are bound to wall
> > time in the first place.
> > 
> > Is there any real reason why the key life time can't simply be
> > expressed in monotonic time, e.g. N seconds after creation or M
> > seconds after usage? Looking at the relevant RFCs I can't find any
> > requirement for binding the life time to wall time. 
> > 
> > A life time of 10 minutes does not change when the wall clock is
> > adjusted for whatever reasons. It's still 10 minutes and not some
> > random result of the wall clock adjustments. But I might be wrong as
> > usual :)
> 
> Well we started out with straight timers.  It was changed because
> people wanted IPsec SAs to expire after a suspect/resume which

Right suspend is the usual suspect :)

> AFAIK does not touch normal timers.
> 
> Of course, this brought with it a new set of problems when the
> system time is stepped which now cause SAs to expire even though
> they probably shouldn't.

Right. That's what I guessed. So your problem is that the timer_list
timers which are the proper mechanism for this (the life time has a 1
second granularity, so hrtimers are complete overkill) are not
expiring after a suspend/resume cycle.

So what about going back to timer_list timers and simply utilize
register_pm_notifier(), which will tell you that the system resumed?

Thanks,

	tglx

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[David Miller]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Thu, 12 Sep 2013 16:43:37 +0200 (CEST)

> So what about going back to timer_list timers and simply utilize
> register_pm_notifier(), which will tell you that the system resumed?

The thing to understand is that there are two timeouts for an IPSEC
rule, a soft and a hard timeout.

There is a gap between these two exactly so that we can negotiate a
new encapsulation with the IPSEC gateway before communication ceases
to be possible over the IPSEC protected path.

So the idea is that the soft timeout triggers the re-negotiation,
and after a hard timeout the IPSEC path is no longer usable and
all communication will fail.

Simply triggering a re-negoation after every suspend/resume makes
no sense at all.  Spurious re-negotiations are undesirable.

What we want are real timers.  We want that rather than a "we
suspended so just assume all timers expired" event which is not very
useful for this kind of application.

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[David Miller]

From: Thomas Gleixner <tglx@linutronix.de>
Date: Thu, 12 Sep 2013 16:43:37 +0200 (CEST)

> So what about going back to timer_list timers and simply utilize
> register_pm_notifier(), which will tell you that the system resumed?

The thing to understand is that there are two timeouts for an IPSEC
rule, a soft and a hard timeout.

There is a gap between these two exactly so that we can negotiate a
new encapsulation with the IPSEC gateway before communication ceases
to be possible over the IPSEC protected path.

So the idea is that the soft timeout triggers the re-negotiation,
and after a hard timeout the IPSEC path is no longer usable and
all communication will fail.

Simply triggering a re-negoation after every suspend/resume makes
no sense at all.  Spurious re-negotiations are undesirable.

What we want are real timers.  We want that rather than a "we
suspended so just assume all timers expired" event which is not very
useful for this kind of application.

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Thomas Gleixner]

On Thu, 12 Sep 2013, David Miller wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Thu, 12 Sep 2013 16:43:37 +0200 (CEST)
> 
> > So what about going back to timer_list timers and simply utilize
> > register_pm_notifier(), which will tell you that the system resumed?
> 
> The thing to understand is that there are two timeouts for an IPSEC
> rule, a soft and a hard timeout.
> 
> There is a gap between these two exactly so that we can negotiate a
> new encapsulation with the IPSEC gateway before communication ceases
> to be possible over the IPSEC protected path.
> 
> So the idea is that the soft timeout triggers the re-negotiation,
> and after a hard timeout the IPSEC path is no longer usable and
> all communication will fail.
> 
> Simply triggering a re-negoation after every suspend/resume makes
> no sense at all.  Spurious re-negotiations are undesirable.
> 
> What we want are real timers.  We want that rather than a "we
> suspended so just assume all timers expired" event which is not very
> useful for this kind of application.

Your argumentation makes no sense at all. Where is the difference
between the "real timer" plus a clock was set notification and a timer
list timer and a resume notification?

In both cases you need to walk through the timers and reevaluate
them. Just in the clock was set notification case you need to deal
with NTP/settimeofday/PPS and whatever cases which are completely
irrelevant to the life time management.

So what's wrong with:

   now = get_seconds();
   x->timeout = now + x->soft_timeout;
   x->timeout_active = SOFT;
   start_timer(x->timer, jiffies + sec_to_jiffies(x->soft_timeout));

In the timer handler:

   switch (x->timeout_active) {
      case SOFT:
            trigger_renegotiation();
	    hts = x->hard_timeout - x->soft_timeout;
	    x->timeout += hts;
	    x->timeout_active = HARD;
      	    start_timer(x->timer, jiffies + sec_to_jiffies(hts));
	    break;
      case HARD:
      	   stop_connection();
	   break:
   }
   
If the negotiation succeeds:

   now = get_seconds();
   x->t_timeout = now + x->soft_timeout;
   x->timeout_active = SOFT;
   mod_timer(x->timer, jiffies + sec_to_jiffies(x->soft_timeout));

Now in the resume notification you walk the active timers and do for
each timer:

    now = get_seconds();

    switch (x->timeout_active) {
    case SOFT:
    	 if (now >= x->timeout) {
	    hts = x->hard_timeout - x->soft_timeout;
	    x->timeout += hts;
	    if (now >= x->timeout) {
	       del_timer(x->timeout);
	       stop_connection();
	       break:
	    }
	    trigger_renegotiation();
	    x->timeout_active = HARD;
      	    mod_timer(x->timer, jiffies + sec_to_jiffies(hts));
	    break;
   	 }
	 delta = x->timeout - now;
      	 mod_timer(x->timer, jiffies + sec_to_jiffies(delta));
	 break;

    case HARD:	    
    	 if (now >= x->timeout) {
	       del_timer(x->timeout);
	       stop_connection();
	 }    	 
	 delta = x->timeout - now;
      	 mod_timer(x->timer, jiffies + sec_to_jiffies(delta));
	 break;
    }

That's what you have to do with a clock was set notification as
well. And what's worse is that you need to figure out whether the
clock change was due to a suspend/resume cycle or just because
NTP/settimeofday/PPS or whatever decided to fiddle with the wall
time. And you need to do that to avoid the spurious renegotiations
which you are afraid of.


Thanks,

	tglx

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Thomas Gleixner]

On Thu, 12 Sep 2013, David Miller wrote:
> From: Thomas Gleixner <tglx@linutronix.de>
> Date: Thu, 12 Sep 2013 16:43:37 +0200 (CEST)
> 
> > So what about going back to timer_list timers and simply utilize
> > register_pm_notifier(), which will tell you that the system resumed?
> 
> The thing to understand is that there are two timeouts for an IPSEC
> rule, a soft and a hard timeout.
> 
> There is a gap between these two exactly so that we can negotiate a
> new encapsulation with the IPSEC gateway before communication ceases
> to be possible over the IPSEC protected path.
> 
> So the idea is that the soft timeout triggers the re-negotiation,
> and after a hard timeout the IPSEC path is no longer usable and
> all communication will fail.
> 
> Simply triggering a re-negoation after every suspend/resume makes
> no sense at all.  Spurious re-negotiations are undesirable.
> 
> What we want are real timers.  We want that rather than a "we
> suspended so just assume all timers expired" event which is not very
> useful for this kind of application.

Your argumentation makes no sense at all. Where is the difference
between the "real timer" plus a clock was set notification and a timer
list timer and a resume notification?

In both cases you need to walk through the timers and reevaluate
them. Just in the clock was set notification case you need to deal
with NTP/settimeofday/PPS and whatever cases which are completely
irrelevant to the life time management.

So what's wrong with:

   now = get_seconds();
   x->timeout = now + x->soft_timeout;
   x->timeout_active = SOFT;
   start_timer(x->timer, jiffies + sec_to_jiffies(x->soft_timeout));

In the timer handler:

   switch (x->timeout_active) {
      case SOFT:
            trigger_renegotiation();
	    hts = x->hard_timeout - x->soft_timeout;
	    x->timeout += hts;
	    x->timeout_active = HARD;
      	    start_timer(x->timer, jiffies + sec_to_jiffies(hts));
	    break;
      case HARD:
      	   stop_connection();
	   break:
   }
   
If the negotiation succeeds:

   now = get_seconds();
   x->t_timeout = now + x->soft_timeout;
   x->timeout_active = SOFT;
   mod_timer(x->timer, jiffies + sec_to_jiffies(x->soft_timeout));

Now in the resume notification you walk the active timers and do for
each timer:

    now = get_seconds();

    switch (x->timeout_active) {
    case SOFT:
    	 if (now >= x->timeout) {
	    hts = x->hard_timeout - x->soft_timeout;
	    x->timeout += hts;
	    if (now >= x->timeout) {
	       del_timer(x->timeout);
	       stop_connection();
	       break:
	    }
	    trigger_renegotiation();
	    x->timeout_active = HARD;
      	    mod_timer(x->timer, jiffies + sec_to_jiffies(hts));
	    break;
   	 }
	 delta = x->timeout - now;
      	 mod_timer(x->timer, jiffies + sec_to_jiffies(delta));
	 break;

    case HARD:	    
    	 if (now >= x->timeout) {
	       del_timer(x->timeout);
	       stop_connection();
	 }    	 
	 delta = x->timeout - now;
      	 mod_timer(x->timer, jiffies + sec_to_jiffies(delta));
	 break;
    }

That's what you have to do with a clock was set notification as
well. And what's worse is that you need to figure out whether the
clock change was due to a suspend/resume cycle or just because
NTP/settimeofday/PPS or whatever decided to fiddle with the wall
time. And you need to do that to avoid the spurious renegotiations
which you are afraid of.


Thanks,

	tglx

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[John Stultz]

On Thu, Sep 12, 2013 at 6:44 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, Sep 12, 2013 at 03:21:24PM +0200, Thomas Gleixner wrote:
>>
>> > (3): http://www.spinics.net/lists/netdev/msg245169.html
>>
>> Thanks for the explanation so far.
>>
>> What's still unclear to me is why these timeouts are bound to wall
>> time in the first place.
>>
>> Is there any real reason why the key life time can't simply be
>> expressed in monotonic time, e.g. N seconds after creation or M
>> seconds after usage? Looking at the relevant RFCs I can't find any
>> requirement for binding the life time to wall time.
>>
>> A life time of 10 minutes does not change when the wall clock is
>> adjusted for whatever reasons. It's still 10 minutes and not some
>> random result of the wall clock adjustments. But I might be wrong as
>> usual :)
>
> Well we started out with straight timers.  It was changed because
> people wanted IPsec SAs to expire after a suspect/resume which
> AFAIK does not touch normal timers.

I'm not sure I've totally groked the specific need here, but if you're
wanting a monotonic clockbase that includes suspend time, then you
might checkout CLOCK_BOOTTIME.

thanks
-john

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[John Stultz]

On Thu, Sep 12, 2013 at 6:44 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> On Thu, Sep 12, 2013 at 03:21:24PM +0200, Thomas Gleixner wrote:
>>
>> > (3): http://www.spinics.net/lists/netdev/msg245169.html
>>
>> Thanks for the explanation so far.
>>
>> What's still unclear to me is why these timeouts are bound to wall
>> time in the first place.
>>
>> Is there any real reason why the key life time can't simply be
>> expressed in monotonic time, e.g. N seconds after creation or M
>> seconds after usage? Looking at the relevant RFCs I can't find any
>> requirement for binding the life time to wall time.
>>
>> A life time of 10 minutes does not change when the wall clock is
>> adjusted for whatever reasons. It's still 10 minutes and not some
>> random result of the wall clock adjustments. But I might be wrong as
>> usual :)
>
> Well we started out with straight timers.  It was changed because
> people wanted IPsec SAs to expire after a suspect/resume which
> AFAIK does not touch normal timers.

I'm not sure I've totally groked the specific need here, but if you're
wanting a monotonic clockbase that includes suspend time, then you
might checkout CLOCK_BOOTTIME.

thanks
-john

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Thomas Gleixner]

On Thu, 12 Sep 2013, John Stultz wrote:
> On Thu, Sep 12, 2013 at 6:44 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > On Thu, Sep 12, 2013 at 03:21:24PM +0200, Thomas Gleixner wrote:
> >>
> >> > (3): http://www.spinics.net/lists/netdev/msg245169.html
> >>
> >> Thanks for the explanation so far.
> >>
> >> What's still unclear to me is why these timeouts are bound to wall
> >> time in the first place.
> >>
> >> Is there any real reason why the key life time can't simply be
> >> expressed in monotonic time, e.g. N seconds after creation or M
> >> seconds after usage? Looking at the relevant RFCs I can't find any
> >> requirement for binding the life time to wall time.
> >>
> >> A life time of 10 minutes does not change when the wall clock is
> >> adjusted for whatever reasons. It's still 10 minutes and not some
> >> random result of the wall clock adjustments. But I might be wrong as
> >> usual :)
> >
> > Well we started out with straight timers.  It was changed because
> > people wanted IPsec SAs to expire after a suspect/resume which
> > AFAIK does not touch normal timers.
> 
> I'm not sure I've totally groked the specific need here, but if you're
> wanting a monotonic clockbase that includes suspend time, then you
> might checkout CLOCK_BOOTTIME.

Duh, completely forgot about that one. Sure that would avoid the whole
business.

Thanks,

	tglx

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Thomas Gleixner]

On Thu, 12 Sep 2013, John Stultz wrote:
> On Thu, Sep 12, 2013 at 6:44 AM, Herbert Xu <herbert@gondor.apana.org.au> wrote:
> > On Thu, Sep 12, 2013 at 03:21:24PM +0200, Thomas Gleixner wrote:
> >>
> >> > (3): http://www.spinics.net/lists/netdev/msg245169.html
> >>
> >> Thanks for the explanation so far.
> >>
> >> What's still unclear to me is why these timeouts are bound to wall
> >> time in the first place.
> >>
> >> Is there any real reason why the key life time can't simply be
> >> expressed in monotonic time, e.g. N seconds after creation or M
> >> seconds after usage? Looking at the relevant RFCs I can't find any
> >> requirement for binding the life time to wall time.
> >>
> >> A life time of 10 minutes does not change when the wall clock is
> >> adjusted for whatever reasons. It's still 10 minutes and not some
> >> random result of the wall clock adjustments. But I might be wrong as
> >> usual :)
> >
> > Well we started out with straight timers.  It was changed because
> > people wanted IPsec SAs to expire after a suspect/resume which
> > AFAIK does not touch normal timers.
> 
> I'm not sure I've totally groked the specific need here, but if you're
> wanting a monotonic clockbase that includes suspend time, then you
> might checkout CLOCK_BOOTTIME.

Duh, completely forgot about that one. Sure that would avoid the whole
business.

Thanks,

	tglx

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Fan Du]

Hi Dave/Thomas

On 2013年09月13日 01:32, David Miller wrote:
> From: Thomas Gleixner<tglx@linutronix.de>
> Date: Thu, 12 Sep 2013 16:43:37 +0200 (CEST)
>
>> So what about going back to timer_list timers and simply utilize
>> register_pm_notifier(), which will tell you that the system resumed?
>
> The thing to understand is that there are two timeouts for an IPSEC
> rule, a soft and a hard timeout.
>
> There is a gap between these two exactly so that we can negotiate a
> new encapsulation with the IPSEC gateway before communication ceases
> to be possible over the IPSEC protected path.
>
> So the idea is that the soft timeout triggers the re-negotiation,
> and after a hard timeout the IPSEC path is no longer usable and
> all communication will fail.
>
> Simply triggering a re-negoation after every suspend/resume makes
> no sense at all.  Spurious re-negotiations are undesirable.
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ (*a*)

What's the differences between this with re-negotiation after every
system wall clock changing by using clock_was_set notifier?


 > On 2013年08月02日 06:35, David Miller wrote:
 >
 > I suspect the thing to do is to have system time changes generate a
 > notifier when clock_was_set() is called.
 >
 > The XFRM code would walk the rules and pretend that we hit the soft
 > timeout for every rule that we haven't hit the soft timeout yet
 > already.
 >
 > If a rule hit the soft timeout, force a hard timeout.
 >
 > When forcing a soft timeout, adjust the hard timeout to be
 > (hard_timeout - soft_timeout) into the future.



> What we want are real timers.  We want that rather than a "we
> suspended so just assume all timers expired" event which is not very
> useful for this kind of application.
>

Here we are facing two problems:)

(1) what kind timer should xfrm_state should employ, Two requirements here:
     First one, KEY lifetime should include suspend/resume time. Second one,
     system wall clock time changing(backward/forward) should *not* impact
     *timer* timeout event(not the soft/hard IPsec events fired to user space!)

     net-next commit 99565a6c471cbb66caa68347c195133017559943 ("xfrm: Make
     xfrm_state timer monotonic") by utilizing *CLOCK_BOOTTIME* has solved this problem.

(2) What I have been bugging you around here for this long time is really the second
     problem, I'm sorry I didn't make it clearly to you and others, which is below:

     Why using wall clock time to calculate soft/hard IPsec events when xfrm_state timer
     out happens in its timeout handler? Because even if xfrm_state using CLOCK_BOOTTIME,
     system wall clock time changing will surely disturb soft/hard IPsec events, which
     you raised your concern about in (*a*).

     The initial approach( http://marc.info/?l=linux-netdev&m=137534280429187&w=2) has
     tried to solve this second problem by eliminating depending system wall clock in
     xfrm_state timer timeout handler.

I think this time, I have made this situation crystal clear.

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Thomas Gleixner]

On Fri, 13 Sep 2013, Fan Du wrote:
> (2) What I have been bugging you around here for this long time is really the
> second
>     problem, I'm sorry I didn't make it clearly to you and others, which is
> below:
> 
>     Why using wall clock time to calculate soft/hard IPsec events when
> xfrm_state timer
>     out happens in its timeout handler? Because even if xfrm_state using
> CLOCK_BOOTTIME,
>     system wall clock time changing will surely disturb soft/hard IPsec
> events, which
>     you raised your concern about in (*a*).

No CLOCK_BOOTTIME is not affected by wall clock time changes. It's
basically CLOCK_MONOTONIC, it just keeps counting the suspend time as
well. So without a suspend/resume cycle CLOCK_MONOTONIC and
CLOCK_BOOTTIME are the same. After a suspend/resume cycle
CLOCK_BOOTTIME will be N seconds ahead of CLOCK_MONOTONIC, where N is
the number of seconds spent in suspend.

Thanks,

	tglx

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Fan Du]

Hi, Thomas

On 2013年09月13日 22:32, Thomas Gleixner wrote:
> On Fri, 13 Sep 2013, Fan Du wrote:
>> (2) What I have been bugging you around here for this long time is really the
>> second
>>      problem, I'm sorry I didn't make it clearly to you and others, which is
>> below:
>>
>>      Why using wall clock time to calculate soft/hard IPsec events when
>> xfrm_state timer
>>      out happens in its timeout handler? Because even if xfrm_state using
>> CLOCK_BOOTTIME,
>>      system wall clock time changing will surely disturb soft/hard IPsec
>> events, which
>>      you raised your concern about in (*a*).
>
> No CLOCK_BOOTTIME is not affected by wall clock time changes. It's
> basically CLOCK_MONOTONIC, it just keeps counting the suspend time as
> well. So without a suspend/resume cycle CLOCK_MONOTONIC and
> CLOCK_BOOTTIME are the same. After a suspend/resume cycle
> CLOCK_BOOTTIME will be N seconds ahead of CLOCK_MONOTONIC, where N is
> the number of seconds spent in suspend.

Sorry for the ambiguity. Yes, CLOCK_BOOTTIME is monotonic *and* counting
suspend/resume time as well as not affected by wall clock time change.

Using CLOCK_BOOTTIME guarantees b- will timeout in a right manner, i.e., not
affected by wall clock time change, as well as keep the timer valid when
suspend/resume.

A classic way of using timer is:
  a- Arm a timer with specified interval
  b- Wait for the timer to timeout
  c- After the timeout, notify the event to other place in the timer handler.

IPsec key lifetime timer does its this way:
  a- Arm a timer with specified interval
  b- Wait for the timer to timeout
  c- After timeout, in the timer handler, using wall clock time to calculate
    which kind of event to notify user(soft or hard for both key use time,
    and key created time). So the problem arises at this stage if wall clock
    time changed.



Thanks

> Thanks,
>
> 	tglx
>

-- 
浮沉随浪只记今朝笑

--fan

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2076 bytes --]

On Mon, 16 Sep 2013, Fan Du wrote:
> On 2013年09月13日 22:32, Thomas Gleixner wrote:
> > On Fri, 13 Sep 2013, Fan Du wrote:
> > > (2) What I have been bugging you around here for this long time is really
> > > the
> > > second
> > >      problem, I'm sorry I didn't make it clearly to you and others, which
> > > is
> > > below:
> > > 
> > >      Why using wall clock time to calculate soft/hard IPsec events when
> > > xfrm_state timer
> > >      out happens in its timeout handler? Because even if xfrm_state using
> > > CLOCK_BOOTTIME,
> > >      system wall clock time changing will surely disturb soft/hard IPsec
> > > events, which
> > >      you raised your concern about in (*a*).
> > 
> > No CLOCK_BOOTTIME is not affected by wall clock time changes. It's
> > basically CLOCK_MONOTONIC, it just keeps counting the suspend time as
> > well. So without a suspend/resume cycle CLOCK_MONOTONIC and
> > CLOCK_BOOTTIME are the same. After a suspend/resume cycle
> > CLOCK_BOOTTIME will be N seconds ahead of CLOCK_MONOTONIC, where N is
> > the number of seconds spent in suspend.
> 
> Sorry for the ambiguity. Yes, CLOCK_BOOTTIME is monotonic *and* counting
> suspend/resume time as well as not affected by wall clock time change.
> 
> Using CLOCK_BOOTTIME guarantees b- will timeout in a right manner, i.e., not
> affected by wall clock time change, as well as keep the timer valid when
> suspend/resume.
> 
> A classic way of using timer is:
>  a- Arm a timer with specified interval
>  b- Wait for the timer to timeout
>  c- After the timeout, notify the event to other place in the timer handler.
> 
> IPsec key lifetime timer does its this way:
>  a- Arm a timer with specified interval
>  b- Wait for the timer to timeout
>  c- After timeout, in the timer handler, using wall clock time to calculate
>    which kind of event to notify user(soft or hard for both key use time,
>    and key created time). So the problem arises at this stage if wall clock
>    time changed.

And why on earth must it use wall clock time for selecting the event
type?

Thanks,

	tglx

Re: [PATCHv3 linux-next] hrtimer: Add notifier when clock_was_set was called

[Fan Du]

On 2013年09月16日 17:01, Thomas Gleixner wrote:
> On Mon, 16 Sep 2013, Fan Du wrote:
>> On 2013年09月13日 22:32, Thomas Gleixner wrote:
>>> On Fri, 13 Sep 2013, Fan Du wrote:
>>>> (2) What I have been bugging you around here for this long time is really
>>>> the
>>>> second
>>>>       problem, I'm sorry I didn't make it clearly to you and others, which
>>>> is
>>>> below:
>>>>
>>>>       Why using wall clock time to calculate soft/hard IPsec events when
>>>> xfrm_state timer
>>>>       out happens in its timeout handler? Because even if xfrm_state using
>>>> CLOCK_BOOTTIME,
>>>>       system wall clock time changing will surely disturb soft/hard IPsec
>>>> events, which
>>>>       you raised your concern about in (*a*).
>>>
>>> No CLOCK_BOOTTIME is not affected by wall clock time changes. It's
>>> basically CLOCK_MONOTONIC, it just keeps counting the suspend time as
>>> well. So without a suspend/resume cycle CLOCK_MONOTONIC and
>>> CLOCK_BOOTTIME are the same. After a suspend/resume cycle
>>> CLOCK_BOOTTIME will be N seconds ahead of CLOCK_MONOTONIC, where N is
>>> the number of seconds spent in suspend.
>>
>> Sorry for the ambiguity. Yes, CLOCK_BOOTTIME is monotonic *and* counting
>> suspend/resume time as well as not affected by wall clock time change.
>>
>> Using CLOCK_BOOTTIME guarantees b- will timeout in a right manner, i.e., not
>> affected by wall clock time change, as well as keep the timer valid when
>> suspend/resume.
>>
>> A classic way of using timer is:
>>   a- Arm a timer with specified interval
>>   b- Wait for the timer to timeout
>>   c- After the timeout, notify the event to other place in the timer handler.
>>
>> IPsec key lifetime timer does its this way:
>>   a- Arm a timer with specified interval
>>   b- Wait for the timer to timeout
>>   c- After timeout, in the timer handler, using wall clock time to calculate
>>     which kind of event to notify user(soft or hard for both key use time,
>>     and key created time). So the problem arises at this stage if wall clock
>>     time changed.
>
> And why on earth must it use wall clock time for selecting the event
> type?

/*shivering... sorry to bother you again.*/

Yep, it's a bit twisted. This parts of codes come a long way from v2.5.67
Beyond this fact, I barely know its original design goal by doing so.
The first version of this patch is to remove the wall clock time things
by myself, it's a pity that the feedback is not very welcome at the end.
So later on adding notifier at clock_was_set is suggested by David.

> Thanks,
>
> 	tglx

-- 
浮沉随浪只记今朝笑

--fan