On Tue, Dec 31, 2019 at 12:53 AM Bart Van Assche <bvanassche@xxxxxxx> wrote:
>
> On 2019-12-30 02:29, Jack Wang wrote:
> > + * InfiniBand Transport Layer
>
> InfiniBand or RDMA?
will fix.
>
> > +MODULE_DESCRIPTION("RTRS Client");
>
> Please spell out RTRS in full.
ok.
>
> > +static const struct rtrs_ib_dev_pool_ops dev_pool_ops;
>
> Can this forward declaration be avoided?
I don't see how to do it easily.
>
> > +static struct rtrs_ib_dev_pool dev_pool = {
> > + .ops = &dev_pool_ops
> > +};
>
> Can this structure be declared 'const'?
No, it's not const, we also initialize it in rtrs_ib_dev_pool_init
>
> > +static inline struct rtrs_permit *
> > +__rtrs_get_permit(struct rtrs_clt *clt, enum rtrs_clt_con_type con_type)
> > +{
> > + size_t max_depth = clt->queue_depth;
> > + struct rtrs_permit *permit;
> > + int cpu, bit;
> > +
> > + cpu = get_cpu();
> > + do {
> > + bit = find_first_zero_bit(clt->permits_map, max_depth);
> > + if (unlikely(bit >= max_depth)) {
> > + put_cpu();
> > + return NULL;
> > + }
> > +
> > + } while (unlikely(test_and_set_bit_lock(bit, clt->permits_map)));
> > + put_cpu();
>
> Are the get_cpu() and put_cpu() calls around this loop useful? If not,
> please remove these calls. Otherwise please add a comment that explains
> the purpose of these calls.
>
> An additional question: is it possible to replace the above loop with an
> sbitmap_get() call?
will check.
>
> > +static void complete_rdma_req(struct rtrs_clt_io_req *req, int errno,
> > + bool notify, bool can_wait)
> > +{
> > + struct rtrs_clt_con *con = req->con;
> > + struct rtrs_clt_sess *sess;
> > + int err;
> > +
> > + if (WARN_ON(!req->in_use))
> > + return;
> > + if (WARN_ON(!req->con))
> > + return;
> > + sess = to_clt_sess(con->c.sess);
> > +
> > + if (req->sg_cnt) {
> > + if (unlikely(req->dir == DMA_FROM_DEVICE && req->need_inv)) {
> > + /*
> > + * We are here to invalidate RDMA read requests
> > + * ourselves. In normal scenario server should
> > + * send INV for all requested RDMA reads, but
> > + * we are here, thus two things could happen:
> > + *
> > + * 1. this is failover, when errno != 0
> > + * and can_wait == 1,
> > + *
> > + * 2. something totally bad happened and
> > + * server forgot to send INV, so we
> > + * should do that ourselves.
> > + */
>
> Please document in the protocol documentation when RDMA reads are used.
We don't use RDMA READ, it's requested RDMA read meaning, server side will do
RDMA write to the buffers.
>
> What does "server forgot to send INV" mean?
Means server side malfunctional/server panic/etc, server didnot sent
SEND_WITH_INV WR,
so client have to do local invalidate.
>
> Additionally, if I remember correctly Jason considers it very important
> that invalidation happens from the submitting context because otherwise
> the RDMA retry mechanism can't work.
>
> > +static void process_io_rsp(struct rtrs_clt_sess *sess, u32 msg_id,
> > + s16 errno, bool w_inval)
> > +{
> > + struct rtrs_clt_io_req *req;
> > +
> > + if (WARN_ON(msg_id >= sess->queue_depth))
> > + return;
> > +
> > + req = &sess->reqs[msg_id];
> > + /* Drop need_inv if server responsed with invalidation */
> > + req->need_inv &= !w_inval;
> > + complete_rdma_req(req, errno, true, false);
> > +}
>
> Please document the meaning of the "w_inval" argument. Please also fix
> the spelling of "responsed".
>
> Thanks,
>
> Bart.
OK, thanks
