tipc-discussion

Re: [tipc-discussion] [PATCH net v1 2/2] tipc: Fix missing connection request handling

[Ying X. <yin...@wi...>]

On 03/14/2017 01:57 AM, Parthasarathy Bhuvaragan wrote:
> In filter_connect, we use waitqueue_active() to check for any
> connections to wakeup. But waitqueue_active() is missing memory
> barriers while accessing the critical sections, leading to
> inconsistent results.
> 
> In this commit, we replace this with an SMB safe wq_has_sleeper().
> 

It sounds like you missed my comments below:
https://sourceforge.net/p/tipc/mailman/tipc-discussion/thread/d8bb5634-21ea-f562-423c-d2ebfa648d3f%40windriver.com/#msg35692945

> Signed-off-by: Parthasarathy Bhuvaragan <par...@er...>
> ---
>  net/tipc/socket.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 79e628cd08a9..ce6ed0955e36 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1577,7 +1577,7 @@ static bool filter_connect(struct tipc_sock *tsk, struct sk_buff *skb)
>  			return true;
>  
>  		/* If empty 'ACK-' message, wake up sleeping connect() */
> -		if (waitqueue_active(sk_sleep(sk)))
> +		if (wq_has_sleeper(rcu_dereference(sk->sk_wq)))
>  			wake_up_interruptible(sk_sleep(sk));
>  
>  		/* 'ACK-' message is neither accepted nor rejected: */
> 

[tipc-discussion] [PATCH v3 net-next 2/3] tipc: advance the time of deleting subscription from subscriber->subscrp_list

[Parthasarathy B. <par...@er...>]

From: Ying Xue <yin...@wi...>

After a subscription object is created, it's inserted into its
subscriber subscrp_list list under subscriber lock protection,
similarly, before it's destroyed, it should be first removed from
its subscriber->subscrp_list. Since the subscription list is
accessed with subscriber lock, all the subscriptions are valid
during the lock duration. Hence in tipc_subscrb_subscrp_delete(), we
remove subscription get/put and the extra subscriber unlock/lock.

After this change, the subscriptions refcount cleanup is very simple
and does not access any lock.

Signed-off-by: Ying Xue <yin...@wi...>
Signed-off-by: Parthasarathy Bhuvaragan <par...@er...>
---
v2: Remove the subscrp_list at tipc_subscrp_timeout() also. This
    prevents any subscription cancel or subscriber delete thread
    running in parallel seeing the timeout out subscription.
---
 net/tipc/subscr.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index 271cd66e4b3b..0649bc29c6bb 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -145,6 +145,7 @@ static void tipc_subscrp_timeout(unsigned long data)
 
 	spin_lock_bh(&subscriber->lock);
 	tipc_nametbl_unsubscribe(sub);
+	list_del(&sub->subscrp_list);
 	spin_unlock_bh(&subscriber->lock);
 
 	/* Notify subscriber of timeout */
@@ -177,10 +178,7 @@ static void tipc_subscrp_kref_release(struct kref *kref)
 	struct tipc_net *tn = net_generic(sub->net, tipc_net_id);
 	struct tipc_subscriber *subscriber = sub->subscriber;
 
-	spin_lock_bh(&subscriber->lock);
-	list_del(&sub->subscrp_list);
 	atomic_dec(&tn->subscription_count);
-	spin_unlock_bh(&subscriber->lock);
 	kfree(sub);
 	tipc_subscrb_put(subscriber);
 }
@@ -210,11 +208,8 @@ static void tipc_subscrb_subscrp_delete(struct tipc_subscriber *subscriber,
 			continue;
 
 		tipc_nametbl_unsubscribe(sub);
-		tipc_subscrp_get(sub);
-		spin_unlock_bh(&subscriber->lock);
+		list_del(&sub->subscrp_list);
 		tipc_subscrp_delete(sub);
-		tipc_subscrp_put(sub);
-		spin_lock_bh(&subscriber->lock);
 
 		if (s)
 			break;
-- 
2.1.4

[tipc-discussion] [PATCH net v1 1/2] tipc: fix socket flow control errors

[Parthasarathy B. <par...@er...>]

In this commit, we fix the following two errors:
1. In tipc_send_stream(), fix the return value during congestion
   when the send is partially successful. Until now, we return -1
   instead of returning the partial sent bytes.
2. In tipc_recv_stream(), we update the rcv_unack not based on the
   message size, but on sz. Usually they are the same, but in cases
   where the socket receivers buffer is smaller than the incoming
   message, these two parameters differ greatly. This introduces a
   slack in accounting leading to permanent congestion. In this
   commit, we perform accounting always based on the incoming message.

Signed-off-by: Parthasarathy Bhuvaragan <par...@er...>
---
 net/tipc/socket.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 6b09a778cc71..79e628cd08a9 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -1080,7 +1080,7 @@ static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dlen)
 		}
 	} while (sent < dlen && !rc);
 
-	return rc ? rc : sent;
+	return sent ? sent : rc;
 }
 
 /**
@@ -1481,16 +1481,15 @@ static int tipc_recv_stream(struct socket *sock, struct msghdr *m,
 	if (unlikely(flags & MSG_PEEK))
 		goto exit;
 
-	tsk->rcv_unacked += tsk_inc(tsk, hlen + sz);
+	tsk->rcv_unacked += tsk_inc(tsk, hlen + msg_data_sz(msg));
 	if (unlikely(tsk->rcv_unacked >= (tsk->rcv_win / 4)))
 		tipc_sk_send_ack(tsk);
 	tsk_advance_rx_queue(sk);
 
 	/* Loop around if more data is required */
-	if ((sz_copied < buf_len) &&	/* didn't get all requested data */
+	if ((!err) && (sz_copied < buf_len) &&
 	    (!skb_queue_empty(&sk->sk_receive_queue) ||
-	    (sz_copied < target)) &&	/* and more is ready or required */
-	    (!err))			/* and haven't reached a FIN */
+	     (sz_copied < target)))
 		goto restart;
 
 exit:
-- 
2.1.4

[tipc-discussion] [PATCH v3 net-next 3/3] tipc: adjust the policy of holding subscription kref

[Parthasarathy B. <par...@er...>]

From: Ying Xue <yin...@wi...>

When a new subscription object is inserted into name_seq->subscriptions
list, it's under name_seq->lock protection; when a subscription is
deleted from the list, it's also under the same lock protection;
similarly, when accessing a subscription by going through subscriptions
list, the entire process is also protected by the name_seq->lock.

Therefore, if subscription refcount is increased before it's inserted
into subscriptions list, and its refcount is decreased after it's
deleted from the list, it will be unnecessary to hold refcount at all
before accessing subscription object which is obtained by going through
subscriptions list under name_seq->lock protection.

Signed-off-by: Ying Xue <yin...@wi...>
Reviewed-by: Jon Maloy <jon...@er...>
---
 net/tipc/name_table.c | 2 ++
 net/tipc/subscr.c     | 8 ++------
 net/tipc/subscr.h     | 3 +++
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/net/tipc/name_table.c b/net/tipc/name_table.c
index 9be6592e4a6f..bd0aac87b41a 100644
--- a/net/tipc/name_table.c
+++ b/net/tipc/name_table.c
@@ -416,6 +416,7 @@ static void tipc_nameseq_subscribe(struct name_seq *nseq,
 
 	tipc_subscrp_convert_seq(&s->evt.s.seq, s->swap, &ns);
 
+	tipc_subscrp_get(s);
 	list_add(&s->nameseq_list, &nseq->subscriptions);
 
 	if (!sseq)
@@ -787,6 +788,7 @@ void tipc_nametbl_unsubscribe(struct tipc_subscription *s)
 	if (seq != NULL) {
 		spin_lock_bh(&seq->lock);
 		list_del_init(&s->nameseq_list);
+		tipc_subscrp_put(s);
 		if (!seq->first_free && list_empty(&seq->subscriptions)) {
 			hlist_del_init_rcu(&seq->ns_list);
 			kfree(seq->sseqs);
diff --git a/net/tipc/subscr.c b/net/tipc/subscr.c
index 0649bc29c6bb..0bf91cd3733c 100644
--- a/net/tipc/subscr.c
+++ b/net/tipc/subscr.c
@@ -54,8 +54,6 @@ struct tipc_subscriber {
 
 static void tipc_subscrp_delete(struct tipc_subscription *sub);
 static void tipc_subscrb_put(struct tipc_subscriber *subscriber);
-static void tipc_subscrp_put(struct tipc_subscription *subscription);
-static void tipc_subscrp_get(struct tipc_subscription *subscription);
 
 /**
  * htohl - convert value to endianness used by destination
@@ -125,7 +123,6 @@ void tipc_subscrp_report_overlap(struct tipc_subscription *sub, u32 found_lower,
 {
 	struct tipc_name_seq seq;
 
-	tipc_subscrp_get(sub);
 	tipc_subscrp_convert_seq(&sub->evt.s.seq, sub->swap, &seq);
 	if (!tipc_subscrp_check_overlap(&seq, found_lower, found_upper))
 		return;
@@ -135,7 +132,6 @@ void tipc_subscrp_report_overlap(struct tipc_subscription *sub, u32 found_lower,
 
 	tipc_subscrp_send_event(sub, found_lower, found_upper, event, port_ref,
 				node);
-	tipc_subscrp_put(sub);
 }
 
 static void tipc_subscrp_timeout(unsigned long data)
@@ -183,12 +179,12 @@ static void tipc_subscrp_kref_release(struct kref *kref)
 	tipc_subscrb_put(subscriber);
 }
 
-static void tipc_subscrp_put(struct tipc_subscription *subscription)
+void tipc_subscrp_put(struct tipc_subscription *subscription)
 {
 	kref_put(&subscription->kref, tipc_subscrp_kref_release);
 }
 
-static void tipc_subscrp_get(struct tipc_subscription *subscription)
+void tipc_subscrp_get(struct tipc_subscription *subscription)
 {
 	kref_get(&subscription->kref);
 }
diff --git a/net/tipc/subscr.h b/net/tipc/subscr.h
index ffdc214c117a..ee52957dc952 100644
--- a/net/tipc/subscr.h
+++ b/net/tipc/subscr.h
@@ -78,4 +78,7 @@ u32 tipc_subscrp_convert_seq_type(u32 type, int swap);
 int tipc_topsrv_start(struct net *net);
 void tipc_topsrv_stop(struct net *net);
 
+void tipc_subscrp_put(struct tipc_subscription *subscription);
+void tipc_subscrp_get(struct tipc_subscription *subscription);
+
 #endif
-- 
2.1.4

Re: [tipc-discussion] [PATCH net v1 1/2] tipc: fix socket flow control errors

[Parthasarathy B. <par...@er...>]

Please ignore this patch and
'tipc: Fix missing connection request handling'.

They dont belong to this series and they are not the right version.

/Partha

On 03/13/2017 06:57 PM, Parthasarathy Bhuvaragan wrote:
> In this commit, we fix the following two errors:
> 1. In tipc_send_stream(), fix the return value during congestion
>    when the send is partially successful. Until now, we return -1
>    instead of returning the partial sent bytes.
> 2. In tipc_recv_stream(), we update the rcv_unack not based on the
>    message size, but on sz. Usually they are the same, but in cases
>    where the socket receivers buffer is smaller than the incoming
>    message, these two parameters differ greatly. This introduces a
>    slack in accounting leading to permanent congestion. In this
>    commit, we perform accounting always based on the incoming message.
>
> Signed-off-by: Parthasarathy Bhuvaragan <par...@er...>
> ---
>  net/tipc/socket.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 6b09a778cc71..79e628cd08a9 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -1080,7 +1080,7 @@ static int __tipc_sendstream(struct socket *sock, struct msghdr *m, size_t dlen)
>  		}
>  	} while (sent < dlen && !rc);
>
> -	return rc ? rc : sent;
> +	return sent ? sent : rc;
>  }
>
>  /**
> @@ -1481,16 +1481,15 @@ static int tipc_recv_stream(struct socket *sock, struct msghdr *m,
>  	if (unlikely(flags & MSG_PEEK))
>  		goto exit;
>
> -	tsk->rcv_unacked += tsk_inc(tsk, hlen + sz);
> +	tsk->rcv_unacked += tsk_inc(tsk, hlen + msg_data_sz(msg));
>  	if (unlikely(tsk->rcv_unacked >= (tsk->rcv_win / 4)))
>  		tipc_sk_send_ack(tsk);
>  	tsk_advance_rx_queue(sk);
>
>  	/* Loop around if more data is required */
> -	if ((sz_copied < buf_len) &&	/* didn't get all requested data */
> +	if ((!err) && (sz_copied < buf_len) &&
>  	    (!skb_queue_empty(&sk->sk_receive_queue) ||
> -	    (sz_copied < target)) &&	/* and more is ready or required */
> -	    (!err))			/* and haven't reached a FIN */
> +	     (sz_copied < target)))
>  		goto restart;
>
>  exit:
>

Re: [tipc-discussion] [PATCH net v1 1/2] tipc: fix socket flow control errors

[Ying X. <yin...@wi...>]

On 03/14/2017 02:02 AM, Parthasarathy Bhuvaragan wrote:
> Please ignore this patch and
> 'tipc: Fix missing connection request handling'.
> 
> They dont belong to this series and they are not the right version.
> 

Sorry, I just saw the email. Probably you had noticed my comment about
your patch #2 of the series.

> /Partha