Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Zhu Yanjun]

On Tue, 2021-01-19 at 20:10 +0800, Zhu Yanjun wrote:
> On Tue, Jan 19, 2021 at 6:57 PM <mwilck@suse.com> wrote:
> >
> > From: Martin Wilck <mwilck@suse.com>
> >
> > This reverts commit b2d2440430c0fdd5e0cad3efd6d1c9e3d3d02e5b.
> >
> > It's true that creating rxe on top of 802.1q interfaces doesn't
> > work.
> > Thus, commit fd49ddaf7e26 ("RDMA/rxe: prevent rxe creation on top
> > of vlan interface")
> > was absolutely correct.
> >
> > But b2d2440430c0 was incorrect assuming that with this change,
> > RDMA and VLAN don't work togehter at all. It just has to be
> > set up differently. Rather than creating rxe on top of the VLAN
> > interface, rxe must be created on top of the physical interface.
> > RDMA then works just fine through VLAN interfaces on top of that
> > physical interface, via the "upper device" logic.
>
> I read this commit log for several times. I can not get you.
> Can you show me by an example?

> My test scenario which is broken by your patch uses a script that does
> roughly the following:

> # (set up eth0)
> rdma link add rxe_eth0 type rxe netdev eth0
> ip link add link eth0 name eth0.10 type vlan id 10
> ip link set eth0.10 up
> ip addr add 192.168.10.102/24 dev eth0.10

Thanks a lot.
It seems that the vlan SKBs also enter RXE.

There are 3 hunks in the commit b2d2440430c0("RDMA/rxe: Remove VLAN
code leftovers from RXE").

Can you make more research to find out which hunk causes this problem?

From Jason, vlan is not supported now.
If you want to make more work, the link
https://www.spinics.net/lists/linux-rdma/msg94737.html can give some
tips.

good luck.

Zhu Yanjun

> nvme discover -t rdma -a 192.168.10.101 -s 4420



> 192.168.10.101 is another host that configures the network
> and rxe the same way, and has some nvmet targets.

>  => fails with your patch applied, works otherwise.


> Regards,
> Martin

Re: Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Martin Wilck]

On Wed, 2021-01-20 at 13:33 +0800, Zhu Yanjun wrote:
> On Tue, 2021-01-19 at 20:10 +0800, Zhu Yanjun wrote:
> > On Tue, Jan 19, 2021 at 6:57 PM <mwilck@suse.com> wrote:
> > 
> 
> > My test scenario which is broken by your patch uses a script that
> > does
> > roughly the following:
> 
> > # (set up eth0)
> > rdma link add rxe_eth0 type rxe netdev eth0
> > ip link add link eth0 name eth0.10 type vlan id 10
> > ip link set eth0.10 up
> > ip addr add 192.168.10.102/24 dev eth0.10
> 
> Thanks a lot.
> It seems that the vlan SKBs also enter RXE.
> 
> There are 3 hunks in the commit b2d2440430c0("RDMA/rxe: Remove VLAN
> code leftovers from RXE").
> 
> Can you make more research to find out which hunk causes this
> problem?

I'm positive they all need to be reverted. It's not much code
altogether that is removed, but this much is necessary to make VLANs
work. 

> 
> From Jason, vlan is not supported now.
> If you want to make more work, the link
> https://www.spinics.net/lists/linux-rdma/msg94737.html can give some
> tips.

That's fd49ddaf7e26. Did you notice that I referenced that commit in my
patch description and actually said it was "absolutely correct"?

Anyway, quoting from Mohammad's email: "therefore RXE must behave the
same like HW-RoCE devices and create rxe device per real device only".
This is exactly how the code behaved before your patch was applied.
Or what am I missing?

I have no experience with HW-RoCE. If it's true that RDMA is setup only
"per real device only" there, why would the same thing be wrong for 
SW-RoCE?


Martin

Re: Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Zhu Yanjun]

On Wed, Jan 20, 2021 at 10:30 PM Martin Wilck <mwilck@suse.com> wrote:
>
> On Wed, 2021-01-20 at 13:33 +0800, Zhu Yanjun wrote:
> > On Tue, 2021-01-19 at 20:10 +0800, Zhu Yanjun wrote:
> > > On Tue, Jan 19, 2021 at 6:57 PM <mwilck@suse.com> wrote:
> > >
> >
> > > My test scenario which is broken by your patch uses a script that
> > > does
> > > roughly the following:
> >
> > > # (set up eth0)
> > > rdma link add rxe_eth0 type rxe netdev eth0
> > > ip link add link eth0 name eth0.10 type vlan id 10
> > > ip link set eth0.10 up
> > > ip addr add 192.168.10.102/24 dev eth0.10
> >
> > Thanks a lot.
> > It seems that the vlan SKBs also enter RXE.
> >
> > There are 3 hunks in the commit b2d2440430c0("RDMA/rxe: Remove VLAN
> > code leftovers from RXE").
> >
> > Can you make more research to find out which hunk causes this
> > problem?
>
> I'm positive they all need to be reverted. It's not much code
> altogether that is removed, but this much is necessary to make VLANs

RXE does not support VLAN now.

> work.
>
> >
> > From Jason, vlan is not supported now.
> > If you want to make more work, the link
> > https://www.spinics.net/lists/linux-rdma/msg94737.html can give some
> > tips.
>
> That's fd49ddaf7e26. Did you notice that I referenced that commit in my
> patch description and actually said it was "absolutely correct"?
>
> Anyway, quoting from Mohammad's email: "therefore RXE must behave the
> same like HW-RoCE devices and create rxe device per real device only".
> This is exactly how the code behaved before your patch was applied.
> Or what am I missing?
>
> I have no experience with HW-RoCE. If it's true that RDMA is setup only
> "per real device only" there, why would the same thing be wrong for
> SW-RoCE?
>
>
> Martin
>
>

Re: Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Martin Wilck]

On Wed, 2021-01-20 at 22:44 +0800, Zhu Yanjun wrote:
> On Wed, Jan 20, 2021 at 10:30 PM Martin Wilck <mwilck@suse.com>
> wrote:
> > 
> > On Wed, 2021-01-20 at 13:33 +0800, Zhu Yanjun wrote:
> > > On Tue, 2021-01-19 at 20:10 +0800, Zhu Yanjun wrote:
> > > > On Tue, Jan 19, 2021 at 6:57 PM <mwilck@suse.com> wrote:
> > > > 
> > > 
> > > > My test scenario which is broken by your patch uses a script
> > > > that
> > > > does
> > > > roughly the following:
> > > 
> > > > # (set up eth0)
> > > > rdma link add rxe_eth0 type rxe netdev eth0
> > > > ip link add link eth0 name eth0.10 type vlan id 10
> > > > ip link set eth0.10 up
> > > > ip addr add 192.168.10.102/24 dev eth0.10
> > > 
> > > Thanks a lot.
> > > It seems that the vlan SKBs also enter RXE.
> > > 
> > > There are 3 hunks in the commit b2d2440430c0("RDMA/rxe: Remove
> > > VLAN
> > > code leftovers from RXE").
> > > 
> > > Can you make more research to find out which hunk causes this
> > > problem?
> > 
> > I'm positive they all need to be reverted. It's not much code
> > altogether that is removed, but this much is necessary to make
> > VLANs
> 
> RXE does not support VLAN now.

Yes, because of your patch. What else do you want to tell me with this
statement?

Anyway, Jason seems to agree with you that the way it worked until
5.10, which was fine as far as I could tell, was wrong. I'd still
appreciate some hints explaining what exactly was wrong with the old
code, and how you guys reckon it should work instead. In particular
considering Mohammad's statement I quoted further down. Was Mohammad
wrong?

What I got so far didn't help me much. I'd especially like to
understand how you think the high-level user experience should be. IOW
how would it be set up by the user, and what would the semantics be wrt
permissions, conneting to peers, etc. If I understood that, I might be
able to address the issues and test myself.

Regards,
Martin



> 
> > work.
> > 
> > > 
> > > From Jason, vlan is not supported now.
> > > If you want to make more work, the link
> > > https://www.spinics.net/lists/linux-rdma/msg94737.html can give
> > > some
> > > tips.
> > 
> > That's fd49ddaf7e26. Did you notice that I referenced that commit
> > in my
> > patch description and actually said it was "absolutely correct"?
> > 
> > Anyway, quoting from Mohammad's email: "therefore RXE must behave
> > the
> > same like HW-RoCE devices and create rxe device per real device
> > only".
> > This is exactly how the code behaved before your patch was applied.
> > Or what am I missing?
> > 
> > I have no experience with HW-RoCE. If it's true that RDMA is setup
> > only
> > "per real device only" there, why would the same thing be wrong for
> > SW-RoCE?
> > 
> > 
> > Martin
> > 
> > 
> 

Re: Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Jason Gunthorpe]

On Wed, Jan 20, 2021 at 04:04:44PM +0100, Martin Wilck wrote:
> Anyway, Jason seems to agree with you that the way it worked until
> 5.10, which was fine as far as I could tell, was wrong. I'd still
> appreciate some hints explaining what exactly was wrong with the old
> code, and how you guys reckon it should work instead. In particular
> considering Mohammad's statement I quoted further down. Was Mohammad
> wrong?

In RDMA vlan support revolves around the gid_attr

To have vlan support the device must copy the vlan from the gid_attrs
associated with every tx packet, and match the gid_attr table on every
rx, including the vlan.

For instance, rxe never calls rdma_read_gid_l2_fields to get the
gid_attr for tx, so it doesn't support vlan, at all.

> What I got so far didn't help me much. I'd especially like to
> understand how you think the high-level user experience should be. 

A single rxe device created on the physical netdev. The core code gid
table stuff should import vlan entries of upper vlan net devices and
the general machinery should select those gid table entries when a
vlan is required.

rxe should not be creatable on upper vlan net devices to emulate how
real HW works.

If your use case that work was creating a rxe on a upper vlan device
and relying on the tx of vlan layer to stuff the vlan, then the
problem is how the core code manages the gid table.

Since VLAN is not supported at all in rxe, in that situation the core
code should be filling gid_attrs from the upper vlan netdev only, and
those gid_attrs should have vlan_id set to 0xFFFF.

Then the ib_init_ah_attr_from_wc() will properly match gid table
entries for incoming packets.

There may be other things wrong with the core code because it was
never tested on this situation..

Jason

Re: Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Martin Wilck]

On Wed, 2021-01-20 at 11:19 -0400, Jason Gunthorpe wrote:
> On Wed, Jan 20, 2021 at 04:04:44PM +0100, Martin Wilck wrote:
> > Anyway, Jason seems to agree with you that the way it worked until
> > 5.10, which was fine as far as I could tell, was wrong. I'd still
> > appreciate some hints explaining what exactly was wrong with the
> > old
> > code, and how you guys reckon it should work instead. In particular
> > considering Mohammad's statement I quoted further down. Was
> > Mohammad
> > wrong?
> 
> In RDMA vlan support revolves around the gid_attr
> 
> To have vlan support the device must copy the vlan from the gid_attrs
> associated with every tx packet, and match the gid_attr table on
> every
> rx, including the vlan.
> 
> For instance, rxe never calls rdma_read_gid_l2_fields to get the
> gid_attr for tx, so it doesn't support vlan, at all.
> 
> > What I got so far didn't help me much. I'd especially like to
> > understand how you think the high-level user experience should be. 
> 
> A single rxe device created on the physical netdev. The core code gid
> table stuff should import vlan entries of upper vlan net devices and
> the general machinery should select those gid table entries when a
> vlan is required.
> 
> rxe should not be creatable on upper vlan net devices to emulate how
> real HW works.
> 
> If your use case that work was creating a rxe on a upper vlan device
> and relying on the tx of vlan layer to stuff the vlan, then the
> problem is how the core code manages the gid table.

My use case was creating RXE on the physical device, creating VLANs on
top of the same physical device, and create RDMA connections over these
VLANs. This is what used to work. I have never observed e.g.
interference between RDMA connections over two different VLANs on the
same physical device, or RDMA connections directly on the physical
device.

Thanks for the explanations, anyway.

Regards,
Martin

Re: Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Jason Gunthorpe]

On Wed, Jan 20, 2021 at 04:28:43PM +0100, Martin Wilck wrote:

> > If your use case that work was creating a rxe on a upper vlan device
> > and relying on the tx of vlan layer to stuff the vlan, then the
> > problem is how the core code manages the gid table.
> 
> My use case was creating RXE on the physical device, creating VLANs on
> top of the same physical device, and create RDMA connections over these
> VLANs. This is what used to work. 

Hurm.

So, since rxe never touches the vlan on the tx path, I always thought
it was busted here.

But, it extracts the netdev from the gid_attr and uses that netdev to
tx the skb, which does stuff the correct vlan.

And the rx path, though it is hard to find, does check the gid table
with the vlan, again indirectly through the netdev

So the removed hunk in do_complete is OK

The hunk in rxe_udp_encap_recv is finding the ib_device to use from an
uppser, also OK

The stuff with rxe_dma_device is wrong, just delete that hunk
completely.

So sure, lets apply this - with the rxe_dma_device() part removed
though.

Jason

Re: Revert "RDMA/rxe: Remove VLAN code leftovers from RXE"

[Martin Wilck]

On Wed, 2021-01-20 at 11:45 -0400, Jason Gunthorpe wrote:
> 
> The stuff with rxe_dma_device is wrong, just delete that hunk
> completely.

Right, that function is never called. Still declared in rxe_loc.h,
though.


I'll double check and resubmit.

Thanks
Martin