On Tue, Jan 4, 2022 at 7:52 AM Guoqing Jiang <guoqing.jiang@xxxxxxxxx> 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
