tipc-discussion

[tipc-discussion] [PATCH net-next] tipc: return POLLOUT for sockets in an unconnected state

[Erik H. <eri...@er...>]

If an implied connect is attempted during link congestion,
the connect message will be discarded and sendmsg will return
EAGAIN. The application then need to retry the connection attempt
after congestion have abated.
This fixes a problem where polling on a socket in an
unconnected state always returned a zero mask.

Signed-off-by: Erik Hugne <eri...@er...>
---
 net/tipc/socket.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 03a2610..7710145 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -407,7 +407,7 @@ static int get_name(struct socket *sock, struct sockaddr *uaddr,
  * socket state		flags set
  * ------------		---------
  * unconnected		no read flags
- *			no write flags
+ *			POLLOUT if port is not congested
  *
  * connecting		POLLIN/POLLRDNORM if ACK/NACK in rx queue
  *			no write flags
@@ -437,6 +437,10 @@ static unsigned int poll(struct file *file, struct socket *sock,
 	poll_wait(file, sk_sleep(sk), wait);
 
 	switch ((int)sock->state) {
+	case SS_UNCONNECTED:
+		if (!tipc_sk_port(sk)->congested)
+			mask |= POLLOUT;
+		break;
 	case SS_READY:
 	case SS_CONNECTED:
 		if (!tipc_sk_port(sk)->congested)
-- 
1.7.5.4

Re: [tipc-discussion] [PATCH net-next] tipc: return POLLOUT for sockets in an unconnected state

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

Erik Hugne wrote:
> If an implied connect is attempted during link congestion,
> the connect message will be discarded and sendmsg will return
> EAGAIN. The application then need to retry the connection attempt
> after congestion have abated.
> This fixes a problem where polling on a socket in an
> unconnected state always returned a zero mask.
>
> Signed-off-by: Erik Hugne <eri...@er...>
> ---
>  net/tipc/socket.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 03a2610..7710145 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -407,7 +407,7 @@ static int get_name(struct socket *sock, struct sockaddr *uaddr,
>   * socket state		flags set
>   * ------------		---------
>   * unconnected		no read flags
> - *			no write flags
> + *			POLLOUT if port is not congested
>   *
>   * connecting		POLLIN/POLLRDNORM if ACK/NACK in rx queue
>   *			no write flags
> @@ -437,6 +437,10 @@ static unsigned int poll(struct file *file, struct socket *sock,
>  	poll_wait(file, sk_sleep(sk), wait);
>  
>  	switch ((int)sock->state) {
> +	case SS_UNCONNECTED:
> +		if (!tipc_sk_port(sk)->congested)
> +			mask |= POLLOUT;
> +		break;
>   

Maybe there has some potential issue, please see below scenario:

1. fd = socket(SOCK_STREAM)-->tipc_create() //sock->state = SS_UNCONNECTED
2. poll(fd)

Each time the poll() is timed out, the POLLOUT event will be returned to 
poll() even if any message *doesn't* be sent via the socket.

So please confirm the case.

Thanks,
Ying

>  	case SS_READY:
>  	case SS_CONNECTED:
>  		if (!tipc_sk_port(sk)->congested)
>   

Re: [tipc-discussion] [PATCH net-next] tipc: return POLLOUT for sockets in an unconnected state

[Erik H. <eri...@er...>]

> Maybe there has some potential issue, please see below scenario:
>
> 1. fd = socket(SOCK_STREAM)-->tipc_create() //sock->state = SS_UNCONNECTED
> 2. poll(fd)
>
> Each time the poll() is timed out, the POLLOUT event will be returned to
> poll() even if any message *doesn't* be sent via the socket.
>
> So please confirm the case.
>
I dont think that's a problem.
In this scenario (unconnected socket), the pollout event is necessary 
for the application to know it can retry the implied connection attempt.

//E

Re: [tipc-discussion] [PATCH net-next] tipc: return POLLOUT for sockets in an unconnected state

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

Erik Hugne wrote:
>> Maybe there has some potential issue, please see below scenario:
>>
>> 1. fd = socket(SOCK_STREAM)-->tipc_create() //sock->state = 
>> SS_UNCONNECTED
>> 2. poll(fd)
>>
>> Each time the poll() is timed out, the POLLOUT event will be returned to
>> poll() even if any message *doesn't* be sent via the socket.
>>
>> So please confirm the case.
>>
> I dont think that's a problem.
> In this scenario (unconnected socket), the pollout event is necessary 
> for the application to know it can retry the implied connection attempt.
>
Right, please ignore the noise.

Regards,
Ying

> //E
>
>

Re: [tipc-discussion] [PATCH net-next] tipc: return POLLOUT for sockets in an unconnected state

[Paul G. <pau...@wi...>]

On 12-06-01 07:07 AM, Erik Hugne wrote:

[Eric asked for comments on this patch ; I know it was resent 
 recently but I could only find this older version in my inbox
 (but the patch itself looks the same)]

> If an implied connect is attempted during link congestion,
> the connect message will be discarded and sendmsg will return
> EAGAIN. The application then need to retry the connection attempt
> after congestion have abated.
> This fixes a problem where polling on a socket in an
> unconnected state always returned a zero mask.

Would be nice to expand on why "connect message will be discarded
and sendmsg will return EAGAIN" is a problem -- i.e. what is the
end user symptom this causes, since it is not obvious to me.

In a similar vein, it is not obvious to me how "polling on a
socket in an unconnected state always returned a zero mask" is
a problem (again, what are end user symptoms) and it is also
not obvious what the "new" behaviour is after this commit
is applied.

This is just a repeat of the generalization for commit logs,
where the requirement is to 1) describe the problem, 2) describe
the end user symptoms (the use case it impacts, and how it
breaks it) and 3) describe the fix - i.e. how the fix
works and why it was chosen as the _right_ fix.

Also it looks like there are space-before-tab in the comment
line -- using checkpatch.pl will alert you about these.

Thanks,
Paul.

> 
> Signed-off-by: Erik Hugne <eri...@er...>
> ---
>  net/tipc/socket.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 03a2610..7710145 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -407,7 +407,7 @@ static int get_name(struct socket *sock, struct sockaddr *uaddr,
>   * socket state		flags set
>   * ------------		---------
>   * unconnected		no read flags
> - *			no write flags
> + *			POLLOUT if port is not congested
>   *
>   * connecting		POLLIN/POLLRDNORM if ACK/NACK in rx queue
>   *			no write flags
> @@ -437,6 +437,10 @@ static unsigned int poll(struct file *file, struct socket *sock,
>  	poll_wait(file, sk_sleep(sk), wait);
>  
>  	switch ((int)sock->state) {
> +	case SS_UNCONNECTED:
> +		if (!tipc_sk_port(sk)->congested)
> +			mask |= POLLOUT;
> +		break;
>  	case SS_READY:
>  	case SS_CONNECTED:
>  		if (!tipc_sk_port(sk)->congested)
> 

[tipc-discussion] [PATCH v2] tipc: return POLLOUT for sockets in an unconnected state

[<eri...@er...>]

From: Erik Hugne <eri...@er...>

If an implied connect is attempted on a nonblocking STREAM/SEQPACKET
socket during link congestion, the connect message will be discarded
and sendmsg will return EAGAIN.
This is normal behavior, and the application is expected to poll
the socket until POLLOUT is set, after which the connection
attempt can be retried.
However, the POLLOUT flag is never set for unconnected sockets
and poll() always returns a zero mask.
The application is then left without a trigger for when
it can make another attempt at sending the message.

The solution is to check if we're poll'ing on an unconnected socket
and set the POLLOUT flag if the TIPC port owned by this socket
is not congested.
The TIPC ports waiting on a specific link will be marked as
'not congested' when the link congestion have abated.

Signed-off-by: Erik Hugne <eri...@er...>
---
 net/tipc/socket.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 09dc5b9..e9a9dfd 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -407,7 +407,7 @@ static int get_name(struct socket *sock, struct sockaddr *uaddr,
  * socket state		flags set
  * ------------		---------
  * unconnected		no read flags
- *			no write flags
+ *			POLLOUT if port is not congested
  *
  * connecting		POLLIN/POLLRDNORM if ACK/NACK in rx queue
  *			no write flags
@@ -437,6 +437,10 @@ static unsigned int poll(struct file *file, struct socket *sock,
 	poll_wait(file, sk_sleep(sk), wait);
 
 	switch ((int)sock->state) {
+	case SS_UNCONNECTED:
+		if (!tipc_sk_port(sk)->congested)
+			mask |= POLLOUT;
+		break;
 	case SS_READY:
 	case SS_CONNECTED:
 		if (!tipc_sk_port(sk)->congested)
-- 
1.7.5.4

Re: [tipc-discussion] [PATCH v2] tipc: return POLLOUT for sockets in an unconnected state

[Paul G. <pau...@wi...>]

On 12-10-16 03:22 AM, eri...@er... wrote:
> From: Erik Hugne <eri...@er...>
> 
> If an implied connect is attempted on a nonblocking STREAM/SEQPACKET
> socket during link congestion, the connect message will be discarded
> and sendmsg will return EAGAIN.
> This is normal behavior, and the application is expected to poll
> the socket until POLLOUT is set, after which the connection
> attempt can be retried.
> However, the POLLOUT flag is never set for unconnected sockets
> and poll() always returns a zero mask.
> The application is then left without a trigger for when
> it can make another attempt at sending the message.
> 
> The solution is to check if we're poll'ing on an unconnected socket
> and set the POLLOUT flag if the TIPC port owned by this socket
> is not congested.
> The TIPC ports waiting on a specific link will be marked as
> 'not congested' when the link congestion have abated.

This seems much more informative and demonstrates a clearer
understanding of what is going on.  Lets get rid of the random
insertion of carriage returns after each sentence and reserve
their use for meaningful newlines between paragraphs, and
fix "poll'ing" to be just "polling" though.

Thanks,
Paul.
--

> 
> Signed-off-by: Erik Hugne <eri...@er...>
> ---
>  net/tipc/socket.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 09dc5b9..e9a9dfd 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -407,7 +407,7 @@ static int get_name(struct socket *sock, struct sockaddr *uaddr,
>   * socket state		flags set
>   * ------------		---------
>   * unconnected		no read flags
> - *			no write flags
> + *			POLLOUT if port is not congested
>   *
>   * connecting		POLLIN/POLLRDNORM if ACK/NACK in rx queue
>   *			no write flags
> @@ -437,6 +437,10 @@ static unsigned int poll(struct file *file, struct socket *sock,
>  	poll_wait(file, sk_sleep(sk), wait);
>  
>  	switch ((int)sock->state) {
> +	case SS_UNCONNECTED:
> +		if (!tipc_sk_port(sk)->congested)
> +			mask |= POLLOUT;
> +		break;
>  	case SS_READY:
>  	case SS_CONNECTED:
>  		if (!tipc_sk_port(sk)->congested)
> 

[tipc-discussion] [PATCH v3] tipc: return POLLOUT for sockets in an unconnected state

[<eri...@er...>]

From: Erik Hugne <eri...@er...>

If an implied connect is attempted on a nonblocking STREAM/SEQPACKET
socket during link congestion, the connect message will be discarded
and sendmsg will return EAGAIN. This is normal behavior, and the
application is expected to poll the socket until POLLOUT is set,
after which the connection attempt can be retried.
However, the POLLOUT flag is never set for unconnected sockets and
poll() always returns a zero mask. The application is then left without
a trigger for when it can make another attempt at sending the message.

The solution is to check if we're polling on an unconnected socket
and set the POLLOUT flag if the TIPC port owned by this socket
is not congested. The TIPC ports waiting on a specific link will be
marked as 'not congested' when the link congestion have abated.

Signed-off-by: Erik Hugne <eri...@er...>
---
 net/tipc/socket.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/net/tipc/socket.c b/net/tipc/socket.c
index 09dc5b9..e9a9dfd 100644
--- a/net/tipc/socket.c
+++ b/net/tipc/socket.c
@@ -407,7 +407,7 @@ static int get_name(struct socket *sock, struct sockaddr *uaddr,
  * socket state		flags set
  * ------------		---------
  * unconnected		no read flags
- *			no write flags
+ *			POLLOUT if port is not congested
  *
  * connecting		POLLIN/POLLRDNORM if ACK/NACK in rx queue
  *			no write flags
@@ -437,6 +437,10 @@ static unsigned int poll(struct file *file, struct socket *sock,
 	poll_wait(file, sk_sleep(sk), wait);
 
 	switch ((int)sock->state) {
+	case SS_UNCONNECTED:
+		if (!tipc_sk_port(sk)->congested)
+			mask |= POLLOUT;
+		break;
 	case SS_READY:
 	case SS_CONNECTED:
 		if (!tipc_sk_port(sk)->congested)
-- 
1.7.5.4

Re: [tipc-discussion] [PATCH v3] tipc: return POLLOUT for sockets in an unconnected state

[Paul G. <pau...@wi...>]

On 12-10-16 10:47 AM, eri...@er... wrote:
> From: Erik Hugne <eri...@er...>
> 
> If an implied connect is attempted on a nonblocking STREAM/SEQPACKET
> socket during link congestion, the connect message will be discarded
> and sendmsg will return EAGAIN. This is normal behavior, and the
> application is expected to poll the socket until POLLOUT is set,
> after which the connection attempt can be retried.
> However, the POLLOUT flag is never set for unconnected sockets and
> poll() always returns a zero mask. The application is then left without
> a trigger for when it can make another attempt at sending the message.
> 
> The solution is to check if we're polling on an unconnected socket
> and set the POLLOUT flag if the TIPC port owned by this socket
> is not congested. The TIPC ports waiting on a specific link will be
> marked as 'not congested' when the link congestion have abated.
> 
> Signed-off-by: Erik Hugne <eri...@er...>

Looks good, thanks Erik.  Help me gauge the impact of this -- i.e.
is it a rare corner case, or an easy to trigger bug that is
impacting a lot of people?    If it has a high impact, we can
suggest it for "net" and hence 3.7 inclusion, otherwise we
should not needlessly add to the post merge window burden, and
instead nominate it for net-next which means 3.8.

Paul.
--

> ---
>  net/tipc/socket.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/net/tipc/socket.c b/net/tipc/socket.c
> index 09dc5b9..e9a9dfd 100644
> --- a/net/tipc/socket.c
> +++ b/net/tipc/socket.c
> @@ -407,7 +407,7 @@ static int get_name(struct socket *sock, struct sockaddr *uaddr,
>   * socket state		flags set
>   * ------------		---------
>   * unconnected		no read flags
> - *			no write flags
> + *			POLLOUT if port is not congested
>   *
>   * connecting		POLLIN/POLLRDNORM if ACK/NACK in rx queue
>   *			no write flags
> @@ -437,6 +437,10 @@ static unsigned int poll(struct file *file, struct socket *sock,
>  	poll_wait(file, sk_sleep(sk), wait);
>  
>  	switch ((int)sock->state) {
> +	case SS_UNCONNECTED:
> +		if (!tipc_sk_port(sk)->congested)
> +			mask |= POLLOUT;
> +		break;
>  	case SS_READY:
>  	case SS_CONNECTED:
>  		if (!tipc_sk_port(sk)->congested)
> 

Re: [tipc-discussion] [PATCH v3] tipc: return POLLOUT for sockets in an unconnected state

[Erik H. <eri...@er...>]

> Looks good, thanks Erik.  Help me gauge the impact of this -- i.e.
> is it a rare corner case, or an easy to trigger bug that is
> impacting a lot of people?

It's quite easy to reproduce if you have an application that 
(implicitly) establishes and tears down connections at high rates.
A link congestion lasting 1 second will cause a lot of connections to 
'hang'.

>   If it has a high impact, we can
> suggest it for "net" and hence 3.7 inclusion, otherwise we
> should not needlessly add to the post merge window burden, and
> instead nominate it for net-next which means 3.8.

Not as high as the packet loss bug, but still, this forces one of our 
TIPC applications to do an extra syscall (explicit connect) to send data 
(read, establish a call/context).

I still think it's fine to suggest for net-next, (we backport these 
critical fixes to older kernels anyway).

//E

Re: [tipc-discussion] [PATCH net-next] tipc: return POLLOUT for sockets in an unconnected state

[Erik H. <eri...@er...>]

> Also it looks like there are space-before-tab in the comment
> line -- using checkpatch.pl will alert you about these.
It does pass checkpatch.pl.
If you mean this line:
>> + *			POLLOUT if port is not congested

There's a * before the tabs.

//E