[PATCH V2] Add flow control to the portmapper

[PATCH V2] Add flow control to the portmapper

[Shiraz Saleem]

From: Mustafa Ismail <mustafa.ismail@intel.com>

During connection establishment with a large number of connections,
it is possible that the connection requests might fail. Adding flow
control prevents this failure. Change ibnl unicast to use netlink
messaging with blocking to enable flow control.

Signed-off-by: Faisal Latif <faisal.latif@intel.com>
Signed-off-by: Mustafa Ismail <mustafa.ismail@intel.com>
Signed-off-by: Shiraz Saleem <shiraz.saleem@intel.com>
---

V2: update commit message with justification for flow control. CC'ing
linux-netdev mailing list.
 
 drivers/infiniband/core/netlink.c |  4 ++--
 include/net/netlink.h             | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/core/netlink.c b/drivers/infiniband/core/netlink.c
index 9b8c20c..6b09580 100644
--- a/drivers/infiniband/core/netlink.c
+++ b/drivers/infiniband/core/netlink.c
@@ -229,7 +229,7 @@ static void ibnl_rcv(struct sk_buff *skb)
 int ibnl_unicast(struct sk_buff *skb, struct nlmsghdr *nlh,
 			__u32 pid)
 {
-	return nlmsg_unicast(nls, skb, pid);
+	return nlmsg_unicast_block(nls, skb, pid);
 }
 EXPORT_SYMBOL(ibnl_unicast);
 
@@ -251,7 +251,7 @@ int __init ibnl_init(void)
 		pr_warn("Failed to create netlink socket\n");
 		return -ENOMEM;
 	}
-
+	nls->sk_sndtimeo = 10 * HZ;
 	return 0;
 }
 
diff --git a/include/net/netlink.h b/include/net/netlink.h
index 254a0fc..5434279 100644
--- a/include/net/netlink.h
+++ b/include/net/netlink.h
@@ -598,6 +598,23 @@ static inline int nlmsg_unicast(struct sock *sk, struct sk_buff *skb, u32 portid
 }
 
 /**
+ * nlmsg_unicast_block - unicast a netlink message with blocking
+ * @sk: netlink socket to spread message to
+ * @skb: netlink message as socket buffer
+ * @portid: netlink portid of the destination socket
+ */
+static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff *skb, u32 portid)
+{
+	int err;
+
+	err = netlink_unicast(sk, skb, portid, 0);
+	if (err > 0)
+		err = 0;
+
+	return err;
+}
+
+/**
  * nlmsg_for_each_msg - iterate over a stream of messages
  * @pos: loop counter, set to current message
  * @head: head of message stream
-- 
2.1.3

Re: [PATCH V2] Add flow control to the portmapper

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 597 bytes --]

On Mon, Jul 18, 2016 at 02:23:30PM -0500, Shiraz Saleem wrote:
> From: Mustafa Ismail <mustafa.ismail-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> 
> During connection establishment with a large number of connections,
> it is possible that the connection requests might fail. Adding flow
> control prevents this failure. Change ibnl unicast to use netlink
> messaging with blocking to enable flow control.

You are the one user of this new inline function.
Why don't you directly call to netlink_unicast() in your ibnl_unicast()
without messing with widely visible header file?

Thanks

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] Add flow control to the portmapper

[Shiraz Saleem]

On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> 
> You are the one user of this new inline function.
> Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> without messing with widely visible header file?

Since there is a non-blocking version of nlmsg_unicast(), the idea is 
to make a blocking version available to others as well as maintain 
consistency of existing code.

Re: [PATCH V2] Add flow control to the portmapper

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1067 bytes --]

On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > 
> > You are the one user of this new inline function.
> > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > without messing with widely visible header file?
> 
> Since there is a non-blocking version of nlmsg_unicast(), the idea is 
> to make a blocking version available to others as well as maintain 
> consistency of existing code.
> 

In such way, please provide patch series which will convert all other
users to this new call.

➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
kernel/audit.c:         netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
samples/connector/cn_test.c:    netlink_unicast(nls, skb, 0, 0);

Thanks

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] Add flow control to the portmapper

[Shiraz Saleem]

On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > 
> > > You are the one user of this new inline function.
> > > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > > without messing with widely visible header file?
> > 
> > Since there is a non-blocking version of nlmsg_unicast(), the idea is 
> > to make a blocking version available to others as well as maintain 
> > consistency of existing code.
> > 
> 
> In such way, please provide patch series which will convert all other
> users to this new call.
> 
> ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> kernel/audit.c:         netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
> kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
> kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> samples/connector/cn_test.c:    netlink_unicast(nls, skb, 0, 0);

These usages of netlink_unicast() with blocking are not the same as the new
nlmsg_unicast_block() function. You can't drop in nlmsg_unicast_block() in 
place of netlink_unicast() in these places. I'm not going to introduce code 
which modifies old behavior.

Re: [PATCH V2] Add flow control to the portmapper

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 2259 bytes --]

On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > 
> > > > You are the one user of this new inline function.
> > > > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > > > without messing with widely visible header file?
> > > 
> > > Since there is a non-blocking version of nlmsg_unicast(), the idea is 
> > > to make a blocking version available to others as well as maintain 
> > > consistency of existing code.
> > > 
> > 
> > In such way, please provide patch series which will convert all other
> > users to this new call.
> > 
> > ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > kernel/audit.c:         netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
> > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
> > kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > samples/connector/cn_test.c:    netlink_unicast(nls, skb, 0, 0);
> 
> These usages of netlink_unicast() with blocking are not the same as the new
> nlmsg_unicast_block() function. 

Really?
Did you look in the code?
Let's take first function from that grep output

414         err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
415         if (err < 0) {
			... do something ...
437         } else
			... do something else ...

which fits nicely with your proposal.

+static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff *skb, u32 portid)
+{
+       int err;
+
+       err = netlink_unicast(sk, skb, portid, 0);
+       if (err > 0)
+               err = 0;
+
+       return err;
+}


> You can't drop in nlmsg_unicast_block() in 
> place of netlink_unicast() in these places. I'm not going to introduce code 
> which modifies old behavior.

Again, you aren't changing any behaviour.
Anyway we are not adding general function to common include file just
because one caller wants it.

> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

RE: [PATCH V2] Add flow control to the portmapper

[Steve Wise]

> 
> On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> > On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > >
> > > > > You are the one user of this new inline function.
> > > > > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > > > > without messing with widely visible header file?
> > > >
> > > > Since there is a non-blocking version of nlmsg_unicast(), the idea is
> > > > to make a blocking version available to others as well as maintain
> > > > consistency of existing code.
> > > >
> > >
> > > In such way, please provide patch series which will convert all other
> > > users to this new call.
> > >
> > > ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > > kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > kernel/audit.c:         netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
> > > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
> > > kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > samples/connector/cn_test.c:    netlink_unicast(nls, skb, 0, 0);
> >
> > These usages of netlink_unicast() with blocking are not the same as the new
> > nlmsg_unicast_block() function.
> 
> Really?
> Did you look in the code?
> Let's take first function from that grep output
> 
> 414         err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> 415         if (err < 0) {
> 			... do something ...
> 437         } else
> 			... do something else ...
> 
> which fits nicely with your proposal.
>

The key is to ensure that places calling a blocking service are never called in a non-blocking context.   Leon, do you know if the new sites are always safe to block?  

In general, I think blocking due to sockbuf flow control vs dropping or retrying is a good thing for all the users in the rdam core, assuming they are safe to block.

 
> +static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff *skb, u32
> portid)
> +{
> +       int err;
> +
> +       err = netlink_unicast(sk, skb, portid, 0);
> +       if (err > 0)
> +               err = 0;
> +
> +       return err;
> +}
> 
> 
> > You can't drop in nlmsg_unicast_block() in
> > place of netlink_unicast() in these places. I'm not going to introduce code
> > which modifies old behavior.
> 
> Again, you aren't changing any behaviour.

Potential block/sleep is a change.  But if we can conclude that these additional sites are safe to block, then probably its ok to just go ahead and use the blocking service everywhere.



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH V2] Add flow control to the portmapper

[Steve Wise]

> 
> On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> > On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > >
> > > > > You are the one user of this new inline function.
> > > > > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > > > > without messing with widely visible header file?
> > > >
> > > > Since there is a non-blocking version of nlmsg_unicast(), the idea is
> > > > to make a blocking version available to others as well as maintain
> > > > consistency of existing code.
> > > >
> > >
> > > In such way, please provide patch series which will convert all other
> > > users to this new call.
> > >
> > > ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > > kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > kernel/audit.c:         netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
> > > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
> > > kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > samples/connector/cn_test.c:    netlink_unicast(nls, skb, 0, 0);
> >
> > These usages of netlink_unicast() with blocking are not the same as the new
> > nlmsg_unicast_block() function.
> 
> Really?
> Did you look in the code?
> Let's take first function from that grep output
> 
> 414         err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> 415         if (err < 0) {
> 			... do something ...
> 437         } else
> 			... do something else ...
> 
> which fits nicely with your proposal.
>

The key is to ensure that places calling a blocking service are never called in a non-blocking context.   Leon, do you know if the new sites are always safe to block?  

In general, I think blocking due to sockbuf flow control vs dropping or retrying is a good thing for all the users in the rdam core, assuming they are safe to block.

 
> +static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff *skb, u32
> portid)
> +{
> +       int err;
> +
> +       err = netlink_unicast(sk, skb, portid, 0);
> +       if (err > 0)
> +               err = 0;
> +
> +       return err;
> +}
> 
> 
> > You can't drop in nlmsg_unicast_block() in
> > place of netlink_unicast() in these places. I'm not going to introduce code
> > which modifies old behavior.
> 
> Again, you aren't changing any behaviour.

Potential block/sleep is a change.  But if we can conclude that these additional sites are safe to block, then probably its ok to just go ahead and use the blocking service everywhere.



--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] Add flow control to the portmapper

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3711 bytes --]

On Thu, Jul 21, 2016 at 12:42:42PM -0500, Steve Wise wrote:
> > 
> > On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> > > On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > > > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > > >
> > > > > > You are the one user of this new inline function.
> > > > > > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > > > > > without messing with widely visible header file?
> > > > >
> > > > > Since there is a non-blocking version of nlmsg_unicast(), the idea is
> > > > > to make a blocking version available to others as well as maintain
> > > > > consistency of existing code.
> > > > >
> > > >
> > > > In such way, please provide patch series which will convert all other
> > > > users to this new call.
> > > >
> > > > ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > > > kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > > kernel/audit.c:         netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
> > > > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
> > > > kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > > samples/connector/cn_test.c:    netlink_unicast(nls, skb, 0, 0);
> > >
> > > These usages of netlink_unicast() with blocking are not the same as the new
> > > nlmsg_unicast_block() function.
> > 
> > Really?
> > Did you look in the code?
> > Let's take first function from that grep output
> > 
> > 414         err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > 415         if (err < 0) {
> > 			... do something ...
> > 437         } else
> > 			... do something else ...
> > 
> > which fits nicely with your proposal.
> >
> 
> The key is to ensure that places calling a blocking service are never called in a non-blocking context.   Leon, do you know if the new sites are always safe to block?  
> 
> In general, I think blocking due to sockbuf flow control vs dropping or retrying is a good thing for all the users in the rdam core, assuming they are safe to block.

Steve,
Sorry for my slow response,

I afraid that you was misled by the author of the proposed patch who did
two logical changes in one patch. One is move from non-blocking mode to
blocking mode which is fine enough after justification was added. And
the second change is introduction of general inline function in common
header file (include/net/netlink.h) with one caller only.

This second change is in question and I'm not feeling comfortable by
half done work.

> 
>  
> > +static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff *skb, u32
> > portid)
> > +{
> > +       int err;
> > +
> > +       err = netlink_unicast(sk, skb, portid, 0);
> > +       if (err > 0)
> > +               err = 0;
> > +
> > +       return err;
> > +}
> > 
> > 
> > > You can't drop in nlmsg_unicast_block() in
> > > place of netlink_unicast() in these places. I'm not going to introduce code
> > > which modifies old behavior.
> > 
> > Again, you aren't changing any behaviour.
> 
> Potential block/sleep is a change.  But if we can conclude that these additional sites are safe to block, then probably its ok to just go ahead and use the blocking service everywhere.

These potential sites has the same blocking call now netlink_unicast(... , ... , ... , 0),
the difference and question if they can handle normalized return value from new nlmsg_unicast_block
function. I'm convinced that the answer is yes.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH V2] Add flow control to the portmapper

[Shiraz Saleem]

On Thu, Jul 21, 2016 at 08:29:42PM +0300, Leon Romanovsky wrote:
> On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> > On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > > 
> > > > > You are the one user of this new inline function.
> > > > > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > > > > without messing with widely visible header file?
> > > > 
> > > > Since there is a non-blocking version of nlmsg_unicast(), the idea is 
> > > > to make a blocking version available to others as well as maintain 
> > > > consistency of existing code.
> > > > 
> > > 
> > > In such way, please provide patch series which will convert all other
> > > users to this new call.
> > > 
> > > ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > > kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > kernel/audit.c:         netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
> > > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
> > > kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > samples/connector/cn_test.c:    netlink_unicast(nls, skb, 0, 0);
> > 
> > These usages of netlink_unicast() with blocking are not the same as the new
> > nlmsg_unicast_block() function. 
> 
> Really?
> Did you look in the code?
> Let's take first function from that grep output
> 
> 414         err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> 415         if (err < 0) {
> 			... do something ...
> 437         } else
> 			... do something else ...
> 
> which fits nicely with your proposal.
> 
> +static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff *skb, u32 portid)
> +{
> +       int err;
> +
> +       err = netlink_unicast(sk, skb, portid, 0);
> +       if (err > 0)
> +               err = 0;
> +
> +       return err;
> +}
> 
> 
> > You can't drop in nlmsg_unicast_block() in 
> > place of netlink_unicast() in these places. I'm not going to introduce code 
> > which modifies old behavior.
> 
> Again, you aren't changing any behaviour.
> Anyway we are not adding general function to common include file just
> because one caller wants it.
> 

We assumed the nlmsg_ API in linux/include/net/netlink.h is there for a purpose. 
That purpose is to normalize the return code. That API is used in places where 
the return code needs to be normalized, and when normalization is not needed, 
then the direct calls are used. 

Now since the nlm_ API in netlink.h is missing a blocking version of the 
nlmsg_unicast function, it would seem reasonable to add it there.

Changing all the direct calls as you suggest would at the very least be 
less efficient since it would normalize return codes when not needed. 

However, if there is a strict rule against adding an API unless you immediately 
have at least 2 callers, then I guess, we will make the direct call. The amount 
of code added will be the same, except that the next person who wants a normalized 
return code will have to duplicate the same code.

Changing other code to be less efficient so that we can meet the 2 caller criteria 
doesn't seem reasonable.


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V2] Add flow control to the portmapper

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 3878 bytes --]

On Fri, Jul 22, 2016 at 10:26:01AM -0500, Shiraz Saleem wrote:
> On Thu, Jul 21, 2016 at 08:29:42PM +0300, Leon Romanovsky wrote:
> > On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> > > On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > > > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > > > 
> > > > > > You are the one user of this new inline function.
> > > > > > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > > > > > without messing with widely visible header file?
> > > > > 
> > > > > Since there is a non-blocking version of nlmsg_unicast(), the idea is 
> > > > > to make a blocking version available to others as well as maintain 
> > > > > consistency of existing code.
> > > > > 
> > > > 
> > > > In such way, please provide patch series which will convert all other
> > > > users to this new call.
> > > > 
> > > > ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > > > kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > > kernel/audit.c:         netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
> > > > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
> > > > kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > > > samples/connector/cn_test.c:    netlink_unicast(nls, skb, 0, 0);
> > > 
> > > These usages of netlink_unicast() with blocking are not the same as the new
> > > nlmsg_unicast_block() function. 
> > 
> > Really?
> > Did you look in the code?
> > Let's take first function from that grep output
> > 
> > 414         err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > 415         if (err < 0) {
> > 			... do something ...
> > 437         } else
> > 			... do something else ...
> > 
> > which fits nicely with your proposal.
> > 
> > +static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff *skb, u32 portid)
> > +{
> > +       int err;
> > +
> > +       err = netlink_unicast(sk, skb, portid, 0);
> > +       if (err > 0)
> > +               err = 0;
> > +
> > +       return err;
> > +}
> > 
> > 
> > > You can't drop in nlmsg_unicast_block() in 
> > > place of netlink_unicast() in these places. I'm not going to introduce code 
> > > which modifies old behavior.
> > 
> > Again, you aren't changing any behaviour.
> > Anyway we are not adding general function to common include file just
> > because one caller wants it.
> > 
> 
> We assumed the nlmsg_ API in linux/include/net/netlink.h is there for a purpose. 
> That purpose is to normalize the return code. That API is used in places where 
> the return code needs to be normalized, and when normalization is not needed, 
> then the direct calls are used. 
> 
> Now since the nlm_ API in netlink.h is missing a blocking version of the 
> nlmsg_unicast function, it would seem reasonable to add it there.
> 
> Changing all the direct calls as you suggest would at the very least be 
> less efficient since it would normalize return codes when not needed. 

One if with one assignment in non data path.
Please look at the code.

> 
> However, if there is a strict rule against adding an API unless you immediately 
> have at least 2 callers, then I guess, we will make the direct call. The amount 
> of code added will be the same, except that the next person who wants a normalized 
> return code will have to duplicate the same code.

Yes, we are not adding to general header file code which has not
multiple callers.

> 
> Changing other code to be less efficient so that we can meet the 2 caller criteria 
> doesn't seem reasonable.

I'm sorry to hear that you didn't look at the code.

> 
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]