Re: [PATCH v6 05/25] rtrs: client: private header with client structs and functions

[Jinpu Wang <jinpu.wang@cloud.ionos.com>]

On Mon, Dec 30, 2019 at 11:51 PM Bart Van Assche <bvanassche@acm.org> 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