Re: [PATCH v6 11/25] rtrs: server: statistics functions

Bart Van Assche
2020-01-02T22:02:27Z

On 12/30/19 2:29 AM, Jack Wang wrote:
> +int rtrs_srv_reset_rdma_stats(struct rtrs_srv_stats *stats, bool enable)
> +{
> + if (enable) {
> + struct rtrs_srv_stats_rdma_stats *r = &stats->rdma_stats;
> +
> + memset(r, 0, sizeof(*r));
> + return 0;
> + }
> +
> + return -EINVAL;
> +}

I think the traditional kernel coding style is "if (!enable) return ...".

> +ssize_t rtrs_srv_stats_rdma_to_str(struct rtrs_srv_stats *stats,
> + char *page, size_t len)
> +{
> + struct rtrs_srv_stats_rdma_stats *r = &stats->rdma_stats;
> + struct rtrs_srv_sess *sess;
> +
> + sess = container_of(stats, typeof(*sess), stats);
> +
> + return scnprintf(page, len, "%lld %lld %lld %lld %u\n",
> + (s64)atomic64_read(&r->dir[READ].cnt),
> + (s64)atomic64_read(&r->dir[READ].size_total),
> + (s64)atomic64_read(&r->dir[WRITE].cnt),
> + (s64)atomic64_read(&r->dir[WRITE].size_total),
> + atomic_read(&sess->ids_inflight));
> +}

Does this follow the sysfs one-value-per-file rule?

> +int rtrs_srv_stats_wc_completion_to_str(struct rtrs_srv_stats *stats,
> + char *buf, size_t len)
> +{
> + return snprintf(buf, len, "%lld %lld\n",
> + (s64)atomic64_read(&stats->wc_comp.total_wc_cnt),
> + (s64)atomic64_read(&stats->wc_comp.calls));
> +}

Same comment here.

Thanks,

Bart.