Re: [PATCHv2 for-next 3/5] RDMA/rtrs-clt: Rename rtrs_clt_sess to rtrs_clt_path

[Vaishali Thakkar <vaishali.thakkar@ionos.com>]

On Tue, Jan 4, 2022 at 7:52 AM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
>

Thanks for the reviews, Guoqing.

> On 1/3/22 9:33 PM, Jack Wang wrote:
>
> [ .. ]
>
> > -static int post_recv_sess(struct rtrs_clt_sess *sess)
> > +static int post_recv_path(struct rtrs_clt_path *clt_path)
>
> How about rename it to post_recv_clt_path to make it obviously if you
> has the same change to post_recv_sess in rtrs-srv.c?
>
> [ ... ]
>
> > @@ -1378,13 +1383,14 @@ static int alloc_sess_reqs(struct rtrs_clt_sess *sess)
> >               if (!req->sge)
> >                       goto out;
> >
> > -             req->mr = ib_alloc_mr(sess->s.dev->ib_pd, IB_MR_TYPE_MEM_REG,
> > -                                   sess->max_pages_per_mr);
> > +             req->mr = ib_alloc_mr(clt_path->s.dev->ib_pd,
> > +                                   IB_MR_TYPE_MEM_REG,
> > +                                   clt_path->max_pages_per_mr);
> >               if (IS_ERR(req->mr)) {
> >                       err = PTR_ERR(req->mr);
> >                       req->mr = NULL;
> >                       pr_err("Failed to alloc sess->max_pages_per_mr %d\n",
>
> s/sess/clt_path/

Good catch.

> > -                            sess->max_pages_per_mr);
> > +                            clt_path->max_pages_per_mr);
> >                       goto out;
> >               }
> >
>
> [ ... ]
>
> > -static int create_con(struct rtrs_clt_sess *sess, unsigned int cid)
> > +static int create_con(struct rtrs_clt_path *clt_path, unsigned int cid)
>
> Could you rename it to create_clt_con? Because the function name appears
> in both client and server.

I think renaming the functions with the same name on client and
server side should be a seperate patch/series as these patches
are only for fixing the incorrect naming.

> >   {
> >       struct rtrs_clt_con *con;
> >
> > @@ -1601,28 +1609,28 @@ static int create_con(struct rtrs_clt_sess *sess, unsigned int cid)
> >       /* Map first two connections to the first CPU */
> >       con->cpu  = (cid ? cid - 1 : 0) % nr_cpu_ids;
> >       con->c.cid = cid;
> > -     con->c.path = &sess->s;
> > +     con->c.path = &clt_path->s;
> >       /* Align with srv, init as 1 */
> >       atomic_set(&con->c.wr_cnt, 1);
> >       mutex_init(&con->con_mutex);
> >
> > -     sess->s.con[cid] = &con->c;
> > +     clt_path->s.con[cid] = &con->c;
> >
> >       return 0;
> >   }
> >
>
> [ ... ]
>
> > -static inline bool xchg_sessions(struct rtrs_clt_sess __rcu **rcu_ppcpu_path,
> > -                              struct rtrs_clt_sess *sess,
> > -                              struct rtrs_clt_sess *next)
> > +static inline bool xchg_sessions(struct rtrs_clt_path __rcu **rcu_ppcpu_path,
> > +                              struct rtrs_clt_path *clt_path,
> > +                              struct rtrs_clt_path *next)
>
> Now, it is used to exchange paths instead of sessions, right?
>
> >   {
> > -     struct rtrs_clt_sess **ppcpu_path;
> > +     struct rtrs_clt_path **ppcpu_path;
> >
> >       /* Call cmpxchg() without sparse warnings */
> >       ppcpu_path = (typeof(ppcpu_path))rcu_ppcpu_path;
> > -     return sess == cmpxchg(ppcpu_path, sess, next);
> > +     return clt_path == cmpxchg(ppcpu_path, clt_path, next);
> >   }
>
> [ ... ]
>
> > @@ -2375,31 +2387,32 @@ static int init_conns(struct rtrs_clt_sess *sess)
> >   static void rtrs_clt_info_req_done(struct ib_cq *cq, struct ib_wc *wc)
> >   {
> >       struct rtrs_clt_con *con = to_clt_con(wc->qp->qp_context);
> > -     struct rtrs_clt_sess *sess = to_clt_sess(con->c.path);
> > +     struct rtrs_clt_path *clt_path = to_clt_path(con->c.path);
> >       struct rtrs_iu *iu;
> >
> >       iu = container_of(wc->wr_cqe, struct rtrs_iu, cqe);
> > -     rtrs_iu_free(iu, sess->s.dev->ib_dev, 1);
> > +     rtrs_iu_free(iu, clt_path->s.dev->ib_dev, 1);
> >
> >       if (wc->status != IB_WC_SUCCESS) {
> > -             rtrs_err(sess->clt, "Sess info request send failed: %s\n",
> > +             rtrs_err(clt_path->clt, "*Sess*  info request send failed: %s\n",
>
> Path info, same as below.
>
> [ ... ]
>
> >       if (wc->status != IB_WC_SUCCESS) {
> > -             rtrs_err(sess->clt, "Sess info response recv failed: %s\n",
> > +             rtrs_err(clt_path->clt, "Path info response recv failed: %s\n",
> >                         ib_wc_status_msg(wc->status));
> >               goto out;
> >       }
> >       WARN_ON(wc->opcode != IB_WC_RECV);
> >
> >       if (wc->byte_len < sizeof(*msg)) {
> > -             rtrs_err(sess->clt, "Sess info response is malformed: size %d\n",
> > +             rtrs_err(clt_path->clt, "Path info response is malformed: size %d\n",
> >                         wc->byte_len);
> >               goto out;
> >       }
>
> [ ... ]
>
> > @@ -2533,33 +2548,34 @@ static int rtrs_send_sess_info(struct rtrs_clt_sess *sess)
> >       /* Prepare for getting info response */
> >       err = rtrs_iu_post_recv(&usr_con->c, rx_iu);
> >       if (err) {
> > -             rtrs_err(sess->clt, "rtrs_iu_post_recv(), err: %d\n", err);
> > +             rtrs_err(clt_path->clt, "rtrs_iu_post_recv(), err: %d\n", err);
> >               goto out;
> >       }
> >       rx_iu = NULL;
> >
> >       msg = tx_iu->buf;
> >       msg->type = cpu_to_le16(RTRS_MSG_INFO_REQ);
> > -     memcpy(msg->sessname, sess->s.sessname, sizeof(msg->sessname));
> > +     memcpy(msg->sessname, clt_path->s.sessname, sizeof(msg->sessname));
>
> Pls check if it is pathname.

Will do. Actually sessname is correctly named at a bunch of places
so didn't require renaming. But I'll double check again.

Thanks for all other comments. Will address them in the next version
of a patchset.

> [ ... ]
>
>
> >   /**
> > - * init_sess() - establishes all session connections and does handshake
> > - * @sess: client session.
> > + * init_pathi() - establishes all path connections and does handshake
>
> s/init_pathi/init_path, maybe it would be better to call it
> init_clt_path here.
>
> > + * @clt_path: client path.
> >    * In case of error full close or reconnect procedure should be taken,
> >    * because reconnect or close async works can be started.
> >    */
> > -static int init_sess(struct rtrs_clt_sess *sess)
> > +static int init_path(struct rtrs_clt_path *clt_path)
>
> [ ... ]
>
> > -     if (sess->reconnect_attempts >= clt->max_reconnect_attempts) {
> > +     if (clt_path->reconnect_attempts >= clt->max_reconnect_attempts) {
> >               /* Close a*session*  completely if max attempts is reached */
>
> I guess it is close a path now.
>
> > -             rtrs_clt_close_conns(sess, false);
> > +             rtrs_clt_close_conns(clt_path, false);
> >               return;
> >       }
>
> [ ... ]
>
> Thanks,
> Guoqing