[PATCH net-next 0/5] SCTP update

[PATCH net-next 0/5] SCTP update

[Daniel Borkmann]

This set contains transport path selection improvements in
SCTP. Please see individual patches for details.

Thanks!

Daniel Borkmann (5):
  ktime: add ktime_after and ktime_before helper
  net: sctp: refactor active path selection
  net: sctp: migrate most recently used transport to ktime
  net: sctp: improve sctp_select_active_and_retran_path selection
  net: sctp: fix incorrect type in gfp initializer

 include/linux/ktime.h      |  24 +++++++
 include/net/sctp/structs.h |   6 +-
 net/sctp/associola.c       | 168 ++++++++++++++++++++++++++++-----------------
 net/sctp/endpointola.c     |   2 +-
 net/sctp/sm_make_chunk.c   |   2 +-
 net/sctp/transport.c       |   2 +-
 6 files changed, 136 insertions(+), 68 deletions(-)

-- 
1.7.11.7

[PATCH net-next 1/5] ktime: add ktime_after and ktime_before helper

[Daniel Borkmann]

Add two minimal helper functions analogous to time_before() and
time_after() that will later on both be needed by SCTP code.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/linux/ktime.h    | 24 ++++++++++++++++++++++++
 net/sctp/sm_make_chunk.c |  2 +-
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/include/linux/ktime.h b/include/linux/ktime.h
index 31c0cd1..3787f33 100644
--- a/include/linux/ktime.h
+++ b/include/linux/ktime.h
@@ -304,6 +304,30 @@ static inline int ktime_compare(const ktime_t cmp1, const ktime_t cmp2)
 	return 0;
 }
 
+/**
+ * ktime_after - Compare if a ktime_t value is bigger than another one.
+ * @cmp1:	comparable1
+ * @cmp2:	comparable2
+ *
+ * Return: true if cmp1 happened after cmp2.
+ */
+static inline bool ktime_after(const ktime_t cmp1, const ktime_t cmp2)
+{
+	return ktime_compare(cmp1, cmp2) > 0;
+}
+
+/**
+ * ktime_after - Compare if a ktime_t value is smaller than another one.
+ * @cmp1:	comparable1
+ * @cmp2:	comparable2
+ *
+ * Return: true if cmp1 happened before cmp2.
+ */
+static inline bool ktime_before(const ktime_t cmp1, const ktime_t cmp2)
+{
+	return ktime_compare(cmp1, cmp2) < 0;
+}
+
 static inline s64 ktime_to_us(const ktime_t kt)
 {
 	struct timeval tv = ktime_to_timeval(kt);
diff --git a/net/sctp/sm_make_chunk.c b/net/sctp/sm_make_chunk.c
index fee5552..ae0e616 100644
--- a/net/sctp/sm_make_chunk.c
+++ b/net/sctp/sm_make_chunk.c
@@ -1782,7 +1782,7 @@ no_hmac:
 	else
 		kt = ktime_get();
 
-	if (!asoc && ktime_compare(bear_cookie->expiration, kt) < 0) {
+	if (!asoc && ktime_before(bear_cookie->expiration, kt)) {
 		/*
 		 * Section 3.3.10.3 Stale Cookie Error (3)
 		 *
-- 
1.7.11.7

[PATCH net-next 2/5] net: sctp: refactor active path selection

[Daniel Borkmann]

This patch just refactors and moves the code for the active
path selection into its own helper function outside of
sctp_assoc_control_transport() which is already big enough.
No functional changes here.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/associola.c | 120 ++++++++++++++++++++++++++-------------------------
 1 file changed, 61 insertions(+), 59 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 39579c3..9f1cc6f 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -55,6 +55,7 @@
 #include <net/sctp/sm.h>
 
 /* Forward declarations for internal functions. */
+static void sctp_select_active_and_retran_path(struct sctp_association *asoc);
 static void sctp_assoc_bh_rcv(struct work_struct *work);
 static void sctp_assoc_free_asconf_acks(struct sctp_association *asoc);
 static void sctp_assoc_free_asconf_queue(struct sctp_association *asoc);
@@ -774,9 +775,6 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 				  sctp_transport_cmd_t command,
 				  sctp_sn_error_t error)
 {
-	struct sctp_transport *t = NULL;
-	struct sctp_transport *first;
-	struct sctp_transport *second;
 	struct sctp_ulpevent *event;
 	struct sockaddr_storage addr;
 	int spc_state = 0;
@@ -829,13 +827,14 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 		return;
 	}
 
-	/* Generate and send a SCTP_PEER_ADDR_CHANGE notification to the
-	 * user.
+	/* Generate and send a SCTP_PEER_ADDR_CHANGE notification
+	 * to the user.
 	 */
 	if (ulp_notify) {
 		memset(&addr, 0, sizeof(struct sockaddr_storage));
 		memcpy(&addr, &transport->ipaddr,
 		       transport->af_specific->sockaddr_len);
+
 		event = sctp_ulpevent_make_peer_addr_change(asoc, &addr,
 					0, spc_state, error, GFP_ATOMIC);
 		if (event)
@@ -843,60 +842,7 @@ void sctp_assoc_control_transport(struct sctp_association *asoc,
 	}
 
 	/* Select new active and retran paths. */
-
-	/* Look for the two most recently used active transports.
-	 *
-	 * This code produces the wrong ordering whenever jiffies
-	 * rolls over, but we still get usable transports, so we don't
-	 * worry about it.
-	 */
-	first = NULL; second = NULL;
-
-	list_for_each_entry(t, &asoc->peer.transport_addr_list,
-			transports) {
-
-		if ((t->state == SCTP_INACTIVE) ||
-		    (t->state == SCTP_UNCONFIRMED) ||
-		    (t->state == SCTP_PF))
-			continue;
-		if (!first || t->last_time_heard > first->last_time_heard) {
-			second = first;
-			first = t;
-		} else if (!second ||
-			   t->last_time_heard > second->last_time_heard)
-			second = t;
-	}
-
-	/* RFC 2960 6.4 Multi-Homed SCTP Endpoints
-	 *
-	 * By default, an endpoint should always transmit to the
-	 * primary path, unless the SCTP user explicitly specifies the
-	 * destination transport address (and possibly source
-	 * transport address) to use.
-	 *
-	 * [If the primary is active but not most recent, bump the most
-	 * recently used transport.]
-	 */
-	if (((asoc->peer.primary_path->state == SCTP_ACTIVE) ||
-	     (asoc->peer.primary_path->state == SCTP_UNKNOWN)) &&
-	    first != asoc->peer.primary_path) {
-		second = first;
-		first = asoc->peer.primary_path;
-	}
-
-	if (!second)
-		second = first;
-	/* If we failed to find a usable transport, just camp on the
-	 * primary, even if it is inactive.
-	 */
-	if (!first) {
-		first = asoc->peer.primary_path;
-		second = asoc->peer.primary_path;
-	}
-
-	/* Set the active and retran transports.  */
-	asoc->peer.active_path = first;
-	asoc->peer.retran_path = second;
+	sctp_select_active_and_retran_path(asoc);
 }
 
 /* Hold a reference to an association. */
@@ -1325,6 +1271,62 @@ void sctp_assoc_update_retran_path(struct sctp_association *asoc)
 		 __func__, asoc, &asoc->peer.retran_path->ipaddr.sa);
 }
 
+static void sctp_select_active_and_retran_path(struct sctp_association *asoc)
+{
+	struct sctp_transport *trans, *trans_pri = NULL, *trans_sec = NULL;
+
+	/* Look for the two most recently used active transports. */
+	list_for_each_entry(trans, &asoc->peer.transport_addr_list,
+			    transports) {
+		if (trans->state == SCTP_INACTIVE ||
+		    trans->state == SCTP_UNCONFIRMED ||
+		    trans->state == SCTP_PF)
+			continue;
+		if (trans_pri == NULL ||
+		    trans->last_time_heard > trans_pri->last_time_heard) {
+			trans_sec = trans_pri;
+			trans_pri = trans;
+		} else if (trans_sec == NULL ||
+			   trans->last_time_heard > trans_sec->last_time_heard) {
+			trans_sec = trans;
+		}
+	}
+
+	/* RFC 2960 6.4 Multi-Homed SCTP Endpoints
+	 *
+	 * By default, an endpoint should always transmit to the primary
+	 * path, unless the SCTP user explicitly specifies the
+	 * destination transport address (and possibly source transport
+	 * address) to use. [If the primary is active but not most recent,
+	 * bump the most recently used transport.]
+	 */
+	if ((asoc->peer.primary_path->state == SCTP_ACTIVE ||
+	     asoc->peer.primary_path->state == SCTP_UNKNOWN) &&
+	     asoc->peer.primary_path != trans_pri) {
+		trans_sec = trans_pri;
+		trans_pri = asoc->peer.primary_path;
+	}
+
+	/* We did not find anything useful for a possible retransmission
+	 * path; either primary path that we found is the the same as
+	 * the current one, or we didn't generally find an active one.
+	 */
+	if (trans_sec == NULL)
+		trans_sec = trans_pri;
+
+	/* If we failed to find a usable transport, just camp on the
+	 * primary, even if they are inactive.
+	 */
+	if (trans_pri == NULL) {
+		trans_pri = asoc->peer.primary_path;
+		trans_sec = asoc->peer.primary_path;
+	}
+
+	/* Set the active and retran transports. */
+	asoc->peer.active_path = trans_pri;
+	asoc->peer.retran_path = trans_sec;
+}
+
 struct sctp_transport *
 sctp_assoc_choose_alter_transport(struct sctp_association *asoc,
 				  struct sctp_transport *last_sent_to)
-- 
1.7.11.7

[PATCH net-next 3/5] net: sctp: migrate most recently used transport to ktime

[Daniel Borkmann]

Be more precise in transport path selection and use ktime
helpers instead of jiffies to compare and pick the better
primary and secondary recently used transports. This also
avoids any side-effects during a possible roll-over, and
could lead to better path decision-making.

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 include/net/sctp/structs.h | 6 +++---
 net/sctp/associola.c       | 8 +++++---
 net/sctp/endpointola.c     | 2 +-
 net/sctp/transport.c       | 2 +-
 4 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/net/sctp/structs.h b/include/net/sctp/structs.h
index 0dfcc92..f38588b 100644
--- a/include/net/sctp/structs.h
+++ b/include/net/sctp/structs.h
@@ -838,10 +838,10 @@ struct sctp_transport {
 	unsigned long sackdelay;
 	__u32 sackfreq;
 
-	/* When was the last time (in jiffies) that we heard from this
-	 * transport?  We use this to pick new active and retran paths.
+	/* When was the last time that we heard from this transport? We use
+	 * this to pick new active and retran paths.
 	 */
-	unsigned long last_time_heard;
+	ktime_t last_time_heard;
 
 	/* Last time(in jiffies) when cwnd is reduced due to the congestion
 	 * indication based on ECNE chunk.
diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 9f1cc6f..620c99e 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1036,7 +1036,7 @@ static void sctp_assoc_bh_rcv(struct work_struct *work)
 		}
 
 		if (chunk->transport)
-			chunk->transport->last_time_heard = jiffies;
+			chunk->transport->last_time_heard = ktime_get();
 
 		/* Run through the state machine. */
 		error = sctp_do_sm(net, SCTP_EVENT_T_CHUNK, subtype,
@@ -1283,11 +1283,13 @@ static void sctp_select_active_and_retran_path(struct sctp_association *asoc)
 		    trans->state == SCTP_PF)
 			continue;
 		if (trans_pri == NULL ||
-		    trans->last_time_heard > trans_pri->last_time_heard) {
+		    ktime_after(trans->last_time_heard,
+				trans_pri->last_time_heard)) {
 			trans_sec = trans_pri;
 			trans_pri = trans;
 		} else if (trans_sec == NULL ||
-			   trans->last_time_heard > trans_sec->last_time_heard) {
+			   ktime_after(trans->last_time_heard,
+				       trans_sec->last_time_heard)) {
 			trans_sec = trans;
 		}
 	}
diff --git a/net/sctp/endpointola.c b/net/sctp/endpointola.c
index 3d9f429..9da76ba 100644
--- a/net/sctp/endpointola.c
+++ b/net/sctp/endpointola.c
@@ -481,7 +481,7 @@ normal:
 		}
 
 		if (chunk->transport)
-			chunk->transport->last_time_heard = jiffies;
+			chunk->transport->last_time_heard = ktime_get();
 
 		error = sctp_do_sm(net, SCTP_EVENT_T_CHUNK, subtype, state,
 				   ep, asoc, chunk, GFP_ATOMIC);
diff --git a/net/sctp/transport.c b/net/sctp/transport.c
index 1d348d1..7dd672f 100644
--- a/net/sctp/transport.c
+++ b/net/sctp/transport.c
@@ -72,7 +72,7 @@ static struct sctp_transport *sctp_transport_init(struct net *net,
 	 */
 	peer->rto = msecs_to_jiffies(net->sctp.rto_initial);
 
-	peer->last_time_heard = jiffies;
+	peer->last_time_heard = ktime_get();
 	peer->last_time_ecne_reduced = jiffies;
 
 	peer->param_flags = SPP_HB_DISABLE |
-- 
1.7.11.7

[PATCH net-next 4/5] net: sctp: improve sctp_select_active_and_retran_path selection

[Daniel Borkmann]

In case we neither found a possible ACTIVE or UNKNOWN trans_pri
resp. trans_sec from our current transport list, we currently
just camp on a possibly PF or INACTIVE transport that is primary
path; this behaviour actually dates back to linux-history tree
of the very early days of lksctp, and can yield a behaviour that
chooses suboptimal transport paths.

Instead, be a bit more clever by reusing and extending the
recently introduced sctp_trans_elect_best() handler: In case of
a tie, that is, both transports that are to be evaluated have
the same score resulting from same states, will be examined
further in that case based on other properties, that is, 1) we
look at the transport path error_count, and if also that is a
tie, 2) we look at the last_time_heard. This is analogous to
Nishida's Quick Failover draft, section 5.1, 3:

  The sender SHOULD avoid data transmission to PF destinations.
  When all destinations are in either PF or Inactive state,
  the sender MAY either move the destination from PF to active
  state (and transmit data to the active destination) or the
  sender MAY transmit data to a PF destination. In the former
  scenario, (i) the sender MUST NOT notify the ULP about the
  state transition, and (ii) MUST NOT clear the destination's
  error counter. It is recommended that the sender picks the
  PF destination with least error count (fewest consecutive
  timeouts) for data transmission. In case of a tie (multiple PF
  destinations with same error count), the sender MAY choose the
  last active destination.

Thus for sctp_select_active_and_retran_path(), we keep track of
the best, if any, transport in PF state and in case no ACTIVE or
UNKNOWN transport has been found (hence trans_{pri,sec} is NULL)
we select the best out of the current primary_path and retran_path
as well as the best found PF transport via sctp_trans_elect_best().
The secondary may still camp on the original primary_path as
before. The change in sctp_trans_elect_best() with a more fine
grained tie selection also improves at the same time path selection
for sctp_assoc_update_retran_path() in cases of non-ACTIVE states.

  [1] http://tools.ietf.org/html/draft-nishida-tsvwg-sctp-failover-05

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/associola.c | 50 +++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 45 insertions(+), 5 deletions(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 620c99e..58bbb73 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1224,13 +1224,41 @@ static u8 sctp_trans_score(const struct sctp_transport *trans)
 	return sctp_trans_state_to_prio_map[trans->state];
 }
 
+static struct sctp_transport *sctp_trans_elect_tie(struct sctp_transport *trans1,
+						   struct sctp_transport *trans2)
+{
+	if (trans1->error_count > trans2->error_count) {
+		return trans2;
+	} else if (trans1->error_count == trans2->error_count &&
+		   ktime_after(trans2->last_time_heard,
+			       trans1->last_time_heard)) {
+		return trans2;
+	} else {
+		return trans1;
+	}
+}
+
 static struct sctp_transport *sctp_trans_elect_best(struct sctp_transport *curr,
 						    struct sctp_transport *best)
 {
+	u8 score_curr, score_best;
+
 	if (best == NULL)
 		return curr;
 
-	return sctp_trans_score(curr) > sctp_trans_score(best) ? curr : best;
+	score_curr = sctp_trans_score(curr);
+	score_best = sctp_trans_score(best);
+
+	/* First, try a score-based selection if both transport states
+	 * differ. If we're in a tie, lets try to make a more clever
+	 * decision here based on error counts and last time heard.
+	 */
+	if (score_curr > score_best)
+		return curr;
+	else if (score_curr == score_best)
+		return sctp_trans_elect_tie(curr, best);
+	else
+		return best;
 }
 
 void sctp_assoc_update_retran_path(struct sctp_association *asoc)
@@ -1274,14 +1302,23 @@ void sctp_assoc_update_retran_path(struct sctp_association *asoc)
 static void sctp_select_active_and_retran_path(struct sctp_association *asoc)
 {
 	struct sctp_transport *trans, *trans_pri = NULL, *trans_sec = NULL;
+	struct sctp_transport *trans_pf = NULL;
 
 	/* Look for the two most recently used active transports. */
 	list_for_each_entry(trans, &asoc->peer.transport_addr_list,
 			    transports) {
+		/* Skip uninteresting transports. */
 		if (trans->state == SCTP_INACTIVE ||
-		    trans->state == SCTP_UNCONFIRMED ||
-		    trans->state == SCTP_PF)
+		    trans->state == SCTP_UNCONFIRMED)
 			continue;
+		/* Keep track of the best PF transport from our
+		 * list in case we don't find an active one.
+		 */
+		if (trans->state == SCTP_PF) {
+			trans_pf = sctp_trans_elect_best(trans, trans_pf);
+			continue;
+		}
+		/* For active transports, pick the most recent ones. */
 		if (trans_pri == NULL ||
 		    ktime_after(trans->last_time_heard,
 				trans_pri->last_time_heard)) {
@@ -1317,10 +1354,13 @@ static void sctp_select_active_and_retran_path(struct sctp_association *asoc)
 		trans_sec = trans_pri;
 
 	/* If we failed to find a usable transport, just camp on the
-	 * primary, even if they are inactive.
+	 * primary or retran, even if they are inactive, if possible
+	 * pick a PF iff it's the better choice.
 	 */
 	if (trans_pri == NULL) {
-		trans_pri = asoc->peer.primary_path;
+		trans_pri = sctp_trans_elect_best(asoc->peer.primary_path,
+						  asoc->peer.retran_path);
+		trans_pri = sctp_trans_elect_best(trans_pri, trans_pf);
 		trans_sec = asoc->peer.primary_path;
 	}
 
-- 
1.7.11.7

[PATCH net-next 5/5] net: sctp: fix incorrect type in gfp initializer

[Daniel Borkmann]

This fixes the following sparse warning:

  net/sctp/associola.c:1556:29: warning: incorrect type in initializer (different base types)
  net/sctp/associola.c:1556:29:    expected bool [unsigned] [usertype] preload
  net/sctp/associola.c:1556:29:    got restricted gfp_t

Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
---
 net/sctp/associola.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/sctp/associola.c b/net/sctp/associola.c
index 58bbb73..9e0509c 100644
--- a/net/sctp/associola.c
+++ b/net/sctp/associola.c
@@ -1591,7 +1591,7 @@ int sctp_assoc_lookup_laddr(struct sctp_association *asoc,
 /* Set an association id for a given association */
 int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
 {
-	bool preload = gfp & __GFP_WAIT;
+	bool preload = !!(gfp & __GFP_WAIT);
 	int ret;
 
 	/* If the id is already assigned, keep it. */
-- 
1.7.11.7

RE: [PATCH net-next 5/5] net: sctp: fix incorrect type in gfp initializer

[David Laight]

From: Daniel Borkmann
> This fixes the following sparse warning:
> 
>   net/sctp/associola.c:1556:29: warning: incorrect type in initializer (different base types)
>   net/sctp/associola.c:1556:29:    expected bool [unsigned] [usertype] preload
>   net/sctp/associola.c:1556:29:    got restricted gfp_t
...
> --- a/net/sctp/associola.c
> +++ b/net/sctp/associola.c
> @@ -1591,7 +1591,7 @@ int sctp_assoc_lookup_laddr(struct sctp_association *asoc,
>  /* Set an association id for a given association */
>  int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
>  {
> -	bool preload = gfp & __GFP_WAIT;
> +	bool preload = !!(gfp & __GFP_WAIT);
>  	int ret;
> 
>  	/* If the id is already assigned, keep it. */
> --

I was wondering if the compiler still manages to optimise this in a
manner that avoids actually calculating the boolean value...

So I disassembled the compilation I just did of the old code (gcc 4.7.3).
The object code looks strange.
I think that idr_preload_end() must be an empty inline function.
The compiler has duplicated the code between the two 'if (preload)'
clauses (to avoid the conditional test), and the failed to tail
merge everything in the latter stages.
I suspect that an empty '#define' would generate smaller code.

	David

Re: [PATCH net-next 5/5] net: sctp: fix incorrect type in gfp initializer

[Daniel Borkmann]

On 06/11/2014 04:55 PM, David Laight wrote:
> From: Daniel Borkmann
>> This fixes the following sparse warning:
>>
>>    net/sctp/associola.c:1556:29: warning: incorrect type in initializer (different base types)
>>    net/sctp/associola.c:1556:29:    expected bool [unsigned] [usertype] preload
>>    net/sctp/associola.c:1556:29:    got restricted gfp_t
> ...
>> --- a/net/sctp/associola.c
>> +++ b/net/sctp/associola.c
>> @@ -1591,7 +1591,7 @@ int sctp_assoc_lookup_laddr(struct sctp_association *asoc,
>>   /* Set an association id for a given association */
>>   int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
>>   {
>> -	bool preload = gfp & __GFP_WAIT;
>> +	bool preload = !!(gfp & __GFP_WAIT);
>>   	int ret;
>>
>>   	/* If the id is already assigned, keep it. */
>> --
>
> I was wondering if the compiler still manages to optimise this in a
> manner that avoids actually calculating the boolean value...
>
> So I disassembled the compilation I just did of the old code (gcc 4.7.3).
> The object code looks strange.
> I think that idr_preload_end() must be an empty inline function.

Cc'ing Tejun. ;-)

> The compiler has duplicated the code between the two 'if (preload)'
> clauses (to avoid the conditional test), and the failed to tail
> merge everything in the latter stages.
> I suspect that an empty '#define' would generate smaller code.
>
> 	David
>
>
>

Re: [PATCH net-next 5/5] net: sctp: fix incorrect type in gfp initializer

[Alexei Starovoitov]

On Wed, Jun 11, 2014 at 8:09 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> On 06/11/2014 04:55 PM, David Laight wrote:
>>
>> From: Daniel Borkmann
>>>
>>> This fixes the following sparse warning:
>>>
>>>    net/sctp/associola.c:1556:29: warning: incorrect type in initializer
>>> (different base types)
>>>    net/sctp/associola.c:1556:29:    expected bool [unsigned] [usertype]
>>> preload
>>>    net/sctp/associola.c:1556:29:    got restricted gfp_t
>>
>> ...
>>>
>>> --- a/net/sctp/associola.c
>>> +++ b/net/sctp/associola.c
>>> @@ -1591,7 +1591,7 @@ int sctp_assoc_lookup_laddr(struct sctp_association
>>> *asoc,
>>>   /* Set an association id for a given association */
>>>   int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
>>>   {
>>> -       bool preload = gfp & __GFP_WAIT;
>>> +       bool preload = !!(gfp & __GFP_WAIT);
>>>         int ret;
>>>
>>>         /* If the id is already assigned, keep it. */
>>> --
>>
>>
>> I was wondering if the compiler still manages to optimise this in a
>> manner that avoids actually calculating the boolean value...
>>
>> So I disassembled the compilation I just did of the old code (gcc 4.7.3).
>> The object code looks strange.

I'm not sure where you see this.
Just tried with gcc 4.7.2 on x64 and assembler code is exactly the
same before/after this change.

>> I think that idr_preload_end() must be an empty inline function.
>
>
> Cc'ing Tejun. ;-)
>
>
>> The compiler has duplicated the code between the two 'if (preload)'
>> clauses (to avoid the conditional test), and the failed to tail
>> merge everything in the latter stages.
>> I suspect that an empty '#define' would generate smaller code.
>>
>>         David
>>
>>
>>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH net-next 5/5] net: sctp: fix incorrect type in gfp initializer

[David Laight]

From: Alexei Starovoitov
> On Wed, Jun 11, 2014 at 8:09 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> > On 06/11/2014 04:55 PM, David Laight wrote:
> >>
> >> From: Daniel Borkmann
...
> >>> --- a/net/sctp/associola.c
> >>> +++ b/net/sctp/associola.c
> >>> @@ -1591,7 +1591,7 @@ int sctp_assoc_lookup_laddr(struct sctp_association
> >>> *asoc,
> >>>   /* Set an association id for a given association */
> >>>   int sctp_assoc_set_id(struct sctp_association *asoc, gfp_t gfp)
> >>>   {
> >>> -       bool preload = gfp & __GFP_WAIT;
> >>> +       bool preload = !!(gfp & __GFP_WAIT);
> >>>         int ret;
> >>>
> >>>         /* If the id is already assigned, keep it. */
> >>> --
> >>
> >>
> >> I was wondering if the compiler still manages to optimise this in a
> >> manner that avoids actually calculating the boolean value...
> >>
> >> So I disassembled the compilation I just did of the old code (gcc 4.7.3).
> >> The object code looks strange.
> 
> I'm not sure where you see this.
> Just tried with gcc 4.7.2 on x64 and assembler code is exactly the
> same before/after this change.

I was slightly worried it might generate the boolean value - something
that you really don't want it to do.

I only looked at the output for the old version.
The compiler seemed to have converted:
	if (preload)
		x();
	y;
	if (preload)
		z();
into:
	if (preload) {
		x(); y; z();
	} else {
		y;
	}
and then found out that z() was empty, leaving two copies of y().

	David

> >> I think that idr_preload_end() must be an empty inline function.
> >
> >
> > Cc'ing Tejun. ;-)
> >
> >
> >> The compiler has duplicated the code between the two 'if (preload)'
> >> clauses (to avoid the conditional test), and the failed to tail
> >> merge everything in the latter stages.
> >> I suspect that an empty '#define' would generate smaller code.
> >>
> >>         David

Re: [PATCH net-next 5/5] net: sctp: fix incorrect type in gfp initializer

[Tejun Heo]

On Thu, Jun 12, 2014 at 08:46:44AM +0000, David Laight wrote:
> I was slightly worried it might generate the boolean value - something
> that you really don't want it to do.

I don't think gcc is that stupid at this point.

> I only looked at the output for the old version.
> The compiler seemed to have converted:
> 	if (preload)
> 		x();
> 	y;
> 	if (preload)
> 		z();
> into:
> 	if (preload) {
> 		x(); y; z();
> 	} else {
> 		y;
> 	}
> and then found out that z() was empty, leaving two copies of y().

So, nothing wrong, right?

-- 
tejun

RE: [PATCH net-next 5/5] net: sctp: fix incorrect type in gfp initializer

[David Laight]

From: Tejun Heo
> On Thu, Jun 12, 2014 at 08:46:44AM +0000, David Laight wrote:
> > I was slightly worried it might generate the boolean value - something
> > that you really don't want it to do.
> 
> I don't think gcc is that stupid at this point.
> 
> > I only looked at the output for the old version.
> > The compiler seemed to have converted:
> > 	if (preload)
> > 		x();
> > 	y;
> > 	if (preload)
> > 		z();
> > into:
> > 	if (preload) {
> > 		x(); y; z();
> > 	} else {
> > 		y;
> > 	}
> > and then found out that z() was empty, leaving two copies of y().
> 
> So, nothing wrong, right?

Apart from the fact that it is badly optimised.

If z() were an empty #define rather than an empty inline function
then you'd end up with the:
	if (preload)
		z();
being completely optimised away.

	David

Re: [PATCH net-next 5/5] net: sctp: fix incorrect type in gfp initializer

['Tejun Heo']

On Thu, Jun 19, 2014 at 08:13:48AM +0000, David Laight wrote:
> Apart from the fact that it is badly optimised.
> 
> If z() were an empty #define rather than an empty inline function
> then you'd end up with the:
> 	if (preload)
> 		z();
> being completely optimised away.

This isn't that hot a path and I don't wanna change it to work around
compiler behavior at this level.  If you care about it, please report
it to the compiler people.

Thanks.

-- 
tejun