| Message ID | 1507556073.46071.27.camel@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
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@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. > > 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@mellanox.com> 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@mellanox.com> Reviewed-by: Daniel Jurgens <danielj@mellanox.com> Reported-by: Roland Dreier <roland@purestorage.com> Signed-off-by: Leon Romanovsky <leon@kernel.org> Thanks
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.
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.
> 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@mellanox.com> > 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@mellanox.com> > Reviewed-by: Daniel Jurgens <danielj@mellanox.com> > Reported-by: Roland Dreier <roland@purestorage.com> > Signed-off-by: Leon Romanovsky <leon@kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> -----Original Message----- > From: Amrani, Ram [mailto:Ram.Amrani@cavium.com] > Sent: Tuesday, October 10, 2017 5:30 AM > To: Leon Romanovsky <leon@kernel.org>; Doug Ledford > <dledford@redhat.com>; 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? > > > Fixes: 3c86aa70bf67 ("RDMA/cm: Add RDMA CM support for IBoE devices") > > Signed-off-by: Parav Pandit <parav@mellanox.com> > > Reviewed-by: Daniel Jurgens <danielj@mellanox.com> > > Reported-by: Roland Dreier <roland@purestorage.com> > > Signed-off-by: Leon Romanovsky <leon@kernel.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----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.
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@redhat.com> > GPG Key ID: B826A3330E572FDD > Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57 2FDD >
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.
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;