On Mon, Dec 30, 2019 at 11:51 PM Bart Van Assche <bvanassche@xxxxxxx> wrote:
>
> On 2019-12-30 02:29, Jack Wang wrote:
> > + * InfiniBand Transport Layer
>
> InfiniBand or RDMA?
will fix.
>
> > +static inline const char *rtrs_clt_state_str(enum rtrs_clt_state state)
> > +{
> > + switch (state) {
> > + case RTRS_CLT_CONNECTING:
> > + return "RTRS_CLT_CONNECTING";
> > + case RTRS_CLT_CONNECTING_ERR:
> > + return "RTRS_CLT_CONNECTING_ERR";
> > + case RTRS_CLT_RECONNECTING:
> > + return "RTRS_CLT_RECONNECTING";
> > + case RTRS_CLT_CONNECTED:
> > + return "RTRS_CLT_CONNECTED";
> > + case RTRS_CLT_CLOSING:
> > + return "RTRS_CLT_CLOSING";
> > + case RTRS_CLT_CLOSED:
> > + return "RTRS_CLT_CLOSED";
> > + case RTRS_CLT_DEAD:
> > + return "RTRS_CLT_DEAD";
> > + default:
> > + return "UNKNOWN";
> > + }
> > +}
>
> This function is not in the hot path so it shouldn't be inline.
no longer in use, will remove.
>
> > +#define MIN_LOG_SG 2
> > +#define MAX_LOG_SG 5
> > +#define MAX_LIN_SG BIT(MIN_LOG_SG)
> > +#define SG_DISTR_SZ (MAX_LOG_SG - MIN_LOG_SG + MAX_LIN_SG + 2)
>
> I think these constants deserve a comment that explains what their
> meaning is.
will add comment.
>
> > +/**
> > + * rtrs_permit - permits the memory allocation for future RDMA operation
> > + */
> > +struct rtrs_permit {
> > + enum rtrs_clt_con_type con_type;
> > + unsigned int cpu_id;
> > + unsigned int mem_id;
> > + unsigned int mem_off;
> > +};
>
> The comment above this structure is confusing. Please make it more clear.
will extend.
>
> Thanks,
>
> Bart.
Thanks Bart
