net: lls cleanup patches

PATCH Applied Eliezer Tamir 4 participants 17 messages Jun 28, 2013 – Jul 3, 2013

Subsystems (3)

Networking Network drivers VFS

lore

Share & tools

lore

Eliezer Tamir

author

2013-06-28 12:59 UTC #0 lore raw

cover net: lls cleanup patches

David,

Here are two cleanup patches.

1. fix warning from debug_smp_processor_id().
- reported by Cody P Schafer.

2. avoid calling sched_clock() in select and poll when lls is disabled.
- reported by Andi Kleen.

Thanks to everyone for their comments.

-Eliezer

Eliezer Tamir

author

2013-06-28 12:59 UTC #1 lore raw

PATCH 1/2 net: fix LLS debug_smp_processor_id() warning

Our use of sched_clock is OK because we don't mind the side effects of calling it and occasionally waking up on a different CPU.

When CONFIG_DEBUG_PREEMPT is on, disable preempt before calling sched_clock() so we don't trigger a debug_smp_processor_id() warning.

Reported-by Cody P Schafer <[email protected]>

Signed-off-by Eliezer Tamir <[email protected]>

---

 include/net/ll_poll.h |   30 +++++++++++++++++++++++++-----
 1 files changed, 25 insertions(+), 5 deletions(-)

include/net/ll_poll.h +25 -5

index 5bf2b3a..6d45e6f 100644
@@ -37,20 +37,40 @@ extern unsigned int sysctl_net_ll_poll __read_mostly;
 #define LL_FLUSH_FAILED		-1
 #define LL_FLUSH_BUSY		-2
 
-/* we can use sched_clock() because we don't care much about precision
+/* a wrapper to make debug_smp_processor_id() happy
+ * we can use sched_clock() because we don't care much about precision
  * we only care that the average is bounded
- * we don't mind a ~2.5% imprecision so <<10 instead of *1000
+ */
+#ifdef CONFIG_DEBUG_PREEMPT
+static inline u64 ll_sched_clock(void)
+{
+	u64 rc;
+
+	preempt_disable_notrace();
+	rc = sched_clock();
+	preempt_enable_no_resched_notrace();
+
+	return rc;
+}
+#else /* CONFIG_DEBUG_PREEMPT */
+static inline u64 ll_sched_clock(void)
+{
+	return sched_clock();
+}
+#endif /* CONFIG_DEBUG_PREEMPT */
+
+/* we don't mind a ~2.5% imprecision so <<10 instead of *1000
  * sk->sk_ll_usec is a u_int so this can't overflow
  */
 static inline u64 ll_sk_end_time(struct sock *sk)
 {
-	return ((u64)ACCESS_ONCE(sk->sk_ll_usec) << 10) + sched_clock();
+	return ((u64)ACCESS_ONCE(sk->sk_ll_usec) << 10) + ll_sched_clock();
 }
 
 /* in poll/select we use the global sysctl_net_ll_poll value */
 static inline u64 ll_end_time(void)
 {
-	return ((u64)ACCESS_ONCE(sysctl_net_ll_poll) << 10) + sched_clock();
+	return ((u64)ACCESS_ONCE(sysctl_net_ll_poll) << 10) + ll_sched_clock();
 }
 
 static inline bool sk_valid_ll(struct sock *sk)

@@ -61,7 +81,7 @@ static inline bool sk_valid_ll(struct sock *sk)
 
 static inline bool can_poll_ll(u64 end_time)
 {
-	return !time_after64(sched_clock(), end_time);
+	return !time_after64(ll_sched_clock(), end_time);
 }
 
 /* when used in sock_poll() nonblock is known at compile time to be true

Ben Hutchings

2013-06-28 16:51 UTC #2 lore raw

Using sched_clock() for polling time limit

Quoted 2 lines · Eliezer Tamir

Our use of sched_clock is OK because we don't mind the side effects of calling it and occasionally waking up on a different CPU.

Sure about that? Jitter matters too.

Quoted 2 lines

When CONFIG_DEBUG_PREEMPT is on, disable preempt before calling sched_clock() so we don't trigger a debug_smp_processor_id() warning.

[...]

I think this is papering over a problem. The warning is there for a good reason.

Would functions like these make it possible to use sched_clock() safely for polling? (I didn't spend much time thinking about the names.)

struct sched_timestamp {
	int cpu;
	unsigned long long clock;
};

static inline struct sched_timestamp sched_get_timestamp(void)

{
	struct sched_timestamp ret;

	preempt_disable_notrace();
	ret.cpu = smp_processor_id();
	ret.clock = sched_clock();
	preempt_enable_no_resched_notrace();

	return ret;
}

static inline bool sched_timeout_or_moved(struct sched_timestamp start,
unsigned long long timeout)

{
	bool ret;

	preempt_disable_notrace();
	ret = start.cpu != smp_processor_id() ||
		(sched_clock() - start.clock) > timeout;
	preempt_enable_no_resched_notrace();

	return ret;
}

Ben.

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Eliezer Tamir

author

2013-06-29 18:50 UTC #3 lore raw

Re: Using sched_clock() for polling time limit

Quoted 6 lines · Ben Hutchings

Quoted 2 lines · Eliezer Tamir
Our use of sched_clock is OK because we don't mind the side effects of calling it and occasionally waking up on a different CPU.
Sure about that? Jitter matters too.

Pretty sure, this is a limit on how long we poll, it's for fairness to the rest of the system not for performance of this code.

What matters is that on average you are bounded by something close to what the user specified. If once in a while you run less because of clock jitter, or even twice the specified time, it's no big deal.

So I don't see how jitter would matter.

And if your workload is jitter sensitive, you should probably be pinning tasks to CPUs anyway.

Quoted 6 lines

Quoted 2 lines
When CONFIG_DEBUG_PREEMPT is on, disable preempt before calling sched_clock() so we don't trigger a debug_smp_processor_id() warning.
[...]
I think this is papering over a problem. The warning is there for a good reason.

I think we understand the warning, and that we are OK with the effects.

looking at how other users in the kernel solved this issue It seems like this is what they do.
for example trace/ring_buffer.c:ring_buffer_time_Stamp()

Also kvm_clock_read() and xen_clokcsource_read() seem to disable preempt just to silence this warning.

If they really cared about reading the value on one CPU, then being scheduled on another they should have disabled interrupts. or am I missing something?

Quoted 9 lines

Would functions like these make it possible to use sched_clock() safely for polling? (I didn't spend much time thinking about the names.)
struct sched_timestamp {
	int cpu;
	unsigned long long clock;
};
static inline struct sched_timestamp sched_get_timestamp(void)

Quoted 10 lines

{
	struct sched_timestamp ret;

	preempt_disable_notrace();
	ret.cpu = smp_processor_id();
	ret.clock = sched_clock();
	preempt_enable_no_resched_notrace();

	return ret;
}

I don't understand, preempt_disable() only makes prevents preempt from taking the CPU away from you, you could still lose it for other reasons.
You would really need to disable interrupts in this region to be sure that it all executed on the same CPU.

-Eliezer

Ben Hutchings

2013-07-01 19:48 UTC #4 lore raw

Re: Using sched_clock() for polling time limit

Quoted 5 lines · Eliezer Tamir

Quoted 4 lines · Ben Hutchings
Quoted 2 lines · Eliezer Tamir
Our use of sched_clock is OK because we don't mind the side effects of calling it and occasionally waking up on a different CPU.

Quoted 8 lines

Quoted 2 lines
Sure about that? Jitter matters too.
Pretty sure, this is a limit on how long we poll, it's for fairness to the rest of the system not for performance of this code.
What matters is that on average you are bounded by something close to what the user specified. If once in a while you run less because of clock jitter, or even twice the specified time, it's no big deal.

So what is the maximum time difference in sched_clock() values between CPUs in the same system?

Quoted 4 lines

So I don't see how jitter would matter.
And if your workload is jitter sensitive, you should probably be pinning tasks to CPUs anyway.

Yes, they should be pinned. But I think a single task that isn't pinned could poll for significantly longer than intended and the effect wouldn't be limited to that one task.

Quoted 4 lines

Quoted 4 lines
Quoted 2 lines
When CONFIG_DEBUG_PREEMPT is on, disable preempt before calling sched_clock() so we don't trigger a debug_smp_processor_id() warning.
[...]

Quoted 8 lines

Quoted 2 lines
I think this is papering over a problem. The warning is there for a good reason.
I think we understand the warning, and that we are OK with the effects.
looking at how other users in the kernel solved this issue It seems like this is what they do.
for example trace/ring_buffer.c:ring_buffer_time_Stamp()

Well, this seems to be debug information, not actually used for timing.

Quoted 2 lines

Also kvm_clock_read() and xen_clokcsource_read() seem to disable preempt just to silence this warning.

I can't see what you're referring to.

Quoted 6 lines

If they really cared about reading the value on one CPU, then being scheduled on another they should have disabled interrupts. or am I missing something?
Quoted 2 lines
Would functions like these make it possible to use sched_clock() safely for polling? (I didn't spend much time thinking about the names.)

[...]

Quoted 5 lines

I don't understand, preempt_disable() only makes prevents preempt from taking the CPU away from you, you could still lose it for other reasons.
You would really need to disable interrupts in this region to be sure that it all executed on the same CPU.

Hmm, yes you're right. Anyway, what I was trying to suggest was that you would record the current CPU along with the initial timestamp, and then abort low-latency polling if either the task is moved onto a different CPU or the time limit was reached. Checking the CPU first means that the sched_clock() comparison is valid.

Ben.

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Eliezer Tamir

author

2013-06-28 12:59 UTC #5 lore raw

PATCH 2/2 net: avoid calling sched_clock when LLS is off

In reply to Eliezer Tamir

Change Low Latency Sockets code for select and poll so that when LLS is disabled sched_clock() is never called.

Also, avoid sending POLL_LL to sockets if disabled.

Reported-by Andi Kleen <[email protected]>

Signed-off-by Eliezer Tamir <[email protected]>

---

 fs/select.c           |   11 +++++++----
 include/net/ll_poll.h |   17 +++++++++++++++--
 2 files changed, 22 insertions(+), 6 deletions(-)

fs/select.c +7 -4

index 79b876e..3654075 100644
@@ -402,7 +402,7 @@ int do_select(int n, fd_set_bits *fds, struct timespec *end_time)
 	poll_table *wait;
 	int retval, i, timed_out = 0;
 	unsigned long slack = 0;
-	unsigned int ll_flag = POLL_LL;
+	unsigned int ll_flag = ll_get_flag();
 	u64 ll_time = ll_end_time();
 
 	rcu_read_lock();

@@ -497,7 +497,8 @@ int do_select(int n, fd_set_bits *fds, struct timespec *end_time)
 			break;
 		}
 
-		if (can_ll && can_poll_ll(ll_time))
+		/* only if on, have sockets with POLL_LL and not out of time */
+		if (ll_flag && can_ll && can_poll_ll(ll_time))
 			continue;
 
 		/*

@@ -768,7 +769,7 @@ static int do_poll(unsigned int nfds,  struct poll_list *list,
 	ktime_t expire, *to = NULL;
 	int timed_out = 0, count = 0;
 	unsigned long slack = 0;
-	unsigned int ll_flag = POLL_LL;
+	unsigned int ll_flag = ll_get_flag();
 	u64 ll_time = ll_end_time();
 
 	/* Optimise the no-wait case */

@@ -817,8 +818,10 @@ static int do_poll(unsigned int nfds,  struct poll_list *list,
 		if (count || timed_out)
 			break;
 
-		if (can_ll && can_poll_ll(ll_time))
+		/* only if on, have sockets with POLL_LL and not out of time */
+		if (ll_flag && can_ll && can_poll_ll(ll_time))
 			continue;
+
 		/*
 		 * If this is the first loop and we have a timeout
 		 * given, then we convert to ktime_t and set the to

include/net/ll_poll.h +15 -2

index 6d45e6f..6c06f7c 100644
@@ -37,6 +37,11 @@ extern unsigned int sysctl_net_ll_poll __read_mostly;
 #define LL_FLUSH_FAILED		-1
 #define LL_FLUSH_BUSY		-2
 
+static inline unsigned int ll_get_flag(void)
+{
+	return sysctl_net_ll_poll ? POLL_LL : 0;
+}
+
 /* a wrapper to make debug_smp_processor_id() happy
  * we can use sched_clock() because we don't care much about precision
  * we only care that the average is bounded

@@ -67,10 +72,14 @@ static inline u64 ll_sk_end_time(struct sock *sk)
 	return ((u64)ACCESS_ONCE(sk->sk_ll_usec) << 10) + ll_sched_clock();
 }
 
-/* in poll/select we use the global sysctl_net_ll_poll value */
+/* in poll/select we use the global sysctl_net_ll_poll value
+ * only call sched_clock() if enabled
+ */
 static inline u64 ll_end_time(void)
 {
-	return ((u64)ACCESS_ONCE(sysctl_net_ll_poll) << 10) + ll_sched_clock();
+	u64 end_time = ACCESS_ONCE(sysctl_net_ll_poll);
+
+	return end_time ? (end_time << 10) + ll_sched_clock() : 0;
 }
 
 static inline bool sk_valid_ll(struct sock *sk)

@@ -141,6 +150,10 @@ static inline void sk_mark_ll(struct sock *sk, struct sk_buff *skb)
 }
 
 #else /* CONFIG_NET_LL_RX_POLL */
+static inline unsigned long ll_get_flag(void)
+{
+	return 0;
+}
 
 static inline u64 sk_ll_end_time(struct sock *sk)
 {

Andi Kleen

2013-06-28 14:38 UTC #6 lore raw

Re: [PATCH net-next 2/2] net: avoid calling sched_clock when LLS is off

fs/select.c int do_select(int n, fd_set_bits *fds, struct timespec *end_time) · line 405

Quoted 10 lines

fs/select.c +1 -1

index 79b876e..3654075 100644
@@ -402,7 +402,7 @@ int do_select(int n, fd_set_bits *fds, struct timespec *end_time)
 	poll_table *wait;
 	int retval, i, timed_out = 0;
 	unsigned long slack = 0;
-	unsigned int ll_flag = POLL_LL;
+	unsigned int ll_flag = ll_get_flag();

Is that a global flag? That's still the wrong level. It should look at something in the file descriptor (preferably without fetching any new cache lines)

-Andi

Quoted 5 lines

fs/select.c +4 -0

+static inline unsigned int ll_get_flag(void)
+{
+	return sysctl_net_ll_poll ? POLL_LL : 0;
+}

Eliezer Tamir

author

2013-06-28 14:54 UTC #7 lore raw

Re: [PATCH net-next 2/2] net: avoid calling sched_clock when LLS is off

Quoted 6 lines · Andi Kleen

Quoted 6 lines

fs/select.c +0 -0

index 79b876e..3654075 100644
@@ -402,7 +402,7 @@ int do_select(int n, fd_set_bits *fds, struct timespec *end_time)
  	poll_table *wait;

Quoted 9 lines

fs/select.c int do_select(int n, fd_set_bits *fds, struct timespec *end_time) · line 405
Quoted 4 lines
fs/select.c +1 -1
  	int retval, i, timed_out = 0;
  	unsigned long slack = 0;
-	unsigned int ll_flag = POLL_LL;
+	unsigned int ll_flag = ll_get_flag();
Is that a global flag? That's still the wrong level. It should look at something in the file descriptor (preferably without fetching any new cache lines)

There is a global flag that decides if we even try to find supported files (sockets).

If it is on we look to see if anyone will return POLL_LL. at the individual socket level, you need both the socket option to be on, and the queue data to be available for it to return POLL_LL.

I wanted to find a way to control this at the time the user calls poll(). I was not able to find any simple way of doing it that way. I'm open to suggestion.

David Miller

maintainer Applied

2013-07-01 21:08 UTC #8 lore raw

In reply to Eliezer Tamir

From: Eliezer Tamir <[email protected]>
Date: Fri, 28 Jun 2013 15:59:18 +0300

Quoted 9 lines

Here are two cleanup patches.
1. fix warning from debug_smp_processor_id().
- reported by Cody P Schafer.
2. avoid calling sched_clock() in select and poll when lls is disabled.
- reported by Andi Kleen.
Thanks to everyone for their comments.

Applied, but like Ben said perhaps you want to remember the last cpu you got the sched_clock() measurement from and abort the ll poll if it changes on you instead of using a comparison between two cpus.

But then again, since preemption is enabled, the cpu could change back and forth during the sched_clock() call, so you wouldn't be able to reliably detect this anyways.

In the grand scheme of things all of this probably doesn't matter at all.

Eliezer Tamir

author

2013-07-02 08:38 UTC #9 lore raw

Quoted 13 lines · David Miller

From: Eliezer Tamir <[email protected]>
Date: Fri, 28 Jun 2013 15:59:18 +0300
Quoted 5 lines
Here are two cleanup patches.
1. fix warning from debug_smp_processor_id().
- reported by Cody P Schafer.
Applied, but like Ben said perhaps you want to remember the last cpu you got the sched_clock() measurement from and abort the ll poll if it changes on you instead of using a comparison between two cpus.

Quoted 6 lines

But then again, since preemption is enabled, the cpu could change back and forth during the sched_clock() call, so you wouldn't be able to reliably detect this anyways.
In the grand scheme of things all of this probably doesn't matter at all.

The only thing that really worries me, is the possibility of time on the new cpu to be completely random, then we could be back in the range where time_after() will be false again and end up polling for another year.

A simple way to limit the damage would be to use time_in_range() instead of time_after(), then if we have a completely random time we would be out of the range and fail safely.

would something like this be an acceptable solution?

---
[PATCH net-next] net: convert lls to use time_in_range()

Time in range will fail safely if we move to a different cpu with an extremely large clock skew.
Add time_in_range64() and convert lls to use it.

Signed-of-by Eliezer Tamir <[email protected]>

---

 fs/select.c             |   10 ++++++----
 include/linux/jiffies.h |    4 ++++
 include/net/ll_poll.h   |   24 +++++++++++++++---------
 3 files changed, 25 insertions(+), 13 deletions(-)

fs/select.c +6 -4

index 3654075..f28a585 100644
@@ -403,7 +403,8 @@ int do_select(int n, fd_set_bits *fds, struct timespec *end_time)
 	int retval, i, timed_out = 0;
 	unsigned long slack = 0;
 	unsigned int ll_flag = ll_get_flag();
-	u64 ll_time = ll_end_time();
+	u64 ll_start = ll_start_time(ll_flag);
+	u64 ll_time = ll_run_time();
 
 	rcu_read_lock();
 	retval = max_select_fd(n, fds);

@@ -498,7 +499,7 @@ int do_select(int n, fd_set_bits *fds, struct timespec *end_time)
 		}
 
 		/* only if on, have sockets with POLL_LL and not out of time */
-		if (ll_flag && can_ll && can_poll_ll(ll_time))
+		if (ll_flag && can_ll && can_poll_ll(ll_start, ll_time))
 			continue;
 
 		/*

@@ -770,7 +771,8 @@ static int do_poll(unsigned int nfds,  struct poll_list *list,
 	int timed_out = 0, count = 0;
 	unsigned long slack = 0;
 	unsigned int ll_flag = ll_get_flag();
-	u64 ll_time = ll_end_time();
+	u64 ll_start = ll_start_time(ll_flag);
+	u64 ll_time = ll_run_time();
 
 	/* Optimise the no-wait case */
 	if (end_time && !end_time->tv_sec && !end_time->tv_nsec) {

@@ -819,7 +821,7 @@ static int do_poll(unsigned int nfds,  struct poll_list *list,
 			break;
 
 		/* only if on, have sockets with POLL_LL and not out of time */
-		if (ll_flag && can_ll && can_poll_ll(ll_time))
+		if (ll_flag && can_ll && can_poll_ll(ll_start, ll_time))
 			continue;
 
 		/*

include/linux/jiffies.h +4 -0

index 8fb8edf..37b7354 100644
@@ -139,6 +139,10 @@ static inline u64 get_jiffies_64(void)
 	 ((__s64)(a) - (__s64)(b) >= 0))
 #define time_before_eq64(a,b)	time_after_eq64(b,a)
 
+#define time_in_range64(a,b,c) \
+	(time_after_eq64(a,b) && \
+	 time_before_eq64(a,c))
+
 /*
  * These four macros compare jiffies and 'a' for convenience.
  */

include/net/ll_poll.h +15 -9

index 6c06f7c..61c2daf 100644
@@ -67,19 +67,23 @@ static inline u64 ll_sched_clock(void)
 /* we don't mind a ~2.5% imprecision so <<10 instead of *1000
  * sk->sk_ll_usec is a u_int so this can't overflow
  */
-static inline u64 ll_sk_end_time(struct sock *sk)
+static inline u64 ll_sk_run_time(struct sock *sk)
 {
-	return ((u64)ACCESS_ONCE(sk->sk_ll_usec) << 10) + ll_sched_clock();
+	return (u64)ACCESS_ONCE(sk->sk_ll_usec) << 10;
 }
 
 /* in poll/select we use the global sysctl_net_ll_poll value
  * only call sched_clock() if enabled
  */
-static inline u64 ll_end_time(void)
+static inline u64 ll_run_time(void)
 {
-	u64 end_time = ACCESS_ONCE(sysctl_net_ll_poll);
+	return (u64)ACCESS_ONCE(sysctl_net_ll_poll) << 10;
+}
 
-	return end_time ? (end_time << 10) + ll_sched_clock() : 0;
+/* if flag is not set we don't need to know the time */
+static inline u64 ll_start_time(unsigned int flag)
+{
+	return flag ? ll_sched_clock() : 0;
 }
 
 static inline bool sk_valid_ll(struct sock *sk)

@@ -88,9 +92,10 @@ static inline bool sk_valid_ll(struct sock *sk)
 	       !need_resched() && !signal_pending(current);
 }
 
-static inline bool can_poll_ll(u64 end_time)
+static inline bool can_poll_ll(u64 start_time, u64 run_time)
 {
-	return !time_after64(ll_sched_clock(), end_time);
+	return time_in_range64(ll_sched_clock(), start_time,
+			       start_time + run_time);
 }
 
 /* when used in sock_poll() nonblock is known at compile time to be true

@@ -98,7 +103,8 @@ static inline bool can_poll_ll(u64 end_time)
  */
 static inline bool sk_poll_ll(struct sock *sk, int nonblock)
 {
-	u64 end_time = nonblock ? 0 : ll_sk_end_time(sk);
+	u64 start_time = ll_start_time(!nonblock);
+	u64 run_time = ll_sk_run_time(sk);
 	const struct net_device_ops *ops;
 	struct napi_struct *napi;
 	int rc = false;

@@ -129,7 +135,7 @@ static inline bool sk_poll_ll(struct sock *sk, int nonblock)
 					 LINUX_MIB_LOWLATENCYRXPACKETS, rc);
 
 	} while (!nonblock && skb_queue_empty(&sk->sk_receive_queue) &&
-		 can_poll_ll(end_time));
+		 can_poll_ll(start_time, run_time));
 
 	rc = !skb_queue_empty(&sk->sk_receive_queue);
 out:

Eliezer Tamir

author

2013-07-02 08:45 UTC #10 lore raw

Quoted 27 lines · Eliezer Tamir

Quoted 14 lines · David Miller
From: Eliezer Tamir <[email protected]>
Date: Fri, 28 Jun 2013 15:59:18 +0300
Quoted 5 lines
Here are two cleanup patches.
1. fix warning from debug_smp_processor_id().
- reported by Cody P Schafer.
Applied, but like Ben said perhaps you want to remember the last cpu you got the sched_clock() measurement from and abort the ll poll if it changes
on you instead of using a comparison between two cpus.
Quoted 6 lines
But then again, since preemption is enabled, the cpu could change back and forth during the sched_clock() call, so you wouldn't be able to reliably detect this anyways.
In the grand scheme of things all of this probably doesn't matter at all.
The only thing that really worries me, is the possibility of time on the new cpu to be completely random, then we could be back in the range where time_after() will be false again and end up polling for another year.

Quoted 7 lines

A simple way to limit the damage would be to use time_in_range() instead of time_after(), then if we have a completely random time we would be out of the range and fail safely.
would something like this be an acceptable solution?
---

actually, this code has a bug.

include/net/ll_poll.h static inline bool sk_valid_ll(struct sock *sk) · line 99

Quoted 8 lines

include/net/ll_poll.h +3 -2

-static inline bool can_poll_ll(u64 end_time)
+static inline bool can_poll_ll(u64 start_time, u64 run_time)
  {
-    return !time_after64(ll_sched_clock(), end_time);
+    return time_in_range64(ll_sched_clock(), start_time,
+                   start_time + run_time);
  }

this will call sched_clock() twice.

I will send a fix after I test it.

-Eliezer

Eliezer Tamir

author

2013-07-02 09:49 UTC #11 lore raw

PATCH v2 1/1 net: convert lls to use time_in_range()

Time in range will fail safely if we move to a different cpu with an extremely large clock skew.
Add time_in_range64() and convert lls to use it.

Signed-off-by Eliezer Tamir <[email protected]>

---
v1->v2
fixed double call to sched_clock() in can_poll_ll(), checkpatchisms

 fs/select.c             |   10 ++++++----
 include/linux/jiffies.h |    4 ++++
 include/net/ll_poll.h   |   26 +++++++++++++++++---------
 3 files changed, 27 insertions(+), 13 deletions(-)

fs/select.c +6 -4

index 3654075..f28a585 100644
@@ -403,7 +403,8 @@ int do_select(int n, fd_set_bits *fds, struct timespec *end_time)
 	int retval, i, timed_out = 0;
 	unsigned long slack = 0;
 	unsigned int ll_flag = ll_get_flag();
-	u64 ll_time = ll_end_time();
+	u64 ll_start = ll_start_time(ll_flag);
+	u64 ll_time = ll_run_time();
 
 	rcu_read_lock();
 	retval = max_select_fd(n, fds);

@@ -498,7 +499,7 @@ int do_select(int n, fd_set_bits *fds, struct timespec *end_time)
 		}
 
 		/* only if on, have sockets with POLL_LL and not out of time */
-		if (ll_flag && can_ll && can_poll_ll(ll_time))
+		if (ll_flag && can_ll && can_poll_ll(ll_start, ll_time))
 			continue;
 
 		/*

@@ -770,7 +771,8 @@ static int do_poll(unsigned int nfds,  struct poll_list *list,
 	int timed_out = 0, count = 0;
 	unsigned long slack = 0;
 	unsigned int ll_flag = ll_get_flag();
-	u64 ll_time = ll_end_time();
+	u64 ll_start = ll_start_time(ll_flag);
+	u64 ll_time = ll_run_time();
 
 	/* Optimise the no-wait case */
 	if (end_time && !end_time->tv_sec && !end_time->tv_nsec) {

@@ -819,7 +821,7 @@ static int do_poll(unsigned int nfds,  struct poll_list *list,
 			break;
 
 		/* only if on, have sockets with POLL_LL and not out of time */
-		if (ll_flag && can_ll && can_poll_ll(ll_time))
+		if (ll_flag && can_ll && can_poll_ll(ll_start, ll_time))
 			continue;
 
 		/*

include/linux/jiffies.h +4 -0

index 8fb8edf..97ba4e7 100644
@@ -139,6 +139,10 @@ static inline u64 get_jiffies_64(void)
 	 ((__s64)(a) - (__s64)(b) >= 0))
 #define time_before_eq64(a,b)	time_after_eq64(b,a)
 
+#define time_in_range64(a, b, c) \
+	(time_after_eq64(a, b) && \
+	 time_before_eq64(a, c))
+
 /*
  * These four macros compare jiffies and 'a' for convenience.
  */

include/net/ll_poll.h +17 -9

index 6c06f7c..b76f004 100644
@@ -67,19 +67,23 @@ static inline u64 ll_sched_clock(void)
 /* we don't mind a ~2.5% imprecision so <<10 instead of *1000
  * sk->sk_ll_usec is a u_int so this can't overflow
  */
-static inline u64 ll_sk_end_time(struct sock *sk)
+static inline u64 ll_sk_run_time(struct sock *sk)
 {
-	return ((u64)ACCESS_ONCE(sk->sk_ll_usec) << 10) + ll_sched_clock();
+	return (u64)ACCESS_ONCE(sk->sk_ll_usec) << 10;
 }
 
 /* in poll/select we use the global sysctl_net_ll_poll value
  * only call sched_clock() if enabled
  */
-static inline u64 ll_end_time(void)
+static inline u64 ll_run_time(void)
 {
-	u64 end_time = ACCESS_ONCE(sysctl_net_ll_poll);
+	return (u64)ACCESS_ONCE(sysctl_net_ll_poll) << 10;
+}
 
-	return end_time ? (end_time << 10) + ll_sched_clock() : 0;
+/* if flag is not set we don't need to know the time */
+static inline u64 ll_start_time(unsigned int flag)
+{
+	return flag ? ll_sched_clock() : 0;
 }
 
 static inline bool sk_valid_ll(struct sock *sk)

@@ -88,9 +92,12 @@ static inline bool sk_valid_ll(struct sock *sk)
 	       !need_resched() && !signal_pending(current);
 }
 
-static inline bool can_poll_ll(u64 end_time)
+/* careful! time_in_range64 will evaluate now twice */
+static inline bool can_poll_ll(u64 start_time, u64 run_time)
 {
-	return !time_after64(ll_sched_clock(), end_time);
+	u64 now = ll_sched_clock();
+
+	return time_in_range64(now, start_time, start_time + run_time);
 }
 
 /* when used in sock_poll() nonblock is known at compile time to be true

@@ -98,7 +105,8 @@ static inline bool can_poll_ll(u64 end_time)
  */
 static inline bool sk_poll_ll(struct sock *sk, int nonblock)
 {
-	u64 end_time = nonblock ? 0 : ll_sk_end_time(sk);
+	u64 start_time = ll_start_time(!nonblock);
+	u64 run_time = ll_sk_run_time(sk);
 	const struct net_device_ops *ops;
 	struct napi_struct *napi;
 	int rc = false;

@@ -129,7 +137,7 @@ static inline bool sk_poll_ll(struct sock *sk, int nonblock)
 					 LINUX_MIB_LOWLATENCYRXPACKETS, rc);
 
 	} while (!nonblock && skb_queue_empty(&sk->sk_receive_queue) &&
-		 can_poll_ll(end_time));
+		 can_poll_ll(start_time, run_time));
 
 	rc = !skb_queue_empty(&sk->sk_receive_queue);
 out:

David Miller

maintainer

2013-07-02 19:56 UTC #12 lore raw

Re: [PATCH v2 net-next] net: convert lls to use time_in_range()

From: Eliezer Tamir <[email protected]>
Date: Tue, 02 Jul 2013 12:49:58 +0300

Review int do_select(int n, fd_set_bits *fds, struct · line 403

Quoted 2 lines

(quoted diff) +0 -0

@@ -403,7 +403,8 @@ int do_select(int n, fd_set_bits *fds, struct
 timespec *end_time)

Your email client has corrupted this patch, please repost with this issue corrected.

Thanks.

Ben Hutchings

2013-07-02 20:10 UTC #13 lore raw

Re: [PATCH v2 net-next] net: convert lls to use time_in_range()

In reply to Eliezer Tamir

Quoted 8 lines · Eliezer Tamir

Time in range will fail safely if we move to a different cpu with an extremely large clock skew.
Add time_in_range64() and convert lls to use it.
Signed-off-by Eliezer Tamir <[email protected]>
---
v1->v2
fixed double call to sched_clock() in can_poll_ll(), checkpatchisms

[...]

include/linux/jiffies.h static inline u64 get_jiffies_64(void) · line 144

Quoted 9 lines

include/linux/jiffies.h +3 -0

@@ -139,6 +139,10 @@ static inline u64 get_jiffies_64(void)
  	 ((__s64)(a) - (__s64)(b) >= 0))
  #define time_before_eq64(a,b)	time_after_eq64(b,a)
 
+#define time_in_range64(a, b, c) \
+	(time_after_eq64(a, b) && \
+	 time_before_eq64(a, c))

[...]

Why not make this an inline function, so the caller doesn't need to worry about repeated evaluation?

Ben.

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Eliezer Tamir

author

2013-07-02 20:28 UTC #14 lore raw

Re: [PATCH v2 net-next] net: convert lls to use time_in_range()

Quoted 10 lines · Ben Hutchings

Quoted 8 lines · Eliezer Tamir
Time in range will fail safely if we move to a different cpu with an extremely large clock skew.
Add time_in_range64() and convert lls to use it.
Signed-off-by Eliezer Tamir <[email protected]>
---
v1->v2
fixed double call to sched_clock() in can_poll_ll(), checkpatchisms

Quoted 7 lines

include/linux/jiffies.h static inline u64 get_jiffies_64(void) · line 144
Quoted 3 lines
include/linux/jiffies.h +3 -0
+#define time_in_range64(a, b, c) \
+	(time_after_eq64(a, b) && \
+	 time_before_eq64(a, c))
[...]
Why not make this an inline function, so the caller doesn't need to worry about repeated evaluation?

I was following the conventions in jiffies.h
(well almost, I did add a few spaces to make checkpatch happy)

Ben Hutchings

2013-07-02 20:42 UTC #15 lore raw

Re: [PATCH v2 net-next] net: convert lls to use time_in_range()

Quoted 16 lines · Eliezer Tamir

Quoted 15 lines · Ben Hutchings
Quoted 12 lines · Eliezer Tamir
Time in range will fail safely if we move to a different cpu with an extremely large clock skew.
Add time_in_range64() and convert lls to use it.
Signed-off-by Eliezer Tamir <[email protected]>
---
v1->v2
fixed double call to sched_clock() in can_poll_ll(), checkpatchisms
include/linux/jiffies.h +3 -0
+#define time_in_range64(a, b, c) \
+	(time_after_eq64(a, b) && \
+	 time_before_eq64(a, c))
[...]

Quoted 5 lines

Quoted 2 lines
Why not make this an inline function, so the caller doesn't need to worry about repeated evaluation?
I was following the conventions in jiffies.h
(well almost, I did add a few spaces to make checkpatch happy)

I see, but now you have a good reason to change that convention.

Ben.

Signature · 3 lines

Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Eliezer Tamir

author

2013-07-03 07:00 UTC #16 lore raw

Re: [PATCH v2 net-next] net: convert lls to use time_in_range()

Quoted 17 lines · Ben Hutchings

Quoted 16 lines · Eliezer Tamir
Quoted 15 lines · Ben Hutchings
Quoted 12 lines · Eliezer Tamir
Time in range will fail safely if we move to a different cpu with an extremely large clock skew.
Add time_in_range64() and convert lls to use it.
Signed-off-by Eliezer Tamir <[email protected]>
---
v1->v2
fixed double call to sched_clock() in can_poll_ll(), checkpatchisms
include/linux/jiffies.h +3 -0
+#define time_in_range64(a, b, c) \
+	(time_after_eq64(a, b) && \
+	 time_before_eq64(a, c))
[...]

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Why not make this an inline function, so the caller doesn't need to worry about repeated evaluation?
I was following the conventions in jiffies.h
(well almost, I did add a few spaces to make checkpatch happy)
I see, but now you have a good reason to change that convention.

I'm not sure that an inline function is always a win. Macros do get evaluated at an earlier stage.

Having just the new function use a different convention is plain wrong, people are virtually guarantied to not notice until they introduce a bug.

Are you suggesting we convert all of jiffies.h into inline functions?

Invoking the principle of least astonishment, I think it's best to keep the changes minimal.

Or would you prefer to risk breaking things all over in subtle ways that are sure to get Linus to go voodoo on your hamster?

I for one care about hamsters.

-Eliezer