Re: [PATCH rdma-next V1 1/9] IB/ipoib: Add warning message when changing the MTU in UD over the max range

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

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

On Thu, Jan 12, 2017 at 01:46:16PM -0500, Doug Ledford wrote:
> On Wed, 2016-12-28 at 14:47 +0200, Leon Romanovsky wrote:
> > From: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > In datagram mode, the IB UD (Unreliable Datagram) transport is used
> > so the MTU of the interface is equal to the IB L2 MTU minus the
> > IPoIB encapsulation header. Any request to change the MTU value
> > above the maximum range will change the MTU to the max allowed, but
> > will not show any warning message. An ipoib_warn is issued in such
> > cases, letting the user know that even though the value is legal,
> > it can't be currently applied.
> >
> > Signed-off-by: Feras Daoud <ferasda-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Noa Osherovich <noaos-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > ---
> >  drivers/infiniband/ulp/ipoib/ipoib_main.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > index 3ce0765..a550cc6 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > @@ -229,6 +229,10 @@ static int ipoib_change_mtu(struct net_device
> > *dev, int new_mtu)
> >  
> >  	priv->admin_mtu = new_mtu;
> >  
> > +	if (priv->mcast_mtu < priv->admin_mtu)
> > +		ipoib_warn(priv, "MTU must be smaller than mcast_mtu
> > (%u)\n",
> > +			   priv->mcast_mtu);
> > +
> >  	dev->mtu = min(priv->mcast_mtu, priv->admin_mtu);
> >  
> >  	return 0;
>
> I don't like this patch.  First, there's no need for a warning here.
>  That's entirely too noisy for this issue.  Second, the wording of the
> message is poor.  The user thinks they are setting the MTU, and there
> is no means of setting a multicast MTU, so telling them that their new
> MTU must be less than the mcast MTU that they can't do anything about
> and don't necessarily know how it is generated makes no sense.  This
> should be no more than ipoib_dbg if we even print anything out at all,
> and the message should be more like "MTU must be <= the link layer MTU
> - 4, use ibv_devinfo on the RDMA device to get the link layer MTU"

First of all, thank you for fixing wording, for me it is the hardest
part of every commit.

Second, I have a different view from you on the issue. User configured
some value, which is not correct for IPoIB. In ideal world (without legacy),
we were supposed to return error to him with proper message, but in our
case (legacy applications) we can't (we tried and it broke some legacy
ifcongfig, if I remember well). So it leaves us with one available
option is to warn user about improper value.

User should know that he supplied wrong parameter.

>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>     GPG KeyID: B826A3330E572FDD
>    
> Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD



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