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

Chien Tin Tung
2017-06-02T16:21:03Z

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.

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

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