Re: [PATCH] RDMA/efa: Add RDMA write HW statistics counters

["Margolin, Michael" <mrgolin@amazon.com>]

On 7/4/2023 10:20 AM, Gal Pressman wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On 03/07/2023 18:34, Michael Margolin wrote:
>> Update device API and request RDMA write counters if RDMA write is
>> supported by device. Expose newly added counters through ib core
>> counters mechanism.
>>
>> Reviewed-by: Daniel Kranzdorf <dkkranzd@amazon.com>
>> Reviewed-by: Yonatan Nachum <ynachum@amazon.com>
>> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
>> ---
>>   .../infiniband/hw/efa/efa_admin_cmds_defs.h    | 13 +++++++++++++
>>   drivers/infiniband/hw/efa/efa_com_cmd.c        |  8 +++++++-
>>   drivers/infiniband/hw/efa/efa_com_cmd.h        | 10 +++++++++-
>>   drivers/infiniband/hw/efa/efa_verbs.c          | 18 ++++++++++++++++++
>>   4 files changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h b/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
>> index 4e93ef7f84ee..9c65bd27bae0 100644
>> --- a/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
>> +++ b/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
>> @@ -66,6 +66,7 @@ enum efa_admin_get_stats_type {
>>        EFA_ADMIN_GET_STATS_TYPE_BASIC              = 0,
>>        EFA_ADMIN_GET_STATS_TYPE_MESSAGES           = 1,
>>        EFA_ADMIN_GET_STATS_TYPE_RDMA_READ          = 2,
>> +     EFA_ADMIN_GET_STATS_TYPE_RDMA_WRITE         = 3,
>>   };
>>
>>   enum efa_admin_get_stats_scope {
>> @@ -570,6 +571,16 @@ struct efa_admin_rdma_read_stats {
>>        u64 read_resp_bytes;
>>   };
>>
>> +struct efa_admin_rdma_write_stats {
>> +     u64 write_wrs;
>> +
>> +     u64 write_bytes;
>> +
>> +     u64 write_wr_err;
>> +
>> +     u64 write_recv_bytes;
>> +};
>> +
>>   struct efa_admin_acq_get_stats_resp {
>>        struct efa_admin_acq_common_desc acq_common_desc;
>>
>> @@ -579,6 +590,8 @@ struct efa_admin_acq_get_stats_resp {
>>                struct efa_admin_messages_stats messages_stats;
>>
>>                struct efa_admin_rdma_read_stats rdma_read_stats;
>> +
>> +             struct efa_admin_rdma_write_stats rdma_write_stats;
>>        } u;
>>   };
>>
>> diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.c b/drivers/infiniband/hw/efa/efa_com_cmd.c
>> index 8f8885e002ba..576811885d59 100644
>> --- a/drivers/infiniband/hw/efa/efa_com_cmd.c
>> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.c
>> @@ -1,6 +1,6 @@
>>   // SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause
>>   /*
>> - * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
>> + * Copyright 2018-2023 Amazon.com, Inc. or its affiliates. All rights reserved.
>>    */
>>
>>   #include "efa_com.h"
>> @@ -794,6 +794,12 @@ int efa_com_get_stats(struct efa_com_dev *edev,
>>                result->rdma_read_stats.read_wr_err = resp.u.rdma_read_stats.read_wr_err;
>>                result->rdma_read_stats.read_resp_bytes = resp.u.rdma_read_stats.read_resp_bytes;
>>                break;
>> +     case EFA_ADMIN_GET_STATS_TYPE_RDMA_WRITE:
>> +             result->rdma_write_stats.write_wrs = resp.u.rdma_write_stats.write_wrs;
>> +             result->rdma_write_stats.write_bytes = resp.u.rdma_write_stats.write_bytes;
>> +             result->rdma_write_stats.write_wr_err = resp.u.rdma_write_stats.write_wr_err;
>> +             result->rdma_write_stats.write_recv_bytes = resp.u.rdma_write_stats.write_recv_bytes;
>> +             break;
>>        }
>>
>>        return 0;
>> diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.h b/drivers/infiniband/hw/efa/efa_com_cmd.h
>> index 0898ad5bc340..fc97f37bb39b 100644
>> --- a/drivers/infiniband/hw/efa/efa_com_cmd.h
>> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.h
>> @@ -1,6 +1,6 @@
>>   /* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
>>   /*
>> - * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved.
>> + * Copyright 2018-2023 Amazon.com, Inc. or its affiliates. All rights reserved.
>>    */
>>
>>   #ifndef _EFA_COM_CMD_H_
>> @@ -262,10 +262,18 @@ struct efa_com_rdma_read_stats {
>>        u64 read_resp_bytes;
>>   };
>>
>> +struct efa_com_rdma_write_stats {
>> +     u64 write_wrs;
>> +     u64 write_bytes;
>> +     u64 write_wr_err;
>> +     u64 write_recv_bytes;
>> +};
>> +
>>   union efa_com_get_stats_result {
>>        struct efa_com_basic_stats basic_stats;
>>        struct efa_com_messages_stats messages_stats;
>>        struct efa_com_rdma_read_stats rdma_read_stats;
>> +     struct efa_com_rdma_write_stats rdma_write_stats;
>>   };
>>
>>   int efa_com_create_qp(struct efa_com_dev *edev,
>> diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c
>> index 2a195c4b0f17..7a27d79c0541 100644
>> --- a/drivers/infiniband/hw/efa/efa_verbs.c
>> +++ b/drivers/infiniband/hw/efa/efa_verbs.c
>> @@ -61,6 +61,10 @@ struct efa_user_mmap_entry {
>>        op(EFA_RDMA_READ_BYTES, "rdma_read_bytes") \
>>        op(EFA_RDMA_READ_WR_ERR, "rdma_read_wr_err") \
>>        op(EFA_RDMA_READ_RESP_BYTES, "rdma_read_resp_bytes") \
>> +     op(EFA_RDMA_WRITE_WRS, "rdma_write_wrs") \
>> +     op(EFA_RDMA_WRITE_BYTES, "rdma_write_bytes") \
>> +     op(EFA_RDMA_WRITE_WR_ERR, "rdma_write_wr_err") \
>> +     op(EFA_RDMA_WRITE_RECV_BYTES, "rdma_write_recv_bytes") \
>>
>>   #define EFA_STATS_ENUM(ename, name) ename,
>>   #define EFA_STATS_STR(ename, nam) \
>> @@ -2080,6 +2084,7 @@ static int efa_fill_port_stats(struct efa_dev *dev, struct rdma_hw_stats *stats,
>>   {
>>        struct efa_com_get_stats_params params = {};
>>        union efa_com_get_stats_result result;
>> +     struct efa_com_rdma_write_stats *rws;
>>        struct efa_com_rdma_read_stats *rrs;
>>        struct efa_com_messages_stats *ms;
>>        struct efa_com_basic_stats *bs;
>> @@ -2121,6 +2126,19 @@ static int efa_fill_port_stats(struct efa_dev *dev, struct rdma_hw_stats *stats,
>>        stats->value[EFA_RDMA_READ_WR_ERR] = rrs->read_wr_err;
>>        stats->value[EFA_RDMA_READ_RESP_BYTES] = rrs->read_resp_bytes;
>>
>> +     if (EFA_DEV_CAP(dev, RDMA_WRITE)) {
> I wonder if the same check is missing for RDMA_READ? Or is it redundant
> for RDMA_WRITE?
>
> Patch looks good,
> Reviewed-by: Gal Pressman <gal.pressman@linux.dev>

Thanks Gal, RDMA read is fully deployed so device will always support 
requests for related counters.


Michael