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

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- 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 --]