Revert "IB/core: Add flow control to the portmapper netlink calls"

PATCH In review Leon Romanovsky 6 participants 28 messages May 29, 2017 – Jun 6, 2017 18 messages not in the archive InfiniBand

lore

Share & tools

lore

Leon Romanovsky

author

2017-05-29 08:24 UTC #0 lore raw

PATCH 1/1 Revert "IB/core: Add flow control to the portmapper netlink calls"

From: Leon Romanovsky <leonro-Nfu5REtnQAJWk0Htik3J/[email protected]>

The commit cea05eadded0 ("IB/core: Add flow control to the portmapper netlink calls") changed netlink to be blocked for all RDMA clients. This workaround worked perfectly for portmapper, but is not correct for the whole NETLINK_RDMA family.

The request/response should always be blocking and asynchronous notification should always be non-blocking.

It is library and user-space application to chose how to handle recvmgs, as an example see nl_recvmsgs() and nl_socket_set_nonblocking() calls of libnl library.

Send timeout is not needed too and can be configured with SO_SNDTIMEO socket option.

This reverts commit cea05eadded0d4eb59f7be6e1f1560eb6bfde2bf.

Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

---
 drivers/infiniband/core/netlink.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

drivers/infiniband/core/netlink.c +1 -5

index b784055423c8..fcc9702efd38 100644
@@ -230,10 +230,7 @@ static void ibnl_rcv(struct sk_buff *skb)
 int ibnl_unicast(struct sk_buff *skb, struct nlmsghdr *nlh,
 			__u32 pid)
 {
-	int err;
-
-	err = netlink_unicast(nls, skb, pid, 0);
-	return (err < 0) ? err : 0;
+	return nlmsg_unicast(nls, skb, pid);
 }
 EXPORT_SYMBOL(ibnl_unicast);

@@ -256,7 +253,6 @@ int __init ibnl_init(void)
 		return -ENOMEM;
 	}
 
-	nls->sk_sndtimeo = 10 * HZ;
 	return 0;
 }

--
2.12.2

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170529082423.1180-1-leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org

Shiraz Saleem

2017-05-30 21:24 UTC #2 lore raw

Quoted 14 lines · Leon Romanovsky

From: Leon Romanovsky <leonro-Nfu5REtnQAJWk0Htik3J/[email protected]>
The commit cea05eadded0 ("IB/core: Add flow control to the portmapper netlink calls") changed netlink to be blocked for all RDMA clients. This workaround worked perfectly for portmapper, but is not correct for the whole NETLINK_RDMA family.
The request/response should always be blocking and asynchronous notification should always be non-blocking.
It is library and user-space application to chose how to handle recvmgs, as an example see nl_recvmsgs() and nl_socket_set_nonblocking() calls of libnl library.

Quoted 6 lines

Send timeout is not needed too and can be configured with SO_SNDTIMEO socket option.
This reverts commit cea05eadded0d4eb59f7be6e1f1560eb6bfde2bf.
Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>

This commit was accepted with much feedback from you.

https://patchwork.kernel.org/patch/9235137/

Not clear from your description to determine what your trying to fix.

Simply reverting a commit that solves a real problem is not acceptable.

Please come up with a solution without breaking portmapper.

Shiraz
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170530212431.GA21008-GOXS9JX10wfOxmVO0tvppfooFf0ArEBIu+b9c/[email protected]

Leon Romanovsky

author

2017-05-31 04:04 UTC #4 lore raw

Quoted 24 lines · Shiraz Saleem

Quoted 14 lines · Leon Romanovsky
From: Leon Romanovsky <leonro-Nfu5REtnQAJWk0Htik3J/[email protected]>
The commit cea05eadded0 ("IB/core: Add flow control to the portmapper netlink calls") changed netlink to be blocked for all RDMA clients. This workaround worked perfectly for portmapper, but is not correct for the whole NETLINK_RDMA family.
The request/response should always be blocking and asynchronous notification should always be non-blocking.
It is library and user-space application to chose how to handle recvmgs, as an example see nl_recvmsgs() and nl_socket_set_nonblocking() calls of libnl library.
Quoted 6 lines
Send timeout is not needed too and can be configured with SO_SNDTIMEO socket option.
This reverts commit cea05eadded0d4eb59f7be6e1f1560eb6bfde2bf.
Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
This commit was accepted with much feedback from you.

Quoted 7 lines

https://patchwork.kernel.org/patch/9235137/
Not clear from your description to determine what your trying to fix.
Simply reverting a commit that solves a real problem is not acceptable.
Please come up with a solution without breaking portmapper.

1. It was cosmetic comments, I didn't give my reviewed-by tag, so please don't put blame on me,
2. The commit cea05eadde broke NETLINK semantics for whole RDMA.
3. The commit cea05eadde made libnl library (basic block of user-space part of netlink) to work incorrectly and not according to _blocking/_nonblocking semantics.
4. Reverting is a common practice in Linux kernel. Patches are not
carved in stones.
5. I proposed a solution -> go and fix your user space program.

Thanks

Quoted 2 lines

Shiraz

A message that is not in the archive (sent off-list or not yet received): 20170531040437.GE5406-U/[email protected]

Shiraz Saleem

2017-05-31 17:42 UTC #6 lore raw

Quoted 27 lines · Leon Romanovsky

Quoted 26 lines · Shiraz Saleem
Quoted 14 lines · Leon Romanovsky
From: Leon Romanovsky <leonro-Nfu5REtnQAJWk0Htik3J/[email protected]>
The commit cea05eadded0 ("IB/core: Add flow control to the portmapper netlink calls") changed netlink to be blocked for all RDMA clients. This workaround worked perfectly for portmapper, but is not correct for the whole NETLINK_RDMA family.
The request/response should always be blocking and asynchronous notification should always be non-blocking.
It is library and user-space application to chose how to handle recvmgs, as an example see nl_recvmsgs() and nl_socket_set_nonblocking() calls of libnl library.
Quoted 6 lines
Send timeout is not needed too and can be configured with SO_SNDTIMEO socket option.
This reverts commit cea05eadded0d4eb59f7be6e1f1560eb6bfde2bf.
Signed-off-by Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/[email protected]>
This commit was accepted with much feedback from you.
https://patchwork.kernel.org/patch/9235137/

Quoted 8 lines

Quoted 5 lines
Not clear from your description to determine what your trying to fix.
Simply reverting a commit that solves a real problem is not acceptable.
Please come up with a solution without breaking portmapper.
1. It was cosmetic comments, I didn't give my reviewed-by tag, so please don't put blame on me,

We are not blaming you. You had a lot of input on the original thread including, "One is move from non-blocking mode to blocking mode which is fine enough after justification was added"

Quoted 1 line

2. The commit cea05eadde broke NETLINK semantics for whole RDMA.

How did it break? What's the issue?

Quoted 2 lines

3. The commit cea05eadde made libnl library (basic block of user-space part of netlink) to work incorrectly and not according to _blocking/_nonblocking semantics.

How? Is libnl calling ibnl_unicast? As far we can understand ibnl_unicast is only called by portmapper kernel code.

Quoted 2 lines

4. Reverting is a common practice in Linux kernel. Patches are not carved in stones.

Reverting a patch that's introduced during RC cycle is fine, introducing regression is NOT and that is what you are doing by simply proposing to revert this patch. Reverting this patch will introduce a REGRESSION error with respect to port mapping functionality for all iWARP vendors.

Quoted 1 line

5. I proposed a solution -> go and fix your user space program.

This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.

Bottom line, come up with a solution that will address both port mapper functionality and your issue.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170531174245.GA16304-GOXS9JX10wfOxmVO0tvppfooFf0ArEBIu+b9c/[email protected]

Steve Wise

2017-05-31 17:46 UTC #8 lore raw

Quoted 7 lines

Quoted 3 lines
4. Reverting is a common practice in Linux kernel. Patches are not carved in stones.
Reverting a patch that's introduced during RC cycle is fine, introducing regression is NOT and that is what you are doing by simply proposing to revert this patch. Reverting this patch will introduce a REGRESSION error with

respect to

Quoted 8 lines

port mapping functionality for all iWARP vendors.
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Bottom line, come up with a solution that will address both port mapper functionality and your issue.

Hey Leon,

I would hate to introduce an iwarp regression vs solving your issue w/o reverting. If I recall, the flow control is need so as to not drop messages flowing from the kernel to the iwpmd. I'm not sure how that could be fixed in the user daemon?

Steve.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2017-05-31 18:20 UTC #9 lore raw

Quoted 17 lines · Steve Wise

Quoted 7 lines
Quoted 3 lines
4. Reverting is a common practice in Linux kernel. Patches are not carved in stones.
Reverting a patch that's introduced during RC cycle is fine, introducing regression is NOT and that is what you are doing by simply proposing to revert this patch. Reverting this patch will introduce a REGRESSION error with
respect to
Quoted 8 lines
port mapping functionality for all iWARP vendors.
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Bottom line, come up with a solution that will address both port mapper functionality and your issue.

Quoted 6 lines

Hey Leon,
I would hate to introduce an iwarp regression vs solving your issue w/o reverting. If I recall, the flow control is need so as to not drop messages flowing from the kernel to the iwpmd. I'm not sure how that could be fixed in the user daemon?

Assume that netlink is unreliable and set socket timeout.

Thanks

Quoted 7 lines

Steve.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170531182050.GL5406-U/[email protected]

Steve Wise

2017-05-31 18:34 UTC #11 lore raw

Quoted 5 lines

Quoted 5 lines
Hey Leon,
I would hate to introduce an iwarp regression vs solving your issue w/o reverting. If I recall, the flow control is need so as to not drop messages flowing from the kernel to the iwpmd. I'm not sure how that could be fixed

Quoted 5 lines

Quoted 1 line
the user daemon?
Assume that netlink is unreliable and set socket timeout.
Thanks

It seems that netlink is becoming more and more a message API between kernel and user. Wouldn't everyone benefit from make it reliable? And I think we can certainly provide blocking/reliable mode as an option to be used only by those subsystems that require it.

Steve

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Bart Van Assche

2017-05-31 20:06 UTC #12 lore raw

Quoted 11 lines · Steve Wise

Quoted 6 lines
Quoted 4 lines
I would hate to introduce an iwarp regression vs solving your issue w/o reverting. If I recall, the flow control is need so as to not drop messages flowing from the kernel to the iwpmd. I'm not sure how that could be fixed in the user daemon?
Assume that netlink is unreliable and set socket timeout.
It seems that netlink is becoming more and more a message API between kernel and user. Wouldn't everyone benefit from make it reliable? And I think we can certainly provide blocking/reliable mode as an option to be used only by those subsystems that require it.

Sorry Steve but I'm not sure that's a good idea. Netlink uses fixed size buffers for sending messages to user space. Making netlink reliable means making the kernel wait if the buffer is full. That would introduce the possibility of a kernel deadlock if the user space application hangs. I don't think we want that.

BTW, are you aware netlink reports buffer overflows to user space through the ENOBUFS error code? Any user space software that reads from a netlink socket should check the error code returned by recv() and resynchronize the state if recv() fails with ENOBUFS. I'm writing here "should" because there are several examples of user space software that doesn't do this correctly.

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Bart Van Assche

2017-05-31 20:10 UTC #13 lore raw

In reply to a message not in the archive

Quoted 7 lines · Shiraz Saleem

Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Bottom line, come up with a solution that will address both port mapper functionality and your issue.

Hello Shiraz,

Sorry that this means additional work for you, but I agree with Leon that user space software should not assume that netlink sockets are a reliable communication mechanism.

Bart.--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

Shiraz Saleem

2017-06-02 16:28 UTC #15 lore raw

Quoted 6 lines · Bart Van Assche

Quoted 5 lines · Shiraz Saleem
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.

Quoted 8 lines

Quoted 2 lines
Bottom line, come up with a solution that will address both port mapper functionality and your issue.
Hello Shiraz,
Sorry that this means additional work for you, but I agree with Leon that user space software should not assume that netlink sockets are a reliable communication mechanism.

Hi Bart - Thank you for your response.

The original problem was that ibnl_unicast, which is used to send nl messages from portmapper kernel space to user-space, would occasionally and momentarily fail under stress. We could have retried the call for a certain amount of time, but since netlink_unicast has a nonblock/block parameter, we chose to use the blocking option with a timeout. So we thought we did account for deadlocks with this timeout.

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170602162849.GA28660-GOXS9JX10wfOxmVO0tvppfooFf0ArEBIu+b9c/[email protected]

Bart Van Assche

2017-06-03 13:02 UTC #17 lore raw

T24gRnJpLCAyMDE3LTA2LTAyIGF0IDExOjI4IC0wNTAwLCBTaGlyYXogU2FsZWVtIHdyb3RlOg0K PiBPbiBXZWQsIE1heSAzMSwgMjAxNyBhdCAwMjoxMDozMVBNIC0wNjAwLCBCYXJ0IFZhbiBBc3Nj aGUgd3JvdGU6DQo+ID4gT24gV2VkLCAyMDE3LTA1LTMxIGF0IDEyOjQyIC0wNTAwLCBTaGlyYXog U2FsZWVtIHdyb3RlOg0KPiA+ID4gPiA1LiBJIHByb3Bvc2VkIGEgc29sdXRpb24gLT4gZ28gYW5k IGZpeCB5b3VyIHVzZXIgc3BhY2UgcHJvZ3JhbS4NCj4gPiA+IA0KPiA+ID4gVGhpcyBpcyBhIGtl cm5lbCBwYXRjaCB5b3UgYXJlIHRyeWluZyB0byByZXZlcnQsIHlvdSBhcmUgYnJlYWtpbmcgZXhp c3RpbmcNCj4gPiA+IGtlcm5lbCBmdW5jdGlvbmFsaXR5LiAgTm90aGluZyB0byBkbyB3aXRoIHVz ZXIgc3BhY2UuDQo+ID4gPiANCj4gPiA+IEJvdHRvbSBsaW5lLCBjb21lIHVwIHdpdGggYSBzb2x1 dGlvbiB0aGF0IHdpbGwgYWRkcmVzcyBib3RoIHBvcnQgbWFwcGVyDQo+ID4gPiBmdW5jdGlvbmFs aXR5IGFuZCB5b3VyIGlzc3VlLg0KPiA+IA0KPiA+IEhlbGxvIFNoaXJheiwNCj4gPiANCj4gPiBT b3JyeSB0aGF0IHRoaXMgbWVhbnMgYWRkaXRpb25hbCB3b3JrIGZvciB5b3UsIGJ1dCBJIGFncmVl IHdpdGggTGVvbiB0aGF0DQo+ID4gdXNlciBzcGFjZSBzb2Z0d2FyZSBzaG91bGQgbm90IGFzc3Vt ZSB0aGF0IG5ldGxpbmsgc29ja2V0cyBhcmUgYSByZWxpYWJsZQ0KPiA+IGNvbW11bmljYXRpb24g bWVjaGFuaXNtLg0KPiANCj4gSGkgQmFydCAtIFRoYW5rIHlvdSBmb3IgeW91ciByZXNwb25zZS4N Cj4gDQo+IFRoZSBvcmlnaW5hbCBwcm9ibGVtIHdhcyB0aGF0IGlibmxfdW5pY2FzdCwgd2hpY2gg aXMgdXNlZCB0byBzZW5kIG5sIG1lc3NhZ2VzIGZyb20gDQo+IHBvcnRtYXBwZXIga2VybmVsIHNw YWNlIHRvIHVzZXItc3BhY2UsIHdvdWxkIG9jY2FzaW9uYWxseSBhbmQgbW9tZW50YXJpbHkgZmFp bCB1bmRlciBzdHJlc3MuIA0KPiBXZSBjb3VsZCBoYXZlIHJldHJpZWQgdGhlIGNhbGwgZm9yIGEg Y2VydGFpbiBhbW91bnQgb2YgdGltZSwgYnV0IHNpbmNlIG5ldGxpbmtfdW5pY2FzdCBoYXMgYSAN Cj4gbm9uYmxvY2svYmxvY2sgcGFyYW1ldGVyLCB3ZSBjaG9zZSB0byB1c2UgdGhlIGJsb2NraW5n IG9wdGlvbiB3aXRoIGEgdGltZW91dC4gU28gd2UgdGhvdWdodCB3ZSANCj4gZGlkIGFjY291bnQg Zm9yIGRlYWRsb2NrcyB3aXRoIHRoaXMgdGltZW91dC4NCg0KSGVsbG8gU2hpcmF6LA0KDQpBY2Nv cmRpbmcgdG8gd2hhdCBJIHNlZSBpbiBpd3BtZC9pd3BtZC5jb25mIHRoZSBkZWZhdWx0IHZhbHVl IGZvciB0aGUgaXdwbWQNCm5ldGxpbmsgc29ja2V0IHJlY2VpdmUgYnVmZmVyIHNpemUgaXMgNDE5 NDMwNDAwIGJ5dGVzLiBTbyBJJ20gc3VycHJpc2VkIHRoYXQNCnNlbmRpbmcgbmV0bGluayBtZXNz YWdlcyBvY2Nhc2lvbmFsbHkgZmFpbHMgc2luY2UgdGhlIHJlY2VpdmUgYnVmZmVyIGlzIHNvDQps YXJnZT8NCg0KQmFydC4NCg==
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2017-06-04 05:36 UTC #18 lore raw

In reply to a message not in the archive

Quoted 16 lines · Shiraz Saleem

Quoted 6 lines · Bart Van Assche
Quoted 5 lines · Shiraz Saleem
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Quoted 8 lines
Quoted 2 lines
Bottom line, come up with a solution that will address both port mapper functionality and your issue.
Hello Shiraz,
Sorry that this means additional work for you, but I agree with Leon that user space software should not assume that netlink sockets are a reliable communication mechanism.

Quoted 7 lines

Hi Bart - Thank you for your response.
The original problem was that ibnl_unicast, which is used to send nl messages from portmapper kernel space to user-space, would occasionally and momentarily fail under stress. We could have retried the call for a certain amount of time, but since netlink_unicast has a nonblock/block parameter, we chose to use the blocking option with a timeout. So we thought we did account for deadlocks with this timeout.

Not really, you just reduced the chances. In very large scale, you will have a very large chances of such deadlocks.

Thanks

Quoted 6 lines

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170604053635.GD6868-U/[email protected]

Chien Tin Tung

2017-06-05 02:23 UTC #20 lore raw

Quoted 19 lines · Leon Romanovsky

Quoted 18 lines · Shiraz Saleem
Quoted 6 lines · Bart Van Assche
Quoted 5 lines · Shiraz Saleem
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Quoted 8 lines
Quoted 2 lines
Bottom line, come up with a solution that will address both port mapper functionality and your issue.
Hello Shiraz,
Sorry that this means additional work for you, but I agree with Leon that user space software should not assume that netlink sockets are a reliable communication mechanism.
Hi Bart - Thank you for your response.

Quoted 8 lines

Quoted 5 lines
The original problem was that ibnl_unicast, which is used to send nl messages from portmapper kernel space to user-space, would occasionally and momentarily fail under stress. We could have retried the call for a certain amount of time, but since netlink_unicast has a nonblock/block parameter, we chose to use the blocking option with a timeout. So we thought we did account for deadlocks with this timeout.
Not really, you just reduced the chances. In very large scale, you will have a very large chances of such deadlocks.

Please stop using the word deadlock until you can prove that the deadlock exists with the timeout in place.

Chien

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170605022313.GB18172-TZeIlv3TuzOfrEmaQUPKxl95YUYmaKo1UNDiOz3kqAs@public.gmane.org

Leon Romanovsky

author

2017-06-05 04:00 UTC #22 lore raw

Quoted 26 lines · Chien Tin Tung

Quoted 25 lines · Leon Romanovsky
Quoted 23 lines · Shiraz Saleem
Quoted 6 lines · Bart Van Assche
Quoted 5 lines · Shiraz Saleem
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Quoted 8 lines
Quoted 2 lines
Bottom line, come up with a solution that will address both port mapper functionality and your issue.
Hello Shiraz,
Sorry that this means additional work for you, but I agree with Leon that user space software should not assume that netlink sockets are a reliable communication mechanism.
Hi Bart - Thank you for your response.
The original problem was that ibnl_unicast, which is used to send nl messages from portmapper kernel space to user-space, would occasionally and momentarily fail under stress. We could have retried the call for a certain amount of time, but since netlink_unicast has a nonblock/block parameter, we chose to use the blocking option with a timeout. So we thought we did account for deadlocks with this timeout.

Quoted 5 lines

Quoted 2 lines
Not really, you just reduced the chances. In very large scale, you will have a very large chances of such deadlocks.
Please stop using the word deadlock until you can prove that the deadlock exists with the timeout in place.

Can you please post the whole list of forbidden words? It will be great to have it accompanied with technical response to my and Bart's claims, and to summarize it, it is very simple: "netlink receive should be non-blocking and asynchronous".

It will allow for all libnl functions, like nl_socket_set_nonblocking, nl_recvmsgsa and nl_wait_for_ack, work correctly with ALL NETLINK_RDMA clients.

Thanks

Quoted 4 lines

Chien

A message that is not in the archive (sent off-list or not yet received): 20170605040030.GG6868-U/[email protected]

Chien Tin Tung

2017-06-05 04:20 UTC #24 lore raw

Quoted 30 lines · Leon Romanovsky

Quoted 29 lines · Chien Tin Tung
Quoted 27 lines · Leon Romanovsky
Quoted 16 lines · Shiraz Saleem
Quoted 6 lines · Bart Van Assche
Quoted 5 lines · Shiraz Saleem
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Quoted 8 lines
Quoted 2 lines
Bottom line, come up with a solution that will address both port mapper functionality and your issue.
Hello Shiraz,
Sorry that this means additional work for you, but I agree with Leon that user space software should not assume that netlink sockets are a reliable communication mechanism.
Quoted 7 lines
Hi Bart - Thank you for your response.
The original problem was that ibnl_unicast, which is used to send nl messages from portmapper kernel space to user-space, would occasionally and momentarily fail under stress. We could have retried the call for a certain amount of time, but since netlink_unicast has a nonblock/block parameter, we chose to use the blocking option with a timeout. So we thought we did account for deadlocks with this timeout.
Not really, you just reduced the chances. In very large scale, you will have a very large chances of such deadlocks.

Quoted 7 lines

Quoted 2 lines
Please stop using the word deadlock until you can prove that the deadlock exists with the timeout in place.
Can you please post the whole list of forbidden words? It will be great to have it accompanied with technical response to my and Bart's claims, and to summarize it, it is very simple: "netlink receive should be non-blocking and asynchronous".

Non-blocking and asynchronous is not deadlock, please get it right. Again, provide proof of deadlock, until then refrain from using that word in support of your argument. Which you still have not proved there is a problem.

Chien
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170605042007.GA19068-TZeIlv3TuzOfrEmaQUPKxl95YUYmaKo1UNDiOz3kqAs@public.gmane.org

Chien Tin Tung

2017-06-05 04:50 UTC #26 lore raw

You jump around in this thread so much it is hard for a sane person to follow so I will attempt to summarize what has taken place.

You try to revert a patch that fixed a real problem in portmapper, claiming:

1) Code in question impacts the whole RDMA subsystem. which is false by my multiple replies on this.
2) There is a deadlock.
Which is false. I'm still asking for proof.
3) you want netlink receive to be non-blocking and asynchronous what does that have to do with the non-existing deadlock? Asnwer is you can't when there isn't one.
If you want it, create a patch for it instead of creating a regression with a lazy revert.

I will continue this discussion if you answer directly to any of those points. If you choose to dance around the subject and claim falsehood, you will only damage your own creditibility on the list. I hope you take that to heart.

Chien

Quoted 43 lines · Chien Tin Tung

Quoted 30 lines · Leon Romanovsky
Quoted 29 lines · Chien Tin Tung
Quoted 27 lines · Leon Romanovsky
Quoted 16 lines · Shiraz Saleem
Quoted 6 lines · Bart Van Assche
Quoted 5 lines · Shiraz Saleem
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Quoted 8 lines
Quoted 2 lines
Bottom line, come up with a solution that will address both port mapper functionality and your issue.
Hello Shiraz,
Sorry that this means additional work for you, but I agree with Leon that user space software should not assume that netlink sockets are a reliable communication mechanism.
Quoted 7 lines
Hi Bart - Thank you for your response.
The original problem was that ibnl_unicast, which is used to send nl messages from portmapper kernel space to user-space, would occasionally and momentarily fail under stress. We could have retried the call for a certain amount of time, but since netlink_unicast has a nonblock/block parameter, we chose to use the blocking option with a timeout. So we thought we did account for deadlocks with this timeout.
Not really, you just reduced the chances. In very large scale, you will have a very large chances of such deadlocks.
Quoted 7 lines
Quoted 2 lines
Please stop using the word deadlock until you can prove that the deadlock exists with the timeout in place.
Can you please post the whole list of forbidden words? It will be great to have it accompanied with technical response to my and Bart's claims, and to summarize it, it is very simple: "netlink receive should be non-blocking and asynchronous".
Non-blocking and asynchronous is not deadlock, please get it right. Again, provide proof of deadlock, until then refrain from using that word in support of your argument. Which you still have not proved there is a problem.

Quoted 5 lines

Chien
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170605045043.GA17148-TZeIlv3TuzOfrEmaQUPKxl95YUYmaKo1UNDiOz3kqAs@public.gmane.org

Leon Romanovsky

author

2017-06-05 06:03 UTC #28 lore raw

Quoted 15 lines · Chien Tin Tung

You jump around in this thread so much it is hard for a sane person to follow so I will attempt to summarize what has taken place.
You try to revert a patch that fixed a real problem in portmapper, claiming:
1) Code in question impacts the whole RDMA subsystem. which is false by my multiple replies on this.
2) There is a deadlock.
Which is false. I'm still asking for proof.
3) you want netlink receive to be non-blocking and asynchronous what does that have to do with the non-existing deadlock? Asnwer is you can't when there isn't one.
If you want it, create a patch for it instead of creating a regression with a lazy revert.

Quoted 4 lines

I will continue this discussion if you answer directly to any of those points. If you choose to dance around the subject and claim falsehood, you will only damage your own creditibility on the list. I hope you take that to heart.

OK, I got your point. It is worthless discussion.

FYI, ibnl_unicast holds global lock for whole NETLINK_RDMA static void ibnl_rcv(struct sk_buff *skb)

{
        mutex_lock(&ibnl_mutex);
        ibnl_rcv_reply_skb(skb);
        netlink_rcv_skb(skb, &ibnl_rcv_msg);
        mutex_unlock(&ibnl_mutex);
}

I'll wait for a maintainer's decision on the proposed patch.

Thanks

Quoted 52 lines

Chien
Quoted 48 lines · Chien Tin Tung
Quoted 30 lines · Leon Romanovsky
Quoted 29 lines · Chien Tin Tung
Quoted 27 lines · Leon Romanovsky
Quoted 16 lines · Shiraz Saleem
Quoted 6 lines · Bart Van Assche
Quoted 5 lines · Shiraz Saleem
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Quoted 8 lines
Quoted 2 lines
Bottom line, come up with a solution that will address both port mapper functionality and your issue.
Hello Shiraz,
Sorry that this means additional work for you, but I agree with Leon that user space software should not assume that netlink sockets are a reliable communication mechanism.
Quoted 7 lines
Hi Bart - Thank you for your response.
The original problem was that ibnl_unicast, which is used to send nl messages from portmapper kernel space to user-space, would occasionally and momentarily fail under stress. We could have retried the call for a certain amount of time, but since netlink_unicast has a nonblock/block parameter, we chose to use the blocking option with a timeout. So we thought we did account for deadlocks with this timeout.
Not really, you just reduced the chances. In very large scale, you will have a very large chances of such deadlocks.
Quoted 7 lines
Quoted 2 lines
Please stop using the word deadlock until you can prove that the deadlock exists with the timeout in place.
Can you please post the whole list of forbidden words? It will be great to have it accompanied with technical response to my and Bart's claims, and to summarize it, it is very simple: "netlink receive should be non-blocking and asynchronous".
Non-blocking and asynchronous is not deadlock, please get it right. Again, provide proof of deadlock, until then refrain from using that word in support of your argument. Which you still have not proved there is a problem.
Chien
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170605060318.GH6868-U/[email protected]

Doug Ledford

2017-06-05 15:08 UTC #30 lore raw

Quoted 11 lines · Leon Romanovsky

Quoted 10 lines · Chien Tin Tung
You jump around in this thread so much it is hard for a sane person to follow
so I will attempt to summarize what has taken place.
You try to revert a patch that fixed a real problem in portmapper, claiming:
1) Code in question impacts the whole RDMA subsystem. which is false by my multiple replies on this.

I haven't seen this as false. Right now the rdma subsystem's netlink usage is pretty small. In the future it will be much larger. Changing the default semantics of a newly opened RDMA netlink socket does in fact impact all of the RDMA subsystem's current and future netlink sockets. So, it *does* impact the whole RDMA subsystem, at least as far as their netlink sockets are concerned.

Quoted 2 lines

Quoted 2 lines
2) There is a deadlock.
Which is false. I'm still asking for proof.

I'm not sure of the deadlock, but as I already pointed out, your fix is not a guaranteed fix. It never was. It just moved the goal post of how hard you would have to push the subsystem before it would fail.

Quoted 2 lines

Quoted 2 lines
3) you want netlink receive to be non-blocking and asynchronous what does that have to do with the non-existing deadlock?

Nothing, it's a separate point and need not be tied to a deadlock. It stands on its own merits.

Quoted 3 lines

Quoted 3 lines
Asnwer is you can't when there isn't one.
If you want it, create a patch for it instead of creating a regression with a lazy revert.

If the original patch broke our semantics so that we have created an ambiguous API for netlink sockets in general, which I will investigate, then this patch is buggy, whether it fixed a problem for you or not, and will have to be reverted eventually. We can coordinate with doing the revert after you've fixed iw_portmapper, but an ambiguous API will not be allowed to remain. See my other emails in this thread.

Quoted 7 lines

Quoted 7 lines
I will continue this discussion if you answer directly to any of those
points. If you choose to dance around the subject and claim falsehood, you
will only damage your own creditibility on the list. I hope you take that
to heart.

Leon is not a native english speaker, as I assume you aren't. I would suggest that it might be more about failure to understand each other and failure to get at the root of the issue than anything that would legitimately do damage to anyone's credibility. You might wish to calm down and read my posts in this thread. I hope I was able to clarify things a bit.

Quoted 11 lines

OK, I got your point. It is worthless discussion.
FYI, ibnl_unicast holds global lock for whole NETLINK_RDMA static void ibnl_rcv(struct sk_buff *skb)
{
mutex_lock(&ibnl_mutex); ibnl_rcv_reply_skb(skb); netlink_rcv_skb(skb, &ibnl_rcv_msg); mutex_unlock(&ibnl_mutex);
}

Quoted 1 line

I'll wait for a maintainer's decision on the proposed patch.

I should be able to finish my investigation by tomorrow (I have two appointments today that will make this a short day for me, unfortunately :-/).

Signature · 9 lines

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1496675296.7171.179.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Chien Tin Tung

2017-06-06 18:17 UTC #32 lore raw

Quoted 8 lines · Doug Ledford

Quoted 1 line · Leon Romanovsky
On Sun, Jun 04, 2017 at 11:50:43PM -0500, Chien Tin Tung wrote:
Leon is not a native english speaker, as I assume you aren't. I would suggest that it might be more about failure to understand each other and failure to get at the root of the issue than anything that would legitimately do damage to anyone's credibility. You might wish to calm down and read my posts in this thread. I hope I was able to clarify things a bit.

Alright, this is my attempt at world peace.

I started with a long email but decided to keep it short. Please ask for details if you have questions and remember to keep an open mind.

Here are some links for those interested in understanding the background leading to this thread:

* Tatyana Nikolova's original commit 30dc5e63d6a5 which introduced ibnl_unicast()
* Leon's comment to Mustafa's original proposal -
https://patchwork.kernel.org/patch/9235137/
* Mustafa Ismail's accepted patch, cea05eadded0d, to ibnl_unicast()

Netlink is fundamentally sockets. You can substitute the word socket for Netlink when you try to get a sense of Netlink functionality. In the RDMA subsystem, we use Netlink to communicate between user-space and kernel.

Current controversy surrounds ibnl_unicast(). It is used to send NL message from the kernel to the user and the NL flow can be triggered from both user-space and kernel code.

ibnl_unicast() uses netlink_unicast with the following signature:

int netlink_unicast(struct sock *ssk, struct sk_buff *skb,
u32 portid, int nonblock)

The last parameter, nonblock, is important. It allows the caller to control the call to wait and retry on failure, provided a timeout value is set, or not block even with a timeout value set. The actual mechanism is very elegant and can be used to solve the issue at hand.

Let me go back to the problem Mustafa tried to solve. Specifically it is an issue with the call to iwpm_add_mapping() failing. That function sends a NL message from the kernel to user-space portmapper daemon using the kernel NL socket. Mustafa originally proposed to have two ibnl_unicast functions, one blocking and the other one non-blocking. It was shot down by Leon. In hindsight, we should of pushed ahead and implemented those two functions. The blocking function for iwpm_add_mapping() and the non-blocking function for any NL interaction from user-space. This solution can still be implemented solving everyone's problem and bring world peace. :-)

Chien
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Doug Ledford

2017-06-05 14:30 UTC #33 lore raw

In reply to a message not in the archive

Quoted 40 lines · Chien Tin Tung

Quoted 39 lines · Leon Romanovsky
Quoted 36 lines · Shiraz Saleem
Quoted 15 lines · Bart Van Assche
Quoted 5 lines · Shiraz Saleem
Quoted 4 lines
5. I proposed a solution -> go and fix your user space program.
Quoted 7 lines
This is a kernel patch you are trying to revert, you are breaking existing
kernel functionality. Nothing to do with user space.
Bottom line, come up with a solution that will address both port mapper
functionality and your issue.
Quoted 7 lines
Hello Shiraz,
Sorry that this means additional work for you, but I agree with Leon that
user space software should not assume that netlink sockets are a reliable
communication mechanism.
Hi Bart - Thank you for your response.
The original problem was that ibnl_unicast, which is used to send nl messages from
portmapper kernel space to user-space, would occasionally and momentarily fail under stress.
We could have retried the call for a certain amount of time, but since netlink_unicast has a
nonblock/block parameter, we chose to use the blocking option with a timeout. So we thought we
did account for deadlocks with this timeout.

Quoted 7 lines

Quoted 3 lines
Not really, you just reduced the chances. In very large scale, you will
have a very large chances of such deadlocks.
Please stop using the word deadlock until you can prove that the deadlock exists with the timeout
in place.

He doesn't need to use the word deadlock for you to know that if you have a non-blocking function that is failing under load, and then you replace it with a blocking function but with a timeout, then it can also fail under load, and therefore you have not really solved the problem.

Signature · 9 lines

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Doug Ledford

2017-06-05 14:29 UTC #34 lore raw

In reply to a message not in the archive

Quoted 38 lines · Leon Romanovsky

Quoted 27 lines · Shiraz Saleem
Quoted 15 lines · Bart Van Assche
Quoted 5 lines · Shiraz Saleem
Quoted 4 lines
5. I proposed a solution -> go and fix your user space program.
Quoted 7 lines
This is a kernel patch you are trying to revert, you are breaking existing
kernel functionality. Nothing to do with user space.
Bottom line, come up with a solution that will address both port mapper
functionality and your issue.
Quoted 7 lines
Hello Shiraz,
Sorry that this means additional work for you, but I agree with Leon that
user space software should not assume that netlink sockets are a reliable
communication mechanism.
Hi Bart - Thank you for your response.
Quoted 9 lines
The original problem was that ibnl_unicast, which is used to send nl messages from
portmapper kernel space to user-space, would occasionally and momentarily fail under stress.
We could have retried the call for a certain amount of time, but since netlink_unicast has a
nonblock/block parameter, we chose to use the blocking option with a timeout. So we thought we
did account for deadlocks with this timeout.

Quoted 3 lines

Not really, you just reduced the chances. In very large scale, you will
have a very large chances of such deadlocks.

Since it has a timeout, it's not a deadlock, it's a state consistency failure. Once it happens, the user space and kernel space port maps will no longer be in sync.

Signature · 9 lines

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Doug Ledford

2017-06-05 14:27 UTC #35 lore raw

In reply to a message not in the archive

Quoted 25 lines · Shiraz Saleem

Quoted 14 lines · Bart Van Assche
Quoted 11 lines · Shiraz Saleem
Quoted 3 lines
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing
kernel functionality. Nothing to do with user space.
Bottom line, come up with a solution that will address both port mapper
functionality and your issue.
Quoted 7 lines
Hello Shiraz,
Sorry that this means additional work for you, but I agree with Leon that
user space software should not assume that netlink sockets are a reliable
communication mechanism.
Hi Bart - Thank you for your response.

Quoted 9 lines

The original problem was that ibnl_unicast, which is used to send nl messages from
portmapper kernel space to user-space, would occasionally and momentarily fail under stress.
We could have retried the call for a certain amount of time, but since netlink_unicast has a
nonblock/block parameter, we chose to use the blocking option with a timeout. So we thought we
did account for deadlocks with this timeout.

Just to be clear, replacing a non-blocking and occasionally dropping function with a blocking function but with a timeout does not actually solve the problem, it merely moves the goal post out. It is entirely possible that you will have the problem again given sufficient load.

Signature · 9 lines

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Leon Romanovsky

author

2017-06-01 04:10 UTC #36 lore raw

In reply to a message not in the archive

Quoted 11 lines · Shiraz Saleem

Quoted 7 lines · Leon Romanovsky
Quoted 3 lines · Shiraz Saleem
Quoted 2 lines · Leon Romanovsky
From: Leon Romanovsky <leonro-Nfu5REtnQAJWk0Htik3J/[email protected]>
3. The commit cea05eadde made libnl library (basic block of user-space part of netlink) to work incorrectly and not according to _blocking/_nonblocking semantics.
How? Is libnl calling ibnl_unicast? As far we can understand ibnl_unicast is only called by portmapper kernel code.

Yes, libnl is calling to ibnl_unicast() and this is why it brought my attention to regression caused by commit which I'm reverting.

Bottom-up flow:

						ibnl_unicast
					send_nlmsg_done
				iwpm_send_mapinfo
			iwpm_mapping_info_cb
		[RDMA_NL_IWPM_MAPINFO] = {.dump = iwpm_mapping_info_cb}
	ibnl_rcv_msg

netlink_rcv_skb(skb, &ibnl_rcv_msg);
-----
libnl:
nl_recvmsgs()

Quoted 8 lines

Quoted 3 lines
4. Reverting is a common practice in Linux kernel. Patches are not carved in stones.
Reverting a patch that's introduced during RC cycle is fine, introducing regression is NOT and that is what you are doing by simply proposing to revert this patch. Reverting this patch will introduce a REGRESSION error with respect to port mapping functionality for all iWARP vendors.

Interesting and how did all these iWARP vendors survive before your patch?

Quoted 4 lines

Quoted 2 lines
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.

Sorry, but it is not the case. The user space is broken.

Thanks

A message that is not in the archive (sent off-list or not yet received): 20170601041022.GM5406-U/[email protected]

Chien Tin Tung

2017-06-02 16:21 UTC #38 lore raw

Quoted 23 lines · Leon Romanovsky

Quoted 11 lines · Shiraz Saleem
Quoted 7 lines · Leon Romanovsky
Quoted 3 lines · Shiraz Saleem
Quoted 2 lines · Leon Romanovsky
From: Leon Romanovsky <leonro-Nfu5REtnQAJWk0Htik3J/[email protected]>
3. The commit cea05eadde made libnl library (basic block of user-space part of netlink) to work incorrectly and not according to _blocking/_nonblocking semantics.
How? Is libnl calling ibnl_unicast? As far we can understand ibnl_unicast is only called by portmapper kernel code.
Yes, libnl is calling to ibnl_unicast() and this is why it brought my attention to regression caused by commit which I'm reverting.
Bottom-up flow:
						ibnl_unicast
					send_nlmsg_done
				iwpm_send_mapinfo
			iwpm_mapping_info_cb
		[RDMA_NL_IWPM_MAPINFO] = {.dump = iwpm_mapping_info_cb}
	ibnl_rcv_msg

Quoted 4 lines

netlink_rcv_skb(skb, &ibnl_rcv_msg);
-----
libnl:
nl_recvmsgs()

Assuming your stack trace is correct, it is showing the normal use case of ibnl_unicate() by portmapper from the iWARP side. I'm still waiting on evidence of this patch breaking rest of RDMA as you so claimed.

Quoted 10 lines

Quoted 7 lines
Quoted 2 lines
4. Reverting is a common practice in Linux kernel. Patches are not carved in stones.
Reverting a patch that's introduced during RC cycle is fine, introducing regression is NOT and that is what you are doing by simply proposing to revert this patch. Reverting this patch will introduce a REGRESSION error with respect to port mapping functionality for all iWARP vendors.
Interesting and how did all these iWARP vendors survive before your patch?

We are all still here. :-) But if we let you simply revert a patch that fixes portmapper for all iWARP vendors then we may be not.

Quoted 3 lines

Quoted 3 lines
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.

Here is a better solution. Post a patch to the kernel that will not introduce a regression and fix whatever the probelm you think there is, then I will personally review the patch.

Chien
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20170602162103.GA12468-TZeIlv3TuzOfrEmaQUPKxl95YUYmaKo1UNDiOz3kqAs@public.gmane.org

Leon Romanovsky

author

2017-06-04 05:31 UTC #40 lore raw

Quoted 29 lines · Chien Tin Tung

Quoted 23 lines · Leon Romanovsky
Quoted 11 lines · Shiraz Saleem
Quoted 7 lines · Leon Romanovsky
Quoted 3 lines · Shiraz Saleem
Quoted 2 lines · Leon Romanovsky
From: Leon Romanovsky <leonro-Nfu5REtnQAJWk0Htik3J/[email protected]>
3. The commit cea05eadde made libnl library (basic block of user-space part of netlink) to work incorrectly and not according to _blocking/_nonblocking semantics.
How? Is libnl calling ibnl_unicast? As far we can understand ibnl_unicast is only called by portmapper kernel code.
Yes, libnl is calling to ibnl_unicast() and this is why it brought my attention to regression caused by commit which I'm reverting.
Bottom-up flow:
						ibnl_unicast
					send_nlmsg_done
				iwpm_send_mapinfo
			iwpm_mapping_info_cb
		[RDMA_NL_IWPM_MAPINFO] = {.dump = iwpm_mapping_info_cb}
	ibnl_rcv_msg
Quoted 4 lines
netlink_rcv_skb(skb, &ibnl_rcv_msg);
-----
libnl:
nl_recvmsgs()

Quoted 3 lines

Assuming your stack trace is correct, it is showing the normal use case of ibnl_unicate() by portmapper from the iWARP side. I'm still waiting on evidence of this patch breaking rest of RDMA as you so claimed.

I think that I understand the differences between our views on the subject. The main difference is if iWARP portmapper is part of large ecosystem (my view) or some standalone feature (your view).

I see iWARP as an integral part of IB core and I do expect that all consumers of NETLINK_RDMA will be complied to libnl library and work in similar manner to whole network stack. I do expect that newcomer won't learn in hard way that his non-blocking calls to netdev NETLINKs and NETLINK_RDMA work except for iWARP portmapper part.

Quoted 9 lines

Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
4. Reverting is a common practice in Linux kernel. Patches are not carved in stones.
Reverting a patch that's introduced during RC cycle is fine, introducing regression is NOT and that is what you are doing by simply proposing to revert this patch. Reverting this patch will introduce a REGRESSION error with respect to port mapping functionality for all iWARP vendors.

Quoted 5 lines

Quoted 2 lines
Interesting and how did all these iWARP vendors survive before your patch?
We are all still here. :-) But if we let you simply revert a patch that fixes portmapper for all iWARP vendors then we may be not.

It is not really fixing but hiding, and it is not related to vendors which want or don't want, it is related to the community and to the right infrastructure for everyone, so everyone will benefit from it.

Quoted 7 lines

Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Here is a better solution. Post a patch to the kernel that will not introduce a regression and fix whatever the probelm you think there is, then I will personally review the patch.

You got proposal, which is not related to kernel.

Quoted 2 lines

Chien

A message that is not in the archive (sent off-list or not yet received): 20170604053123.GC6868-U/[email protected]

Chien Tin Tung

2017-06-05 02:21 UTC #42 lore raw

Quoted 3 lines

I think that I understand the differences between our views on the subject. The main difference is if iWARP portmapper is part of large ecosystem (my view) or some standalone feature (your view).

Please do not speak for me.

Portmapper is a part of the ecosystem but the only user of it is on the iWARP side. It is no different from iw_cm which is only used from iWARP side.

Quoted 5 lines

I see iWARP as an integral part of IB core and I do expect that all consumers of NETLINK_RDMA will be complied to libnl library and work in similar manner to whole network stack. I do expect that newcomer won't learn in hard way that his non-blocking calls to netdev NETLINKs and NETLINK_RDMA work except for iWARP portmapper part.

You are trying to generalize portmapper into something which it is not to justify your argument. Please stop this line of argument. It won't work on me.

Quoted 11 lines

Quoted 11 lines
Quoted 10 lines
Quoted 7 lines
Quoted 2 lines
4. Reverting is a common practice in Linux kernel. Patches are not carved in stones.
Reverting a patch that's introduced during RC cycle is fine, introducing regression is NOT and that is what you are doing by simply proposing to revert this patch. Reverting this patch will introduce a REGRESSION error with respect to port mapping functionality for all iWARP vendors.
Interesting and how did all these iWARP vendors survive before your patch?

Quoted 6 lines

Quoted 2 lines
We are all still here. :-) But if we let you simply revert a patch that fixes portmapper for all iWARP vendors then we may be not.
It is not really fixing but hiding, and it is not related to vendors which want or don't want, it is related to the community and to the right infrastructure for everyone, so everyone will benefit from it.

Again, over generalization.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
Quoted 1 line
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing kernel functionality. Nothing to do with user space.
Here is a better solution. Post a patch to the kernel that will not introduce a regression and fix whatever the probelm you think there is, then I will personally review the patch.
You got proposal, which is not related to kernel.

Still waiting on a usable patch from you, no more unsubstantiated arguments from you.

Chien

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Doug Ledford

2017-06-05 14:55 UTC #43 lore raw

In reply to a message not in the archive

Quoted 5 lines · Shiraz Saleem

Quoted 2 lines · Leon Romanovsky
2. The commit cea05eadde broke NETLINK semantics for whole RDMA.
How did it break? What's the issue?

As I understand it, and I'll let Leon correct me if I'm wrong, the rest of the linux kernel's NETLINK interfaces all have a standard they follow:

When you open a netlink socket, it is by default asynchronous and non- blocking. In order to have it any other way, you must issue the standard libnl commands to switch the socket to the other behavior. Leon's argument is that because of the problem iw_portmapper was having, instead of adding the 3 or 4 lines to iw_portmapper so that it would issue the necessary and typical calls to libnl to change the socket semantics, this patch changed the default symantics for the RDMA subsystems netlink sockets.

The point, then, is that this is not normal accepted practice for solving a problem. Normally, you would fix the user space application. If someone is used to programming network side netlink interfaces, and they then get a new job and end up programming rdma side netlink interfaces, they are going to find this difference confusing, to say the least. We will have broken the normal netlink API for the RDMA subsystem in the name of not fixing a broken user space application.

Does that summarize your argument Leon?

FWIW, I would take this issue very seriously. Having a default netlink socket that behaves one way if it happens to belong to the network subsystem and a default netlink socket that behaves another way if it belongs to the RDMA subsystem is just flat wrong. We should never create an ambiguous API like that. And if we broke it and made it ambiguous, then it absolutely needs fixed.

Quoted 9 lines

Quoted 5 lines
3. The commit cea05eadde made libnl library (basic block of user- space part of netlink)
to work incorrectly and not according to _blocking/_nonblocking semantics.
How? Is libnl calling ibnl_unicast? As far we can understand ibnl_unicast is only called
by portmapper kernel code.

Let's not forget that Leon is working on a new tool, similar to the ip command from iproute2, for configuring the rdma subsystem. Feedback has already established that it needs to be done all via netlink. So, the RDMA subsystem's current use of netlink is but a drop in a bucket compared to what it will be and where it's going. It is highly likely that Leon stumbled across this issue as he was working on this tool and started needing to deal with the RDMA subsystem's netlink support.

Quoted 10 lines

Quoted 2 lines
4. Reverting is a common practice in Linux kernel. Patches are not carved in stones.
Reverting a patch that's introduced during RC cycle is fine, introducing
regression is NOT and that is what you are doing by simply proposing to revert
this patch. Reverting this patch will introduce a REGRESSION error with respect to
port mapping functionality for all iWARP vendors.

Reverting buggy patches, however, is not considered a regression, but a bug fix. This is a bit of a special case in that, if Leon's arguments are right, this is both a bug fix and a regression. It means that there might need to be a flag day to push out a new, fixed iw_portmapper to people, followed by the revert. But just because buggy code is in use does not mean it gets to stay in use.

Quoted 5 lines

Quoted 2 lines
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing
kernel functionality. Nothing to do with user space.

It absolutely has to do with user space. If this is an issue that should have never been fixed via kernel space in the first place and should have been resolved in user space all along by making iw_portmapper treat the RDMA netlink socket just like any network netlink socket would need to be treated, then Leon is right, iw_portmapper needs fixed and this needs reverted.

Quoted 3 lines

Bottom line, come up with a solution that will address both port mapper
functionality and your issue.

No. If he's right (and I plan to investigate whether he is) that the normal netlink socket semantic is asynchronous and non-blocking by default, and you normally use libnl commands to migrate the socket to blocking/synchronous once the socket is opened, then this patch will eventually be reverted and you absolutely need to fix iw_portmapper.

Signature · 9 lines

Doug Ledford <[email protected]>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1496674541.7171.168.camel-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Leon Romanovsky

author

2017-06-05 17:32 UTC #45 lore raw

Quoted 28 lines · Doug Ledford

Quoted 5 lines · Shiraz Saleem
Quoted 2 lines · Leon Romanovsky
2. The commit cea05eadde broke NETLINK semantics for whole RDMA.
How did it break? What's the issue?
As I understand it, and I'll let Leon correct me if I'm wrong, the rest of the linux kernel's NETLINK interfaces all have a standard they follow:
When you open a netlink socket, it is by default asynchronous and non- blocking. In order to have it any other way, you must issue the standard libnl commands to switch the socket to the other behavior. Leon's argument is that because of the problem iw_portmapper was having, instead of adding the 3 or 4 lines to iw_portmapper so that it would issue the necessary and typical calls to libnl to change the socket semantics, this patch changed the default symantics for the RDMA subsystems netlink sockets.
The point, then, is that this is not normal accepted practice for solving a problem. Normally, you would fix the user space application. If someone is used to programming network side netlink interfaces, and they then get a new job and end up programming rdma side netlink interfaces, they are going to find this difference confusing, to say the least. We will have broken the normal netlink API for the RDMA subsystem in the name of not fixing a broken user space application.

Quoted 1 line

Does that summarize your argument Leon?

Thanks Doug,
And to add, it is not only NETLINK_RDMA behaviour vs. all other NETLINKs. It is inconsistency inside NETLINK_RDMA family too. iWARP portmapper is blocking, while rest is non-blocking.

Quoted 18 lines

FWIW, I would take this issue very seriously. Having a default netlink socket that behaves one way if it happens to belong to the network subsystem and a default netlink socket that behaves another way if it belongs to the RDMA subsystem is just flat wrong. We should never create an ambiguous API like that. And if we broke it and made it ambiguous, then it absolutely needs fixed.
Quoted 9 lines
Quoted 5 lines
3. The commit cea05eadde made libnl library (basic block of user- space part of netlink)
to work incorrectly and not according to _blocking/_nonblocking semantics.
How? Is libnl calling ibnl_unicast? As far we can understand ibnl_unicast is only called
by portmapper kernel code.

Quoted 7 lines

Let's not forget that Leon is working on a new tool, similar to the ip command from iproute2, for configuring the rdma subsystem. Feedback has already established that it needs to be done all via netlink. So, the RDMA subsystem's current use of netlink is but a drop in a bucket compared to what it will be and where it's going. It is highly likely that Leon stumbled across this issue as he was working on this tool and started needing to deal with the RDMA subsystem's netlink support.

Right,
And I started from refactoring and cleaning the code.
https://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git/log/?h=topic/rdmatool-rfc-v1
Currently work in progress and I'm trying to minimize "the damage" as much as possible, but future work will include delete of a lot of dead and useless code in iwarp (the whole registration logic is complete trash and can be replaced by very small number of lines), introduce nla_policy to support properly annotated netlink attributes, get rid of custom functions to build messages and many more.

Quoted 48 lines

Quoted 11 lines
Quoted 3 lines
4. Reverting is a common practice in Linux kernel. Patches are not carved in stones.
Reverting a patch that's introduced during RC cycle is fine, introducing
regression is NOT and that is what you are doing by simply proposing to revert
this patch. Reverting this patch will introduce a REGRESSION error with respect to
port mapping functionality for all iWARP vendors.
Reverting buggy patches, however, is not considered a regression, but a bug fix. This is a bit of a special case in that, if Leon's arguments are right, this is both a bug fix and a regression. It means that there might need to be a flag day to push out a new, fixed iw_portmapper to people, followed by the revert. But just because buggy code is in use does not mean it gets to stay in use.
Quoted 5 lines
Quoted 2 lines
5. I proposed a solution -> go and fix your user space program.
This is a kernel patch you are trying to revert, you are breaking existing
kernel functionality. Nothing to do with user space.
It absolutely has to do with user space. If this is an issue that should have never been fixed via kernel space in the first place and should have been resolved in user space all along by making iw_portmapper treat the RDMA netlink socket just like any network netlink socket would need to be treated, then Leon is right, iw_portmapper needs fixed and this needs reverted.
Quoted 3 lines
Bottom line, come up with a solution that will address both port mapper
functionality and your issue.
No. If he's right (and I plan to investigate whether he is) that the normal netlink socket semantic is asynchronous and non-blocking by default, and you normally use libnl commands to migrate the socket to blocking/synchronous once the socket is opened, then this patch will eventually be reverted and you absolutely need to fix iw_portmapper.
--
Doug Ledford <[email protected]> GPG KeyID: B826A3330E572FDD

Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD