Re: [PATCH rdma-next] Revert "IB/core: Add flow control to the portmapper netlink calls"

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

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

On Fri, Jun 02, 2017 at 11:21:03AM -0500, Chien Tin Tung wrote:
> On Thu, Jun 01, 2017 at 07:10:22AM +0300, Leon Romanovsky wrote:
> > On Wed, May 31, 2017 at 12:42:45PM -0500, Shiraz Saleem wrote:
> > > On Wed, May 31, 2017 at 07:04:37AM +0300, Leon Romanovsky wrote:
> > > > On Tue, May 30, 2017 at 04:24:31PM -0500, Shiraz Saleem wrote:
> > > > > On Mon, May 29, 2017 at 11:24:23AM +0300, Leon Romanovsky wrote:
> > > > > > From: Leon Romanovsky <leonro-Nfu5REtnQAJWk0Htik3J/w@public.gmane.org>
> > > > > >
> > >
> > > > 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()
>
> 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.

>
> > > > 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.

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.

>
> > > > 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.

>
> Chien

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]