Re: [PATCH RFC 08/12] RDMA/rtrs: Kill recon_cnt from several structs

[Guoqing Jiang <guoqing.jiang@linux.dev>]

On 11/15/22 5:58 PM, Jinpu Wang wrote:
> On Tue, Nov 15, 2022 at 10:53 AM Jinpu Wang <jinpu.wang@ionos.com> wrote:
>> On Tue, Nov 15, 2022 at 10:39 AM Haris Iqbal <haris.iqbal@ionos.com> wrote:
>>> On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>>>> Seems the only relevant comment about recon_cnt is,
>>>>
>>>> /*
>>>>   * On every new session connections increase reconnect counter
>>>>   * to avoid clashes with previous sessions not yet closed
>>>>   * sessions on a server side.
>>>>   */
>>>>
>>>> However, it is not clear how the recon_cnt avoid clashed at these places
>>>> in the commit since no where checks it.
>>> It does seem redundant. This predates my time, so I don't know if
>>> there was a change which removed the usage of this. I tried to search
>>> in commit history, but couldn't.
>>>
>>> @Jinpu Your thoughts?
> Sorry for sending an empty reply.
> I checked a bit, and also can find a reason why the recon_cnt was
> added, it is not checked on the server side.
> I think it's ok to remove it. But we can simply remove the field in
> rtrs_msg_conn_req, commented below.
>
>>>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
>>>> ---
>>>>   drivers/infiniband/ulp/rtrs/rtrs-clt.c | 8 --------
>>>>   drivers/infiniband/ulp/rtrs/rtrs-pri.h | 3 ---
>>>>   drivers/infiniband/ulp/rtrs/rtrs-srv.c | 7 +------
>>>>   3 files changed, 1 insertion(+), 17 deletions(-)
>>>>
>>>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-clt.c b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>> index 5ffc170dae8c..dcc8c041a141 100644
>>>> --- a/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-clt.c
>>>> @@ -1802,7 +1802,6 @@ static int rtrs_rdma_route_resolved(struct rtrs_clt_con *con)
>>>>                  .version = cpu_to_le16(RTRS_PROTO_VER),
>>>>                  .cid = cpu_to_le16(con->c.cid),
>>>>                  .cid_num = cpu_to_le16(clt_path->s.con_num),
>>>> -               .recon_cnt = cpu_to_le16(clt_path->s.recon_cnt),
>>>>          };
>>>>          msg.first_conn = clt_path->for_new_clt ? FIRST_CONN : 0;
>>>>          uuid_copy(&msg.sess_uuid, &clt_path->s.uuid);
>>>> @@ -2336,13 +2335,6 @@ static int init_conns(struct rtrs_clt_path *clt_path)
>>>>          unsigned int cid;
>>>>          int err;
>>>>
>>>> -       /*
>>>> -        * On every new session connections increase reconnect counter
>>>> -        * to avoid clashes with previous sessions not yet closed
>>>> -        * sessions on a server side.
>>>> -        */
>>>> -       clt_path->s.recon_cnt++;
>>>> -
>>>>          /* Establish all RDMA connections  */
>>>>          for (cid = 0; cid < clt_path->s.con_num; cid++) {
>>>>                  err = create_con(clt_path, cid);
>>>> diff --git a/drivers/infiniband/ulp/rtrs/rtrs-pri.h b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
>>>> index a2420eecaf5a..c4ddaeba1c59 100644
>>>> --- a/drivers/infiniband/ulp/rtrs/rtrs-pri.h
>>>> +++ b/drivers/infiniband/ulp/rtrs/rtrs-pri.h
>>>> @@ -109,7 +109,6 @@ struct rtrs_path {
>>>>          struct rtrs_con **con;
>>>>          unsigned int            con_num;
>>>>          unsigned int            irq_con_num;
>>>> -       unsigned int            recon_cnt;
>>>>          unsigned int            signal_interval;
>>>>          struct rtrs_ib_dev      *dev;
>>>>          int                     dev_ref;
>>>> @@ -177,7 +176,6 @@ struct rtrs_sg_desc {
>>>>    * @version:      RTRS protocol version
>>>>    * @cid:          Current connection id
>>>>    * @cid_num:      Number of connections per session
>>>> - * @recon_cnt:    Reconnections counter
>>>>    * @sess_uuid:    UUID of a session (path)
>>>>    * @paths_uuid:           UUID of a group of sessions (paths)
>>>>    *
>>>> @@ -196,7 +194,6 @@ struct rtrs_msg_conn_req {
>>>>          __le16          version;
>>>>          __le16          cid;
>>>>          __le16          cid_num;
>>>> -       __le16          recon_cnt;
>>>>          uuid_t          sess_uuid;
>>>>          uuid_t          paths_uuid;
>>>>          u8              first_conn : 1;
> We can remove it from protocol, this break compatibility with older kernel.

Can we bump up RTRS_PROTO_VER_MINOR for the compatibility? Otherwise
those structs are immutable forever.

Thanks,
Guoqing