From mboxrd@z Thu Jan 1 00:00:00 1970 From: Leon Romanovsky Subject: Re: [PATCH V2] Add flow control to the portmapper Date: Mon, 25 Jul 2016 08:29:09 +0300 Message-ID: <20160725052909.GZ20674@leon.nu> References: <1468869810-64420-1-git-send-email-shiraz.saleem@intel.com> <20160719054006.GF20674@leon.nu> <20160719145024.GA69464@ssaleem-MOBL4.amr.corp.intel.com> <20160719173253.GL20674@leon.nu> <20160721024750.GA52712@ssaleem-MOBL4.amr.corp.intel.com> <20160721172942.GW20674@leon.nu> <20160722152601.GA77728@ssaleem-MOBL4.amr.corp.intel.com> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="rwlp4HDnqjg7pxzw" Return-path: Content-Disposition: inline In-Reply-To: <20160722152601.GA77728-GOXS9JX10wfOxmVO0tvppfooFf0ArEBIu+b9c/7xato@public.gmane.org> Sender: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Shiraz Saleem Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, swise-7bPotxP6k4+P2YhJcF5u+vpXobYPEAuW@public.gmane.org, e1000-rdma-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Mustafa Ismail List-Id: linux-rdma@vger.kernel.org --rwlp4HDnqjg7pxzw Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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: > > > > > >=20 > > > > > > You are the one user of this new inline function. > > > > > > Why don't you directly call to netlink_unicast() in your ibnl_u= nicast() > > > > > > without messing with widely visible header file? > > > > >=20 > > > > > Since there is a non-blocking version of nlmsg_unicast(), the ide= a is=20 > > > > > to make a blocking version available to others as well as maintai= n=20 > > > > > consistency of existing code. > > > > >=20 > > > >=20 > > > > In such way, please provide patch series which will convert all oth= er > > > > users to this new call. > > > >=20 > > > > =E2=9E=9C linux-rdma git:(master) grep -rI netlink_unicast * | gre= p -I 0 > > > > kernel/audit.c: err =3D netlink_unicast(audit_sock, skb, audit_nlk_= portid, 0); > > > > kernel/audit.c: netlink_unicast(aunet->nlsk, skb, dest->por= tid, 0); > > > > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->po= rtid, 0); > > > > kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_p= ortid, 0); > > > > samples/connector/cn_test.c: netlink_unicast(nls, skb, 0, 0); > > >=20 > > > These usages of netlink_unicast() with blocking are not the same as t= he new > > > nlmsg_unicast_block() function.=20 > >=20 > > Really? > > Did you look in the code? > > Let's take first function from that grep output > >=20 > > 414 err =3D netlink_unicast(audit_sock, skb, audit_nlk_portid, = 0); > > 415 if (err < 0) { > > ... do something ... > > 437 } else > > ... do something else ... > >=20 > > which fits nicely with your proposal. > >=20 > > +static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff = *skb, u32 portid) > > +{ > > + int err; > > + > > + err =3D netlink_unicast(sk, skb, portid, 0); > > + if (err > 0) > > + err =3D 0; > > + > > + return err; > > +} > >=20 > >=20 > > > You can't drop in nlmsg_unicast_block() in=20 > > > place of netlink_unicast() in these places. I'm not going to introduc= e code=20 > > > which modifies old behavior. > >=20 > > Again, you aren't changing any behaviour. > > Anyway we are not adding general function to common include file just > > because one caller wants it. > >=20 >=20 > We assumed the nlmsg_ API in linux/include/net/netlink.h is there for a p= urpose.=20 > That purpose is to normalize the return code. That API is used in places = where=20 > the return code needs to be normalized, and when normalization is not nee= ded,=20 > then the direct calls are used.=20 >=20 > Now since the nlm_ API in netlink.h is missing a blocking version of the= =20 > nlmsg_unicast function, it would seem reasonable to add it there. >=20 > Changing all the direct calls as you suggest would at the very least be= =20 > less efficient since it would normalize return codes when not needed.=20 One if with one assignment in non data path. Please look at the code. >=20 > However, if there is a strict rule against adding an API unless you immed= iately=20 > have at least 2 callers, then I guess, we will make the direct call. The = amount=20 > of code added will be the same, except that the next person who wants a n= ormalized=20 > return code will have to duplicate the same code. Yes, we are not adding to general header file code which has not multiple callers. >=20 > Changing other code to be less efficient so that we can meet the 2 caller= criteria=20 > doesn't seem reasonable. I'm sorry to hear that you didn't look at the code. >=20 >=20 --rwlp4HDnqjg7pxzw Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXlaOlAAoJEORje4g2clinr0AP/3tzV77S/8zohkddL8kN6frN 4Kve2RZS/U9JyX5A9AAl21WXL85+7Lx5VF1HUghbrByaLQEcEUAnoqAZ3NO+K0Jl 4/0ud7/XXYAMsSeY3tkGLbE+JVx6jKph+pysR8X/GIinzmPsg2rn3f8r8CnzTtnC wGPz9m5UDVE1I1Chguq1e4yimrtLN2aXssofh8ZYr2qBoW9cTj3kNwzePe8tJ2Hs bUhUM+3TvmGSf11VmB7UPMeXafMV6KPhzErWGubz1qRRh3ilbB0uxAfR2Fy2P4VL sQpg5YmSkH5oteflb27mAdkCboJqWA5D715vlIvW3hfMTTR+H3gLOOU+Mbpvv8uL MW599V0ZpB4TRBNd9p69bIfxPb6retLQ0iOg6UkQIOekUR1ymx+RqbGDsaPXhXMB 9bC3mdkkaZ7ey01XYYgpYmYIIgGfhx2G+RS6xKXcoZidLSEADMOLOQV8oNRYCVbP USNgBYrxeew+YTULVlR5M0T5NkTlfCoV4tEPEV1/uweCn1hLMGzcGBCgAx+sS065 UPqNfCbldIc5m2lYTsdzu98qNFbJbJJGmdwKZ/rls01LU/JkTxQCDodWGyzpcfey a1vOWOBR9zv4ESNEe8xHBeSbzwyF8QBBK3jYasMC6mwUzOqHNkDW/0YKMJ9uGfrP 42J7gLqbZ+WfK8jTkzBq =X1we -----END PGP SIGNATURE----- --rwlp4HDnqjg7pxzw-- -- 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