Is ib_mtu iboe_get_mtu() slightly off?

Is ib_mtu iboe_get_mtu() slightly off?

[Roland Dreier]

We have:

static inline enum ib_mtu iboe_get_mtu(int mtu)
{
        /*
         * reduce IB headers from effective IBoE MTU. 28 stands for
         * atomic header which is the biggest possible header after BTH
         */
        mtu = mtu - IB_GRH_BYTES - IB_BTH_BYTES - 28;
       ...

28 bytes is the size of the AtomicETH header.  But couldn't we have a
packet with both AtomicETH and XRCETH (4 more bytes)?

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

RE: Is ib_mtu iboe_get_mtu() slightly off?

[Parav Pandit]

> From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> owner@vger.kernel.org] On Behalf Of Roland Dreier
> Sent: Wednesday, September 20, 2017 7:39 PM
> To: linux-rdma@vger.kernel.org
> Subject: Is ib_mtu iboe_get_mtu() slightly off?
> 
> We have:
> 
> static inline enum ib_mtu iboe_get_mtu(int mtu) {
>         /*
>          * reduce IB headers from effective IBoE MTU. 28 stands for
>          * atomic header which is the biggest possible header after BTH
>          */
>         mtu = mtu - IB_GRH_BYTES - IB_BTH_BYTES - 28;
>        ...
> 
> 28 bytes is the size of the AtomicETH header.  But couldn't we have a packet
> with both AtomicETH and XRCETH (4 more bytes)?

XRCETH + AtomicETH is possible.
Additionally I guess 4 bytes of ICRC is missing too in calculation.

Re: Is ib_mtu iboe_get_mtu() slightly off?

[Doug Ledford]

On Thu, 2017-09-21 at 02:20 +0000, Parav Pandit wrote:
> > From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> > owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Roland Dreier
> > Sent: Wednesday, September 20, 2017 7:39 PM
> > To: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Subject: Is ib_mtu iboe_get_mtu() slightly off?
> > 
> > We have:
> > 
> > static inline enum ib_mtu iboe_get_mtu(int mtu) {
> >         /*
> >          * reduce IB headers from effective IBoE MTU. 28 stands for
> >          * atomic header which is the biggest possible header after
> > BTH
> >          */
> >         mtu = mtu - IB_GRH_BYTES - IB_BTH_BYTES - 28;
> >        ...
> > 
> > 28 bytes is the size of the AtomicETH header.  But couldn't we have
> > a packet
> > with both AtomicETH and XRCETH (4 more bytes)?
> 
> XRCETH + AtomicETH is possible.
> Additionally I guess 4 bytes of ICRC is missing too in calculation.

Since no one rolled a patch for this, I did.

commit 7107ee2ea38fb52ffe81be054657e90863ebe8ab (HEAD -> k.o/for-next)
Author: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Mon Oct 9 09:26:47 2017 -0400

    RDMA/core: Fix iboe_get_mtu size calculation
    
    We mistakenly used the size of the largest header possible after
    BTH_BYTES without considering that this largest header might actually be
    in combination with a few select other headers.  So, increase the size
    of the largest header number to be the size of the largest possible
    combination of headers instead of just the single largest header.
    
    Fixes: 3c86aa70bf67 (RDMA/cm: Add RDMA CM support for IBoE devices)
    Reported-by: Roland Drier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
    Reported-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
    Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/include/rdma/ib_addr.h b/include/rdma/ib_addr.h
index ec5008cf5d51..45a03c514437 100644
--- a/include/rdma/ib_addr.h
+++ b/include/rdma/ib_addr.h
@@ -245,10 +245,11 @@ static inline void rdma_addr_set_dgid(struct rdma_dev_addr *dev_addr, union ib_g
 static inline enum ib_mtu iboe_get_mtu(int mtu)
 {
        /*
-        * reduce IB headers from effective IBoE MTU. 28 stands for
-        * atomic header which is the biggest possible header after BTH
+        * reduce IB headers from effective IBoE MTU. 36 stands for
+        * AtomicETH + XRCETH + ICRC, which is the biggest header
+        * combination possible after BTH
         */
-       mtu = mtu - IB_GRH_BYTES - IB_BTH_BYTES - 28;
+       mtu = mtu - IB_GRH_BYTES - IB_BTH_BYTES - 36;
 
        if (mtu >= ib_mtu_enum_to_int(IB_MTU_4096))
                return IB_MTU_4096;


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

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

Re: Is ib_mtu iboe_get_mtu() slightly off?

[Leon Romanovsky]

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

On Mon, Oct 09, 2017 at 09:34:33AM -0400, Doug Ledford wrote:
> On Thu, 2017-09-21 at 02:20 +0000, Parav Pandit wrote:
> > > From: linux-rdma-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org [mailto:linux-rdma-
> > > owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org] On Behalf Of Roland Dreier
> > > Sent: Wednesday, September 20, 2017 7:39 PM
> > > To: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > > Subject: Is ib_mtu iboe_get_mtu() slightly off?
> > >
> > > We have:
> > >
> > > static inline enum ib_mtu iboe_get_mtu(int mtu) {
> > >         /*
> > >          * reduce IB headers from effective IBoE MTU. 28 stands for
> > >          * atomic header which is the biggest possible header after
> > > BTH
> > >          */
> > >         mtu = mtu - IB_GRH_BYTES - IB_BTH_BYTES - 28;
> > >        ...
> > >
> > > 28 bytes is the size of the AtomicETH header.  But couldn't we have
> > > a packet
> > > with both AtomicETH and XRCETH (4 more bytes)?
> >
> > XRCETH + AtomicETH is possible.
> > Additionally I guess 4 bytes of ICRC is missing too in calculation.
>
> Since no one rolled a patch for this, I did.
>

Doug,

Parav created, I waited for our shared code.
https://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git/commit/?h=rdma-next&id=51ed6735dd59008aea4d7308416ed994b277f168

commit 51ed6735dd59008aea4d7308416ed994b277f168
Author: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Date:   Thu Oct 5 10:33:12 2017 -0500

    IB/core: Take into account optional XRC header and mandatory ICRC for RoCE MTU

    This fix considers optional XRC header size and mandatory ICRC 4 bytes in
    calculation of path MTU, by considering additional 8 bytes, path MTU calculation
    is more accurate.

    Link: https://www.spinics.net/lists/linux-rdma/msg54558.html
    Fixes: 3c86aa70bf67 ("RDMA/cm: Add RDMA CM support for IBoE devices")
    Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
    Reviewed-by: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
    Reported-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
    Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Thanks

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

Re: Is ib_mtu iboe_get_mtu() slightly off?

[Doug Ledford]

On Mon, 2017-10-09 at 16:41 +0300, Leon Romanovsky wrote:
> On Mon, Oct 09, 2017 at 09:34:33AM -0400, Doug Ledford wrote:
> > 
> > Since no one rolled a patch for this, I did.
> > 
> 
> Doug,
> 
> Parav created, I waited for our shared code.
> https://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git/c
> ommit/?h=rdma-next&id=51ed6735dd59008aea4d7308416ed994b277f168

Ok, but if the issue has been sitting on the list a while, and I don't
know a fix is coming, then there is a not-0 percent chance I'll just
fix it myself.

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

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

RE: Is ib_mtu iboe_get_mtu() slightly off?

[Parav Pandit]

Hi Doug,

> -----Original Message-----
> From: Doug Ledford [mailto:dledford@redhat.com]
> Sent: Monday, October 09, 2017 11:14 AM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: Parav Pandit <parav@mellanox.com>; Roland Dreier
> <roland@purestorage.com>; linux-rdma@vger.kernel.org
> Subject: Re: Is ib_mtu iboe_get_mtu() slightly off?
> 
> On Mon, 2017-10-09 at 16:41 +0300, Leon Romanovsky wrote:
> > On Mon, Oct 09, 2017 at 09:34:33AM -0400, Doug Ledford wrote:
> > >
> > > Since no one rolled a patch for this, I did.
> > >
> >
> > Doug,
> >
> > Parav created, I waited for our shared code.
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit
> > .kernel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fleon%2Flinux-
> rdma.git
> >
> %2Fc&data=02%7C01%7Cparav%40mellanox.com%7C38b1e48942f24ca363ab0
> 8d50f3
> >
> 0c988%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636431624568
> 134878&
> >
> sdata=LOjuNgEAAVuKS9NgN8QO18lPdNI%2BhBFNwaBcuac5AfY%3D&reserved=
> 0
> > ommit/?h=rdma-next&id=51ed6735dd59008aea4d7308416ed994b277f168
> 
> Ok, but if the issue has been sitting on the list a while, and I don't know a fix is
> coming, then there is a not-0 percent chance I'll just fix it myself.
>

Sorry for not ACKing that I was doing it last week.
If possible, please pick the one that Leon is sending because 
instead of doing calculation in half comment based and half defined based code,
my patch defines all necessary header sizes in calculation, little clean way.

Re: Is ib_mtu iboe_get_mtu() slightly off?

[Doug Ledford]

[-- Attachment #1.1: Type: text/plain, Size: 690 bytes --]

On 10/9/2017 4:47 PM, Parav Pandit wrote:

> Sorry for not ACKing that I was doing it last week.
> If possible, please pick the one that Leon is sending because 
> instead of doing calculation in half comment based and half defined based code,
> my patch defines all necessary header sizes in calculation, little clean way.

In this instance we are lucky that 0day testing was being slow yesterday
or else this would already be pushed to k.o.  Since it's not, I can drop
my patch and take yours.


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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

RE: Is ib_mtu iboe_get_mtu() slightly off?

[Parav Pandit]

> -----Original Message-----
> From: Doug Ledford [mailto:dledford@redhat.com]
> Sent: Tuesday, October 10, 2017 9:48 AM
> To: Parav Pandit <parav@mellanox.com>; Leon Romanovsky
> <leon@kernel.org>
> Cc: Roland Dreier <roland@purestorage.com>; linux-rdma@vger.kernel.org
> Subject: Re: Is ib_mtu iboe_get_mtu() slightly off?
> 
> On 10/9/2017 4:47 PM, Parav Pandit wrote:
> 
> > Sorry for not ACKing that I was doing it last week.
> > If possible, please pick the one that Leon is sending because instead
> > of doing calculation in half comment based and half defined based
> > code, my patch defines all necessary header sizes in calculation, little clean
> way.
> 
> In this instance we are lucky that 0day testing was being slow yesterday or else
> this would already be pushed to k.o.  Since it's not, I can drop my patch and take
> yours.
Thanks. Leon will send v1 where UDP header would be covered too.

Re: Is ib_mtu iboe_get_mtu() slightly off?

[Leon Romanovsky]

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

On Tue, Oct 10, 2017 at 10:48:01AM -0400, Doug Ledford wrote:
> On 10/9/2017 4:47 PM, Parav Pandit wrote:
>
> > Sorry for not ACKing that I was doing it last week.
> > If possible, please pick the one that Leon is sending because
> > instead of doing calculation in half comment based and half defined based code,
> > my patch defines all necessary header sizes in calculation, little clean way.
>
> In this instance we are lucky that 0day testing was being slow yesterday
> or else this would already be pushed to k.o.  Since it's not, I can drop
> my patch and take yours.

Did you post your patch for review and gave enough time to participants
to provide the feedback, before pushing it to your k.o.?

Thanks

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




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

Re: Is ib_mtu iboe_get_mtu() slightly off?

[Doug Ledford]

On Tue, 2017-10-10 at 18:33 +0300, Leon Romanovsky wrote:
> On Tue, Oct 10, 2017 at 10:48:01AM -0400, Doug Ledford wrote:
> > On 10/9/2017 4:47 PM, Parav Pandit wrote:
> > 
> > > Sorry for not ACKing that I was doing it last week.
> > > If possible, please pick the one that Leon is sending because
> > > instead of doing calculation in half comment based and half
> > > defined based code,
> > > my patch defines all necessary header sizes in calculation,
> > > little clean way.
> > 
> > In this instance we are lucky that 0day testing was being slow
> > yesterday
> > or else this would already be pushed to k.o.  Since it's not, I can
> > drop
> > my patch and take yours.
> 
> Did you post your patch for review and gave enough time to
> participants
> to provide the feedback, before pushing it to your k.o.?

I posted it proir to pushing it to k.o, and it was based on the thread
as it stood with about a weeks worth of time for people to comment.

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

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

RE: Is ib_mtu iboe_get_mtu() slightly off?

[Amrani, Ram]

> https://git.kernel.org/pub/scm/linux/kernel/git/leon/linux-rdma.git/commit/?h=rdma-next&id=51ed6735dd59008aea4d7308416ed994b277f168
> 
> commit 51ed6735dd59008aea4d7308416ed994b277f168
> Author: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Date:   Thu Oct 5 10:33:12 2017 -0500
> 
>     IB/core: Take into account optional XRC header and mandatory ICRC for RoCE MTU
> 
>     This fix considers optional XRC header size and mandatory ICRC 4 bytes in
>     calculation of path MTU, by considering additional 8 bytes, path MTU calculation
>     is more accurate.
> 
>     Link: https://www.spinics.net/lists/linux-rdma/msg54558.html
>     Fixes: 3c86aa70bf67 ("RDMA/cm: Add RDMA CM support for IBoE devices")
>     Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>     Reviewed-by: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>     Reported-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
>     Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Hi Parav, All,
Shouldn't the overhead calculation take into account RoCEv2 too,
i.e. include the UDP header length?

Thanks,
Ram

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

RE: Is ib_mtu iboe_get_mtu() slightly off?

[Parav Pandit]

> -----Original Message-----
> From: Amrani, Ram [mailto:Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org]
> Sent: Tuesday, October 10, 2017 5:30 AM
> To: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>; Doug Ledford
> <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>; Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; Roland Dreier
> <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>; linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: RE: Is ib_mtu iboe_get_mtu() slightly off?
> 
> >     Fixes: 3c86aa70bf67 ("RDMA/cm: Add RDMA CM support for IBoE devices")
> >     Signed-off-by: Parav Pandit <parav-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >     Reviewed-by: Daniel Jurgens <danielj-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >     Reported-by: Roland Dreier <roland-BHEL68pLQRGGvPXPguhicg@public.gmane.org>
> >     Signed-off-by: Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> 
> Hi Parav, All,
> Shouldn't the overhead calculation take into account RoCEv2 too, i.e. include
> the UDP header length?
> 
Yes. I will roll v1 with inclusion of UDP header as well.
Now that we have RoCEv2 GIDs as default, it make sense to always include UDP header in calculation.
--
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