| Message ID | 168261567323.5727.12145565111706096503.stgit@oracle-102.nfsv4bat.org (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Jason Gunthorpe |
| Headers | show |
| Series | [RFC] RDMA/core: Store zero GIDs in some cases | expand |
> -----Original Message----- > From: Chuck Lever <cel@kernel.org> > Sent: Thursday, 27 April 2023 19:15 > To: Bernard Metzler <BMT@zurich.ibm.com> > Cc: linux-rdma@vger.kernel.org; linux-nfs@vger.kernel.org > Subject: [EXTERNAL] [PATCH RFC] RDMA/core: Store zero GIDs in some cases > > From: Bernard Metzler <bmt@zurich.ibm.com> > > Tunnel devices have zero GIDs, so skip the zero GID check when > setting up soft iWARP over a tunnel device. > > Suggested-by: Bernard Metzler <bmt@zurich.ibm.com> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > --- > drivers/infiniband/core/cache.c | 4 +++- > drivers/infiniband/sw/siw/siw_main.c | 1 + > include/rdma/iw_cm.h | 9 ++++++++- > 3 files changed, 12 insertions(+), 2 deletions(-) > > diff --git a/drivers/infiniband/core/cache.c > b/drivers/infiniband/core/cache.c > index 2e91d8879326..2493ca4f2739 100644 > --- a/drivers/infiniband/core/cache.c > +++ b/drivers/infiniband/core/cache.c > @@ -41,6 +41,7 @@ > #include <net/addrconf.h> > > #include <rdma/ib_cache.h> > +#include <rdma/iw_cm.h> > > #include "core_priv.h" > > @@ -441,7 +442,8 @@ static int add_modify_gid(struct ib_gid_table *table, > * leave other unused entries as the zero GID. Convert zero GIDs to > * empty table entries instead of storing them. > */ > - if (rdma_is_zero_gid(&attr->gid)) > + if (rdma_is_zero_gid(&attr->gid) && > + !(attr->device->iw_driver_flags & IW_F_STORE_0GID)) > return 0; > > entry = alloc_gid_entry(attr); > diff --git a/drivers/infiniband/sw/siw/siw_main.c > b/drivers/infiniband/sw/siw/siw_main.c > index dacc174604bf..842a039fa457 100644 > --- a/drivers/infiniband/sw/siw/siw_main.c > +++ b/drivers/infiniband/sw/siw/siw_main.c > @@ -359,6 +359,7 @@ static struct siw_device *siw_device_create(struct > net_device *netdev) > > /* Disable TCP port mapping */ > base_dev->iw_driver_flags = IW_F_NO_PORT_MAP; > + base_dev->iw_driver_flags = IW_F_STORE_0GID; > That overwrites the first assignment. Probably better '|= IW_F_STORE_0GID;' ? Or put them on one line... > sdev->attrs.max_qp = SIW_MAX_QP; > sdev->attrs.max_qp_wr = SIW_MAX_QP_WR; > diff --git a/include/rdma/iw_cm.h b/include/rdma/iw_cm.h > index 03abd30e6c8c..c48f2cbe37b5 100644 > --- a/include/rdma/iw_cm.h > +++ b/include/rdma/iw_cm.h > @@ -90,7 +90,14 @@ enum iw_flags { > * reserve the port. This is required for soft iwarp > * to play in the port mapped iwarp space. > */ > - IW_F_NO_PORT_MAP = (1 << 0), > + IW_F_NO_PORT_MAP = BIT(0), > + > + /* > + * This flag allows the insertion of zero GIDs into the > + * stored GID table. That is needed to enable soft iWARP > + * on tunnel devices. > + */ > + IW_F_STORE_0GID = BIT(1), > }; > > /** >
> On Apr 27, 2023, at 1:46 PM, Bernard Metzler <BMT@zurich.ibm.com> wrote: > > > >> -----Original Message----- >> From: Chuck Lever <cel@kernel.org> >> Sent: Thursday, 27 April 2023 19:15 >> To: Bernard Metzler <BMT@zurich.ibm.com> >> Cc: linux-rdma@vger.kernel.org; linux-nfs@vger.kernel.org >> Subject: [EXTERNAL] [PATCH RFC] RDMA/core: Store zero GIDs in some cases >> >> From: Bernard Metzler <bmt@zurich.ibm.com> >> >> Tunnel devices have zero GIDs, so skip the zero GID check when >> setting up soft iWARP over a tunnel device. >> >> Suggested-by: Bernard Metzler <bmt@zurich.ibm.com> >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> >> --- >> drivers/infiniband/core/cache.c | 4 +++- >> drivers/infiniband/sw/siw/siw_main.c | 1 + >> include/rdma/iw_cm.h | 9 ++++++++- >> 3 files changed, 12 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/infiniband/core/cache.c >> b/drivers/infiniband/core/cache.c >> index 2e91d8879326..2493ca4f2739 100644 >> --- a/drivers/infiniband/core/cache.c >> +++ b/drivers/infiniband/core/cache.c >> @@ -41,6 +41,7 @@ >> #include <net/addrconf.h> >> >> #include <rdma/ib_cache.h> >> +#include <rdma/iw_cm.h> >> >> #include "core_priv.h" >> >> @@ -441,7 +442,8 @@ static int add_modify_gid(struct ib_gid_table *table, >> * leave other unused entries as the zero GID. Convert zero GIDs to >> * empty table entries instead of storing them. >> */ >> - if (rdma_is_zero_gid(&attr->gid)) >> + if (rdma_is_zero_gid(&attr->gid) && >> + !(attr->device->iw_driver_flags & IW_F_STORE_0GID)) >> return 0; >> >> entry = alloc_gid_entry(attr); >> diff --git a/drivers/infiniband/sw/siw/siw_main.c >> b/drivers/infiniband/sw/siw/siw_main.c >> index dacc174604bf..842a039fa457 100644 >> --- a/drivers/infiniband/sw/siw/siw_main.c >> +++ b/drivers/infiniband/sw/siw/siw_main.c >> @@ -359,6 +359,7 @@ static struct siw_device *siw_device_create(struct >> net_device *netdev) >> >> /* Disable TCP port mapping */ >> base_dev->iw_driver_flags = IW_F_NO_PORT_MAP; >> + base_dev->iw_driver_flags = IW_F_STORE_0GID; >> > That overwrites the first assignment. Probably better > '|= IW_F_STORE_0GID;' ? Or put them on one line... D'oh! Will fix. >> sdev->attrs.max_qp = SIW_MAX_QP; >> sdev->attrs.max_qp_wr = SIW_MAX_QP_WR; >> diff --git a/include/rdma/iw_cm.h b/include/rdma/iw_cm.h >> index 03abd30e6c8c..c48f2cbe37b5 100644 >> --- a/include/rdma/iw_cm.h >> +++ b/include/rdma/iw_cm.h >> @@ -90,7 +90,14 @@ enum iw_flags { >> * reserve the port. This is required for soft iwarp >> * to play in the port mapped iwarp space. >> */ >> - IW_F_NO_PORT_MAP = (1 << 0), >> + IW_F_NO_PORT_MAP = BIT(0), >> + >> + /* >> + * This flag allows the insertion of zero GIDs into the >> + * stored GID table. That is needed to enable soft iWARP >> + * on tunnel devices. >> + */ >> + IW_F_STORE_0GID = BIT(1), >> }; >> >> /** -- Chuck Lever
> -----Original Message----- > From: Chuck Lever III <chuck.lever@oracle.com> > Sent: Thursday, 27 April 2023 19:48 > To: Bernard Metzler <BMT@zurich.ibm.com> > Cc: Chuck Lever <cel@kernel.org>; linux-rdma@vger.kernel.org; Linux NFS > Mailing List <linux-nfs@vger.kernel.org> > Subject: [EXTERNAL] Re: [PATCH RFC] RDMA/core: Store zero GIDs in some > cases > > > > > On Apr 27, 2023, at 1:46 PM, Bernard Metzler <BMT@zurich.ibm.com> wrote: > > > > > > > >> -----Original Message----- > >> From: Chuck Lever <cel@kernel.org> > >> Sent: Thursday, 27 April 2023 19:15 > >> To: Bernard Metzler <BMT@zurich.ibm.com> > >> Cc: linux-rdma@vger.kernel.org; linux-nfs@vger.kernel.org > >> Subject: [EXTERNAL] [PATCH RFC] RDMA/core: Store zero GIDs in some cases > >> > >> From: Bernard Metzler <bmt@zurich.ibm.com> > >> > >> Tunnel devices have zero GIDs, so skip the zero GID check when > >> setting up soft iWARP over a tunnel device. > >> > >> Suggested-by: Bernard Metzler <bmt@zurich.ibm.com> > >> Signed-off-by: Chuck Lever <chuck.lever@oracle.com> > >> --- > >> drivers/infiniband/core/cache.c | 4 +++- > >> drivers/infiniband/sw/siw/siw_main.c | 1 + > >> include/rdma/iw_cm.h | 9 ++++++++- > >> 3 files changed, 12 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/infiniband/core/cache.c > >> b/drivers/infiniband/core/cache.c > >> index 2e91d8879326..2493ca4f2739 100644 > >> --- a/drivers/infiniband/core/cache.c > >> +++ b/drivers/infiniband/core/cache.c > >> @@ -41,6 +41,7 @@ > >> #include <net/addrconf.h> > >> > >> #include <rdma/ib_cache.h> > >> +#include <rdma/iw_cm.h> > >> > >> #include "core_priv.h" > >> > >> @@ -441,7 +442,8 @@ static int add_modify_gid(struct ib_gid_table > *table, > >> * leave other unused entries as the zero GID. Convert zero GIDs to > >> * empty table entries instead of storing them. > >> */ > >> - if (rdma_is_zero_gid(&attr->gid)) > >> + if (rdma_is_zero_gid(&attr->gid) && > >> + !(attr->device->iw_driver_flags & IW_F_STORE_0GID)) > >> return 0; > >> > >> entry = alloc_gid_entry(attr); > >> diff --git a/drivers/infiniband/sw/siw/siw_main.c > >> b/drivers/infiniband/sw/siw/siw_main.c > >> index dacc174604bf..842a039fa457 100644 > >> --- a/drivers/infiniband/sw/siw/siw_main.c > >> +++ b/drivers/infiniband/sw/siw/siw_main.c > >> @@ -359,6 +359,7 @@ static struct siw_device *siw_device_create(struct > >> net_device *netdev) > >> > >> /* Disable TCP port mapping */ > >> base_dev->iw_driver_flags = IW_F_NO_PORT_MAP; > >> + base_dev->iw_driver_flags = IW_F_STORE_0GID; > >> > > That overwrites the first assignment. Probably better > > '|= IW_F_STORE_0GID;' ? Or put them on one line... > > D'oh! Will fix. Otherwise looks good of course. Could you please check if that does not break 'normal' Ethernet behavior? I am off from any kernel test infrastructure for the next four (!!) weeks unfortunately. Sorry about that. Thanks for the patch! Bernard. > > >> sdev->attrs.max_qp = SIW_MAX_QP; > >> sdev->attrs.max_qp_wr = SIW_MAX_QP_WR; > >> diff --git a/include/rdma/iw_cm.h b/include/rdma/iw_cm.h > >> index 03abd30e6c8c..c48f2cbe37b5 100644 > >> --- a/include/rdma/iw_cm.h > >> +++ b/include/rdma/iw_cm.h > >> @@ -90,7 +90,14 @@ enum iw_flags { > >> * reserve the port. This is required for soft iwarp > >> * to play in the port mapped iwarp space. > >> */ > >> - IW_F_NO_PORT_MAP = (1 << 0), > >> + IW_F_NO_PORT_MAP = BIT(0), > >> + > >> + /* > >> + * This flag allows the insertion of zero GIDs into the > >> + * stored GID table. That is needed to enable soft iWARP > >> + * on tunnel devices. > >> + */ > >> + IW_F_STORE_0GID = BIT(1), > >> }; > >> > >> /** > > > -- > Chuck Lever >
On Thu, Apr 27, 2023 at 01:14:43PM -0400, Chuck Lever wrote: > From: Bernard Metzler <bmt@zurich.ibm.com> > > Tunnel devices have zero GIDs, so skip the zero GID check when > setting up soft iWARP over a tunnel device. Huh? Why? How does that make any sense? Jason
> On Apr 28, 2023, at 9:39 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Thu, Apr 27, 2023 at 01:14:43PM -0400, Chuck Lever wrote: >> From: Bernard Metzler <bmt@zurich.ibm.com> >> >> Tunnel devices have zero GIDs, so skip the zero GID check when >> setting up soft iWARP over a tunnel device. > > Huh? Why? How does that make any sense? Read it as a cry for help. The scenario is attempting to set up a soft iWARP device with a slave that is a tunnel device. The set up seems to work, but when connecting, the ULP gets an ADDR_ERROR because the setup did not add an entry to the GID table. -- Chuck Lever
On Fri, Apr 28, 2023 at 01:42:24PM +0000, Chuck Lever III wrote: > > > > On Apr 28, 2023, at 9:39 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Thu, Apr 27, 2023 at 01:14:43PM -0400, Chuck Lever wrote: > >> From: Bernard Metzler <bmt@zurich.ibm.com> > >> > >> Tunnel devices have zero GIDs, so skip the zero GID check when > >> setting up soft iWARP over a tunnel device. > > > > Huh? Why? How does that make any sense? > > Read it as a cry for help. > > The scenario is attempting to set up a soft iWARP device > with a slave that is a tunnel device. The set up seems to > work, but when connecting, the ULP gets an ADDR_ERROR > because the setup did not add an entry to the GID table. Don't assign a 0 IP to the tunnel? Jason
> On Apr 28, 2023, at 9:47 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > > On Fri, Apr 28, 2023 at 01:42:24PM +0000, Chuck Lever III wrote: >> >> >>> On Apr 28, 2023, at 9:39 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: >>> >>> On Thu, Apr 27, 2023 at 01:14:43PM -0400, Chuck Lever wrote: >>>> From: Bernard Metzler <bmt@zurich.ibm.com> >>>> >>>> Tunnel devices have zero GIDs, so skip the zero GID check when >>>> setting up soft iWARP over a tunnel device. >>> >>> Huh? Why? How does that make any sense? >> >> Read it as a cry for help. >> >> The scenario is attempting to set up a soft iWARP device >> with a slave that is a tunnel device. The set up seems to >> work, but when connecting, the ULP gets an ADDR_ERROR >> because the setup did not add an entry to the GID table. > > Don't assign a 0 IP to the tunnel? That's a little cryptic... can you expand? Right now I have a Tailscale VPN device with assigned IP addresses: 3: tailscale0: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1280 qdisc fq_codel state UNKNOWN group default qlen 500 link/none inet 100.64.0.16/32 scope global tailscale0 valid_lft forever preferred_lft forever inet6 fd7a:115c:a1e0::10/128 scope global valid_lft forever preferred_lft forever inet6 fe80::725c:1b6d:60ed:fce4/64 scope link stable-privacy valid_lft forever preferred_lft forever And after that i/f is UP, I've done this: $ sudo rdma link add siw0 type siw netdev tailscale0 With the patch I sent, I can do NFS/RDMA via soft iWARP through the tunnel. I'm not at all claiming that's a good fix, but only that this scenario is supposed to work, but currently doesn't. -- Chuck Lever
On Fri, Apr 28, 2023 at 01:58:53PM +0000, Chuck Lever III wrote: > > > > On Apr 28, 2023, at 9:47 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > On Fri, Apr 28, 2023 at 01:42:24PM +0000, Chuck Lever III wrote: > >> > >> > >>> On Apr 28, 2023, at 9:39 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > >>> > >>> On Thu, Apr 27, 2023 at 01:14:43PM -0400, Chuck Lever wrote: > >>>> From: Bernard Metzler <bmt@zurich.ibm.com> > >>>> > >>>> Tunnel devices have zero GIDs, so skip the zero GID check when > >>>> setting up soft iWARP over a tunnel device. > >>> > >>> Huh? Why? How does that make any sense? > >> > >> Read it as a cry for help. > >> > >> The scenario is attempting to set up a soft iWARP device > >> with a slave that is a tunnel device. The set up seems to > >> work, but when connecting, the ULP gets an ADDR_ERROR > >> because the setup did not add an entry to the GID table. > > > > Don't assign a 0 IP to the tunnel? > > That's a little cryptic... can you expand? > > Right now I have a Tailscale VPN device with assigned IP > addresses: > > 3: tailscale0: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1280 qdisc fq_codel state UNKNOWN group default qlen 500 > link/none inet 100.64.0.16/32 scope global tailscale0 > valid_lft forever preferred_lft forever > inet6 fd7a:115c:a1e0::10/128 scope global valid_lft forever preferred_lft forever > inet6 fe80::725c:1b6d:60ed:fce4/64 scope link stable-privacy valid_lft forever preferred_lft forever > > And after that i/f is UP, I've done this: That seems OK.. > $ sudo rdma link add siw0 type siw netdev tailscale0 > > With the patch I sent, I can do NFS/RDMA via soft iWARP through > the tunnel. I'm not at all claiming that's a good fix, but only > that this scenario is supposed to work, but currently doesn't. Then there is something wrong in SIW, it should not be reporting 0 GIDs to the core code for that kind of device. I don't remember what iwarp uses for it's guid format .. Maybe it was mac adress or something and tunnels don't have a MAC, it should make up a dummy GID for the tunnel instead of using 0.. Jason
> -----Original Message----- > From: Jason Gunthorpe <jgg@ziepe.ca> > Sent: Friday, 28 April 2023 16:04 > To: Chuck Lever III <chuck.lever@oracle.com> > Cc: Chuck Lever <cel@kernel.org>; Bernard Metzler <BMT@zurich.ibm.com>; > linux-rdma <linux-rdma@vger.kernel.org>; Linux NFS Mailing List <linux- > nfs@vger.kernel.org> > Subject: [EXTERNAL] Re: [PATCH RFC] RDMA/core: Store zero GIDs in some > cases > > On Fri, Apr 28, 2023 at 01:58:53PM +0000, Chuck Lever III wrote: > > > > > > > On Apr 28, 2023, at 9:47 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > > > > > > On Fri, Apr 28, 2023 at 01:42:24PM +0000, Chuck Lever III wrote: > > >> > > >> > > >>> On Apr 28, 2023, at 9:39 AM, Jason Gunthorpe <jgg@ziepe.ca> wrote: > > >>> > > >>> On Thu, Apr 27, 2023 at 01:14:43PM -0400, Chuck Lever wrote: > > >>>> From: Bernard Metzler <bmt@zurich.ibm.com> > > >>>> > > >>>> Tunnel devices have zero GIDs, so skip the zero GID check when > > >>>> setting up soft iWARP over a tunnel device. > > >>> > > >>> Huh? Why? How does that make any sense? > > >> > > >> Read it as a cry for help. > > >> > > >> The scenario is attempting to set up a soft iWARP device > > >> with a slave that is a tunnel device. The set up seems to > > >> work, but when connecting, the ULP gets an ADDR_ERROR > > >> because the setup did not add an entry to the GID table. > > > > > > Don't assign a 0 IP to the tunnel? > > > > That's a little cryptic... can you expand? > > > > Right now I have a Tailscale VPN device with assigned IP > > addresses: > > > > 3: tailscale0: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1280 > qdisc fq_codel state UNKNOWN group default qlen 500 > > link/none inet 100.64.0.16/32 scope global tailscale0 > > valid_lft forever preferred_lft forever > > inet6 fd7a:115c:a1e0::10/128 scope global valid_lft > forever preferred_lft forever > > inet6 fe80::725c:1b6d:60ed:fce4/64 scope link stable-privacy > valid_lft forever preferred_lft forever > > > > And after that i/f is UP, I've done this: > > That seems OK.. > > > $ sudo rdma link add siw0 type siw netdev tailscale0 > > > > With the patch I sent, I can do NFS/RDMA via soft iWARP through > > the tunnel. I'm not at all claiming that's a good fix, but only > > that this scenario is supposed to work, but currently doesn't. > > Then there is something wrong in SIW, it should not be reporting 0 > GIDs to the core code for that kind of device. > > I don't remember what iwarp uses for it's guid format .. Maybe it was > mac adress or something and tunnels don't have a MAC, it should make > up a dummy GID for the tunnel instead of using 0.. > Yes. it is taken from the MAC address and we don't have one here. I don't remember the iwcm code by heart - maybe any unique GID makes it and there is no need to have the MAC there. Unfortunately I cannot look into it deeper right now since away from my desk a long time, until end of May. Sorry about that confusion. Bernard
diff --git a/drivers/infiniband/core/cache.c b/drivers/infiniband/core/cache.c index 2e91d8879326..2493ca4f2739 100644 --- a/drivers/infiniband/core/cache.c +++ b/drivers/infiniband/core/cache.c @@ -41,6 +41,7 @@ #include <net/addrconf.h> #include <rdma/ib_cache.h> +#include <rdma/iw_cm.h> #include "core_priv.h" @@ -441,7 +442,8 @@ static int add_modify_gid(struct ib_gid_table *table, * leave other unused entries as the zero GID. Convert zero GIDs to * empty table entries instead of storing them. */ - if (rdma_is_zero_gid(&attr->gid)) + if (rdma_is_zero_gid(&attr->gid) && + !(attr->device->iw_driver_flags & IW_F_STORE_0GID)) return 0; entry = alloc_gid_entry(attr); diff --git a/drivers/infiniband/sw/siw/siw_main.c b/drivers/infiniband/sw/siw/siw_main.c index dacc174604bf..842a039fa457 100644 --- a/drivers/infiniband/sw/siw/siw_main.c +++ b/drivers/infiniband/sw/siw/siw_main.c @@ -359,6 +359,7 @@ static struct siw_device *siw_device_create(struct net_device *netdev) /* Disable TCP port mapping */ base_dev->iw_driver_flags = IW_F_NO_PORT_MAP; + base_dev->iw_driver_flags = IW_F_STORE_0GID; sdev->attrs.max_qp = SIW_MAX_QP; sdev->attrs.max_qp_wr = SIW_MAX_QP_WR; diff --git a/include/rdma/iw_cm.h b/include/rdma/iw_cm.h index 03abd30e6c8c..c48f2cbe37b5 100644 --- a/include/rdma/iw_cm.h +++ b/include/rdma/iw_cm.h @@ -90,7 +90,14 @@ enum iw_flags { * reserve the port. This is required for soft iwarp * to play in the port mapped iwarp space. */ - IW_F_NO_PORT_MAP = (1 << 0), + IW_F_NO_PORT_MAP = BIT(0), + + /* + * This flag allows the insertion of zero GIDs into the + * stored GID table. That is needed to enable soft iWARP + * on tunnel devices. + */ + IW_F_STORE_0GID = BIT(1), }; /**