On Tue, Nov 15, 2022 at 10:53 AM Jinpu Wang <jinpu.wang@xxxxxxxxx> wrote:
>
> On Tue, Nov 15, 2022 at 10:39 AM Haris Iqbal <haris.iqbal@xxxxxxxxx> wrote:
> >
> > On Sun, Nov 13, 2022 at 2:08 AM Guoqing Jiang <guoqing.jiang@xxxxxxxxx> 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@xxxxxxxxx>
> > > ---
> > > 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.
> > > diff --git a/drivers/infiniband/ulp/rtrs/rtrs-srv.c b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > index 4c883c57c2ef..e2ea09a8def7 100644
> > > --- a/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > +++ b/drivers/infiniband/ulp/rtrs/rtrs-srv.c
> > > @@ -1712,7 +1712,6 @@ static int create_con(struct rtrs_srv_path *srv_path,
> > > static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
> > > struct rdma_cm_id *cm_id,
> > > unsigned int con_num,
> > > - unsigned int recon_cnt,
> > > const uuid_t *uuid)
> > > {
> > > struct rtrs_srv_path *srv_path;
> > > @@ -1768,7 +1767,6 @@ static struct rtrs_srv_path *__alloc_path(struct rtrs_srv_sess *srv,
> > >
> > > srv_path->s.con_num = con_num;
> > > srv_path->s.irq_con_num = con_num;
> > > - srv_path->s.recon_cnt = recon_cnt;
> > > uuid_copy(&srv_path->s.uuid, uuid);
> > > spin_lock_init(&srv_path->state_lock);
> > > INIT_WORK(&srv_path->close_work, rtrs_srv_close_work);
> > > @@ -1818,7 +1816,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> > > struct rtrs_srv_sess *srv;
> > >
> > > u16 version, con_num, cid;
> > > - u16 recon_cnt;
> > > int err = -ECONNRESET;
> > > bool alloc_path = false;
> > >
> > > @@ -1848,7 +1845,6 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> > > pr_err("Incorrect cid: %d >= %d\n", cid, con_num);
> > > goto reject_w_err;
> > > }
> > > - recon_cnt = le16_to_cpu(msg->recon_cnt);
> > > srv = get_or_create_srv(ctx, &msg->paths_uuid, msg->first_conn);
> > > if (IS_ERR(srv)) {
> > > err = PTR_ERR(srv);
> > > @@ -1885,8 +1881,7 @@ static int rtrs_rdma_connect(struct rdma_cm_id *cm_id,
> > > goto reject_w_err;
> > > }
> > > } else {
> > > - srv_path = __alloc_path(srv, cm_id, con_num, recon_cnt,
> > > - &msg->sess_uuid);
> > > + srv_path = __alloc_path(srv, cm_id, con_num, &msg->sess_uuid);
> > > if (IS_ERR(srv_path)) {
> > > mutex_unlock(&srv->paths_mutex);
> > > put_srv(srv);
> > > --
> > > 2.31.1
> > >
