Re: [PATCH for-next v3 0/2] RDMA/rxe: RDMA FLUSH and ATOMIC WRITE with ODP

[Leon Romanovsky <leon@kernel.org>]

On Fri, Apr 18, 2025 at 02:07:49AM +0000, Daisuke Matsuda (Fujitsu) wrote:
> On Thu, April 17, 2025 1:59 AM Leon Romanovsky wrote:
> > On Mon, Apr 14, 2025 at 02:56:51PM +0200, Zhu Yanjun wrote:
> > > On 14.04.25 12:16, Daisuke Matsuda (Fujitsu) wrote:
> > > > On Sat, April 12, 2025 2:55 AM Leon Romanovsky wrote:
> > > > > > Hi Leon,
> > > > > >
> > > > > > I have noticed the 2nd patch caused "kernel test robot" error, and you
> > > > > > kindly amended the patch. However, another error has been detected by "the bot"
> > > > > > because of the remaining fundamental problem that ATOMIC WRITE cannot
> > > > > > be executed on non-64-bit architectures (at least on rxe).
> > > > > >
> > > > > > I think applying the change below to the original patch(*1) will resolve the issue.
> > > > > > (*1) https://lore.kernel.org/linux-rdma/20250324075649.3313968-3-matsuda-daisuke@fujitsu.com/
> > > > > > ```
> > > > > > diff --git a/drivers/infiniband/sw/rxe/rxe_odp.c b/drivers/infiniband/sw/rxe/rxe_odp.c
> > > > > > index 02de05d759c6..ac3b3039db22 100644
> > > > > > --- a/drivers/infiniband/sw/rxe/rxe_odp.c
> > > > > > +++ b/drivers/infiniband/sw/rxe/rxe_odp.c
> > > > > > @@ -380,6 +380,7 @@ int rxe_odp_flush_pmem_iova(struct rxe_mr *mr, u64 iova,
> > > > > >   }
> > > > > >
> > > > > >   /* CONFIG_64BIT=y */
> > > > > > +#ifdef CONFIG_64BIT
> > > > > >   enum resp_states rxe_odp_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
> > > > > >   {
> > > > > >          struct ib_umem_odp *umem_odp = to_ib_umem_odp(mr->umem);
> > > > > > @@ -424,3 +425,4 @@ enum resp_states rxe_odp_do_atomic_write(struct rxe_mr *mr, u64 iova, u64 value)
> > > > > >
> > > > > >          return RESPST_NONE;
> > > > > >   }
> > > > > > +#endif
> > > > > > ```
> > > > > > The definition of rxe_odp_do_atomic_write() should have been guarded in #ifdef CONFIG_64BIT.
> > > > > > I believe this fix can address both:
> > > > > >    - the first error "error: redefinition of 'rxe_odp_do_atomic_write' " that could be caused when
> > > > > >      CONFIG_INFINIBAND_ON_DEMAND_PAGING=y && CONFIG_64BIT=n.
> > > > > >    - the second error caused by trying to compile 64-bit codes on 32-bit architectures.
> > > > > >
> > > > > > I am very sorry to bother you, but is it possible to make the modification?
> > > > > > If I should provide a replacement patch, I will do so.
> > > > >
> > > > > I think that better will be simply make sure that RXE is dependent on 64bits.
> > > > >
> > > > > diff --git a/drivers/infiniband/sw/rxe/Kconfig b/drivers/infiniband/sw/rxe/Kconfig
> > > > > index c180e7ebcfc5..1ed5b63f8afc 100644
> > > > > --- a/drivers/infiniband/sw/rxe/Kconfig
> > > > > +++ b/drivers/infiniband/sw/rxe/Kconfig
> > > > > @@ -1,7 +1,7 @@
> > > > >   # SPDX-License-Identifier: GPL-2.0-only
> > > > >   config RDMA_RXE
> > > > >          tristate "Software RDMA over Ethernet (RoCE) driver"
> > > > > -       depends on INET && PCI && INFINIBAND
> > > > > +       depends on INET && PCI && INFINIBAND && 64BIT
> > > > >          depends on INFINIBAND_VIRT_DMA
> > > > >          select NET_UDP_TUNNEL
> > > > >          select CRC32
> > > > >
> > > > > WDYT?
> > > >
> > > > It seems the driver is designed to be runnable on 32-bit nodes, so it may be
> > > > overkill to disable 32-bit mode only for "ATOMIC WRITE" functionality.
> > > > However, I do not have strong objection to making this change if you
> > > > think it is better in terms of maintainability.
> > > >
> > > > Before making the change, I'd like to get an ACK or NACK from Zhu Yanjun.
> > > > As far as I am aware, no one is actively maintaining or testing RXE on 32-bit,
> > > > so it may be acceptable to drop 32-bit support, but it's best to confirm before proceeding.
> > >
> > > Hi, Daisuke Matsuda
> > >
> > > Thanks a lot for your efforts.
> > >
> > > There are some problems with 32-bit architectures, such as Year 2038 problem
> > > ( many 32-bit systems will stop working in the year 2038 when the 32-bit
> > > time_t overflows).
> > >
> > > And many binary distributions, like Fedora, Ubuntu, and openSUSE Leap, have
> > > dropped support for all 32-bit architectures other than Armv7 and are likely
> > > to drop that as well before they would consider rebuilding against a new
> > > glibc.
> > >
> > > In the kernel 6.15, support for larger 32-bit x86 systems (those with more
> > > than eight CPUs or more than 4GB of RAM) has been removed. Those hardware
> > > configurations have been unavailable for a long time, and any workloads
> > > needing such resources should have long since moved to 64-bit systems.
> > >
> > > Thus, it seems that it is a trend to not support 32-bit architecture in
> > > Linux kernel. In rxe, we will also follow this trend.
> > >
> > > If some user-space applications still use 32-bit architecture currently, we
> > > can apply your commit. But from Linux kernel community, sooner or later, the
> > > support of 32-bit architecture will be dropped.
> > >
> > > Finally if some user-space applications still need 32-bit architecture in
> > > rxe, we can keep it. Or else, we will follow Leon's advice.
> > >
> > >
> > > It is just my 2-cent advice.^_^
> > >
> > > Please Jason Gunthorpe or Leon Romanovsky comments on this.
> > 
> > At the end RXE is for development, testing and early prototyping. I can't
> > believe that we have developers who are using 32bits machines for such type
> > of work in RDMA domain.
> 
> Agreed.
> Could you modify "RDMA/rxe: Enable ODP in ATOMIC WRITE operation"
> in the tree and change Kconfig as you suggested?

Just send separate patch, please.

> 
> Thanks,
> Daisuke
>