diff mbox series

[v3,3/3] RDMA/rxe: Add RDMA Atomic Write attribute for rxe device

Message ID 20220311115247.23521-4-yangx.jy@fujitsu.com (mailing list archive)
State Superseded
Delegated to: Jason Gunthorpe
Headers show
Series RDMA/rxe: Add RDMA Atomic Write operation | expand

Commit Message

Xiao Yang March 11, 2022, 11:52 a.m. UTC
The attribute shows that rxe device supports RDMA Atomic Write operation.

Signed-off-by: Xiao Yang <yangx.jy@fujitsu.com>
---
 drivers/infiniband/sw/rxe/rxe_param.h | 3 ++-
 include/rdma/ib_verbs.h               | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

Comments

Jason Gunthorpe March 15, 2022, 6:53 p.m. UTC | #1
On Fri, Mar 11, 2022 at 07:52:47PM +0800, Xiao Yang wrote:

> diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
> index abd1c5d3dc66..580b5cacec09 100644
> +++ b/include/rdma/ib_verbs.h
> @@ -291,6 +291,8 @@ enum ib_device_cap_flags {
>  	/* The device supports padding incoming writes to cacheline. */
>  	IB_DEVICE_PCI_WRITE_END_PADDING		= (1ULL << 36),
>  	IB_DEVICE_ALLOW_USER_UNREG		= (1ULL << 37),
> +	/* Atomic write attributes */
> +	IB_DEVICE_ATOMIC_WRITE			= (1ULL << 40),
>  };

Can you make a patch to clean this up too? The right parts of it need
to get moved to the uapi header and it should work similarly to the
ib_uverbs_wr_opcode / ib_wr_opcode thing.

You'll also need to do something about the 32 bit compatability that
kbuild detected - I suppose this can't work on 32 bit platforms? So
IS_ENABLED() it off or something?

Jason
Xiao Yang March 17, 2022, 5:58 a.m. UTC | #2
On 2022/3/16 2:53, Jason Gunthorpe wrote:
> Can you make a patch to clean this up too? The right parts of it need
> to get moved to the uapi header and it should work similarly to the
> ib_uverbs_wr_opcode / ib_wr_opcode thing.
>
> You'll also need to do something about the 32 bit compatability that
> kbuild detected - I suppose this can't work on 32 bit platforms? So
> IS_ENABLED() it off or something?

Hi Jason,

Thanks for your reminder.

I will do the cleanup and fix the issue detected by kbuild.

Best Regards,

Xiao Yang
Xiao Yang March 21, 2022, 3:55 a.m. UTC | #3
On 2022/3/16 2:53, Jason Gunthorpe wrote:
> You'll also need to do something about the 32 bit compatability that
> kbuild detected - I suppose this can't work on 32 bit platforms? So
> IS_ENABLED() it off or something?
Hi Jason,

Is it possible to fix the issue by atomic64_set_release()?

If not, we may need to add a check for __native_word(*dst) and return an 
unsupported error when __native_word(*dst) is false.

Best Regards,

Xiao Yang
Jason Gunthorpe March 21, 2022, 3:32 p.m. UTC | #4
On Mon, Mar 21, 2022 at 03:55:01AM +0000, yangx.jy@fujitsu.com wrote:
> On 2022/3/16 2:53, Jason Gunthorpe wrote:
> > You'll also need to do something about the 32 bit compatability that
> > kbuild detected - I suppose this can't work on 32 bit platforms? So
> > IS_ENABLED() it off or something?
> Hi Jason,
> 
> Is it possible to fix the issue by atomic64_set_release()?

No

> If not, we may need to add a check for __native_word(*dst) and return an 
> unsupported error when __native_word(*dst) is false.

The whole feature, including the cap bits should be turned off for 32
bit builds because it cannot possibly work

Jason
Xiao Yang March 25, 2022, 11:44 a.m. UTC | #5
On 2022/3/21 23:32, Jason Gunthorpe write:
> On Mon, Mar 21, 2022 at 03:55:01AM +0000, yangx.jy@fujitsu.com wrote:
>> On 2022/3/16 2:53, Jason Gunthorpe wrote:
>>> You'll also need to do something about the 32 bit compatability that
>>> kbuild detected - I suppose this can't work on 32 bit platforms? So
>>> IS_ENABLED() it off or something?
>> Hi Jason,
>>
>> Is it possible to fix the issue by atomic64_set_release()?
> No
>
>> If not, we may need to add a check for __native_word(*dst) and return an
>> unsupported error when __native_word(*dst) is false.
> The whole feature, including the cap bits should be turned off for 32
> bit builds because it cannot possibly work

Hi Jason,

Is it ok to disable the whole atomic write by checking CONFIG_64BIT?

Best Regards,

Xiao Yang

>
> Jason
Jason Gunthorpe March 25, 2022, 1:22 p.m. UTC | #6
On Fri, Mar 25, 2022 at 11:44:53AM +0000, yangx.jy@fujitsu.com wrote:
> On 2022/3/21 23:32, Jason Gunthorpe write:
> > On Mon, Mar 21, 2022 at 03:55:01AM +0000, yangx.jy@fujitsu.com wrote:
> >> On 2022/3/16 2:53, Jason Gunthorpe wrote:
> >>> You'll also need to do something about the 32 bit compatability that
> >>> kbuild detected - I suppose this can't work on 32 bit platforms? So
> >>> IS_ENABLED() it off or something?
> >> Hi Jason,
> >>
> >> Is it possible to fix the issue by atomic64_set_release()?
> > No
> >
> >> If not, we may need to add a check for __native_word(*dst) and return an
> >> unsupported error when __native_word(*dst) is false.
> > The whole feature, including the cap bits should be turned off for 32
> > bit builds because it cannot possibly work
> 
> Hi Jason,
> 
> Is it ok to disable the whole atomic write by checking CONFIG_64BIT?

It is not great, but there is not another choice I can see..

Jason
Xiao Yang March 28, 2022, 10:07 a.m. UTC | #7
On 2022/3/25 21:22, Jason Gunthorpe wrote:
> It is not great, but there is not another choice I can see..

Hi Jason,

I plan to only disable the key places by the following change so that 
user cannot use the atomic write:

-----------------------------------------------------------------------------------------------------------

diff --git a/drivers/infiniband/sw/rxe/rxe_req.c 
b/drivers/infiniband/sw/rxe/rxe_req.c
index e2d332a5af0a..7f83b5e39ace 100644
--- a/drivers/infiniband/sw/rxe/rxe_req.c
+++ b/drivers/infiniband/sw/rxe/rxe_req.c
@@ -238,8 +238,10 @@ static int next_opcode_rc(struct rxe_qp *qp, u32 
opcode, int fits)
                         return fits ? 
IB_OPCODE_RC_SEND_ONLY_WITH_INVALIDATE :
                                 IB_OPCODE_RC_SEND_FIRST;

+#ifdef CONFIG_64BIT
         case IB_WR_RDMA_ATOMIC_WRITE:
                 return IB_OPCODE_RC_RDMA_ATOMIC_WRITE;
+#endif /* CONFIG_64BIT */

         case IB_WR_REG_MR:
         case IB_WR_LOCAL_INV:
diff --git a/drivers/infiniband/sw/rxe/rxe_resp.c 
b/drivers/infiniband/sw/rxe/rxe_resp.c
index ddc791b199e2..311ec523fb57 100644
--- a/drivers/infiniband/sw/rxe/rxe_resp.c
+++ b/drivers/infiniband/sw/rxe/rxe_resp.c
@@ -592,6 +592,7 @@ static enum resp_states process_atomic(struct rxe_qp 
*qp,
         return ret;
  }

+#ifdef CONFIG_64BIT
  static enum resp_states process_atomic_write(struct rxe_qp *qp,
                                              struct rxe_pkt_info *pkt)
  {
@@ -613,6 +614,7 @@ static enum resp_states process_atomic_write(struct 
rxe_qp *qp,

         return RESPST_NONE;
  }
+#endif /* CONFIG_64BIT */

  static struct sk_buff *prepare_ack_packet(struct rxe_qp *qp,
                                           struct rxe_pkt_info *pkt,
@@ -871,10 +873,12 @@ static enum resp_states execute(struct rxe_qp *qp, 
struct rxe_pkt_info *pkt)
                 err = process_atomic(qp, pkt);
                 if (err)
                         return err;
+#ifdef CONFIG_64BIT
         } else if (pkt->mask & RXE_ATOMIC_WRITE_MASK) {
                 err = process_atomic_write(qp, pkt);
                 if (err)
                         return err;
+#endif /* CONFIG_64BIT */
         } else {
                 /* Unreachable */
                 WARN_ON_ONCE(1);

-----------------------------------------------------------------------------------------------------------

Best Regards,

Xiao Yang

>
> Jason
Jason Gunthorpe March 28, 2022, 11:39 a.m. UTC | #8
On Mon, Mar 28, 2022 at 10:07:26AM +0000, yangx.jy@fujitsu.com wrote:
> On 2022/3/25 21:22, Jason Gunthorpe wrote:
> > It is not great, but there is not another choice I can see..
> 
> Hi Jason,
> 
> I plan to only disable the key places by the following change so that 
> user cannot use the atomic write:

Isn't there a cap flag too?

Jason
Xiao Yang March 29, 2022, 2:36 a.m. UTC | #9
On 2022/3/28 19:39, Jason Gunthorpe wrote:
> On Mon, Mar 28, 2022 at 10:07:26AM +0000, yangx.jy@fujitsu.com wrote:
>> On 2022/3/25 21:22, Jason Gunthorpe wrote:
>>> It is not great, but there is not another choice I can see..
>> Hi Jason,
>>
>> I plan to only disable the key places by the following change so that
>> user cannot use the atomic write:
> Isn't there a cap flag too?

Hi Jason,

I will disable the atomic write cap flag as well, like this:

-----------------------------------

diff --git a/drivers/infiniband/sw/rxe/rxe_param.h 
b/drivers/infiniband/sw/rxe/rxe_param.h
index 918270e34a35..88953f9c26e4 100644
--- a/drivers/infiniband/sw/rxe/rxe_param.h
+++ b/drivers/infiniband/sw/rxe/rxe_param.h
@@ -53,7 +53,12 @@ enum rxe_device_param {
                                         | IB_DEVICE_ALLOW_USER_UNREG
                                         | IB_DEVICE_MEM_WINDOW
                                         | IB_DEVICE_MEM_WINDOW_TYPE_2A
+#ifdef CONFIG_64BIT
+                                       | IB_DEVICE_MEM_WINDOW_TYPE_2B
+                                       | IB_DEVICE_ATOMIC_WRITE,
+#else
                                         | IB_DEVICE_MEM_WINDOW_TYPE_2B,
+#endif /* CONFIG_64BIT */
         RXE_MAX_SGE                     = 32,
         RXE_MAX_WQE_SIZE                = sizeof(struct rxe_send_wqe) +
                                           sizeof(struct ib_sge) * 
RXE_MAX_SGE,

-----------------------------------

BTW:

I hope we can review and merge my cleanup patchset[1][2] first. So that 
I will update the third patch[3] based on it. ^_^

[1]: [PATCH v2 1/2] IB/uverbs: Move enum ib_raw_packet_caps to uapi

[2]: [PATCH v2 2/2] IB/uverbs: Move part of enum ib_device_cap_flags to uapi

[3]: [PATCH v3 3/3] RDMA/rxe: Add RDMA Atomic Write attribute for rxe device

Best Regards,

Xiao Yang

>
> Jason
Xiao Yang March 29, 2022, 2:39 a.m. UTC | #10
On 2022/3/29 10:36, yangx.jy@fujitsu.com wrote:
> On 2022/3/28 19:39, Jason Gunthorpe wrote:
>> On Mon, Mar 28, 2022 at 10:07:26AM +0000, yangx.jy@fujitsu.com wrote:
>>> On 2022/3/25 21:22, Jason Gunthorpe wrote:
>>>> It is not great, but there is not another choice I can see..
>>> Hi Jason,
>>>
>>> I plan to only disable the key places by the following change so that
>>> user cannot use the atomic write:
>> Isn't there a cap flag too?
> Hi Jason,
>
> I will disable the atomic write cap flag as well, like this:
>
> -----------------------------------
>
> diff --git a/drivers/infiniband/sw/rxe/rxe_param.h
> b/drivers/infiniband/sw/rxe/rxe_param.h
> index 918270e34a35..88953f9c26e4 100644
> --- a/drivers/infiniband/sw/rxe/rxe_param.h
> +++ b/drivers/infiniband/sw/rxe/rxe_param.h
> @@ -53,7 +53,12 @@ enum rxe_device_param {
>                                           | IB_DEVICE_ALLOW_USER_UNREG
>                                           | IB_DEVICE_MEM_WINDOW
>                                           | IB_DEVICE_MEM_WINDOW_TYPE_2A
> +#ifdef CONFIG_64BIT
> +                                       | IB_DEVICE_MEM_WINDOW_TYPE_2B
> +                                       | IB_DEVICE_ATOMIC_WRITE,
> +#else
>                                           | IB_DEVICE_MEM_WINDOW_TYPE_2B,
> +#endif /* CONFIG_64BIT */
>           RXE_MAX_SGE                     = 32,
>           RXE_MAX_WQE_SIZE                = sizeof(struct rxe_send_wqe) +
>                                             sizeof(struct ib_sge) *
> RXE_MAX_SGE,
>
> -----------------------------------
>
> BTW:
>
> I hope we can review and merge my cleanup patchset[1][2] first. So that
> I will update the third patch[3] based on it. ^_^
>
> [1]: [PATCH v2 1/2] IB/uverbs: Move enum ib_raw_packet_caps to uapi
>
> [2]: [PATCH v2 2/2] IB/uverbs: Move part of enum ib_device_cap_flags to uapi

Sorry, the correct version is my v3 cleanup patchset

[1]: [PATCH v3 1/2] IB/uverbs: Move enum ib_raw_packet_caps to uapi

[2]: [PATCH v3 2/2] IB/uverbs: Move part of enum ib_device_cap_flags to uapi

>
> [3]: [PATCH v3 3/3] RDMA/rxe: Add RDMA Atomic Write attribute for rxe device
>
> Best Regards,
>
> Xiao Yang
>
>> Jason
diff mbox series

Patch

diff --git a/drivers/infiniband/sw/rxe/rxe_param.h b/drivers/infiniband/sw/rxe/rxe_param.h
index 918270e34a35..6ae6c4079639 100644
--- a/drivers/infiniband/sw/rxe/rxe_param.h
+++ b/drivers/infiniband/sw/rxe/rxe_param.h
@@ -53,7 +53,8 @@  enum rxe_device_param {
 					| IB_DEVICE_ALLOW_USER_UNREG
 					| IB_DEVICE_MEM_WINDOW
 					| IB_DEVICE_MEM_WINDOW_TYPE_2A
-					| IB_DEVICE_MEM_WINDOW_TYPE_2B,
+					| IB_DEVICE_MEM_WINDOW_TYPE_2B
+					| IB_DEVICE_ATOMIC_WRITE,
 	RXE_MAX_SGE			= 32,
 	RXE_MAX_WQE_SIZE		= sizeof(struct rxe_send_wqe) +
 					  sizeof(struct ib_sge) * RXE_MAX_SGE,
diff --git a/include/rdma/ib_verbs.h b/include/rdma/ib_verbs.h
index abd1c5d3dc66..580b5cacec09 100644
--- a/include/rdma/ib_verbs.h
+++ b/include/rdma/ib_verbs.h
@@ -291,6 +291,8 @@  enum ib_device_cap_flags {
 	/* The device supports padding incoming writes to cacheline. */
 	IB_DEVICE_PCI_WRITE_END_PADDING		= (1ULL << 36),
 	IB_DEVICE_ALLOW_USER_UNREG		= (1ULL << 37),
+	/* Atomic write attributes */
+	IB_DEVICE_ATOMIC_WRITE			= (1ULL << 40),
 };
 
 enum ib_atomic_cap {