Linux-RDMA Archive on lore.kernel.org
 help / color / Atom feed
From: Jinpu Wang <jinpu.wang@cloud.ionos.com>
To: Bart Van Assche <bvanassche@acm.org>
Cc: Jack Wang <jinpuwang@gmail.com>,
	linux-block@vger.kernel.org, linux-rdma@vger.kernel.org,
	Jens Axboe <axboe@kernel.dk>,
	Christoph Hellwig <hch@infradead.org>,
	Sagi Grimberg <sagi@grimberg.me>,
	Leon Romanovsky <leon@kernel.org>,
	Doug Ledford <dledford@redhat.com>,
	Danil Kipnis <danil.kipnis@cloud.ionos.com>,
	rpenyaev@suse.de
Subject: Re: [PATCH v6 03/25] rtrs: private headers with rtrs protocol structs and helpers
Date: Thu, 2 Jan 2020 16:27:37 +0100
Message-ID: <CAMGffEmC3T9M+RmeOXX4ecE3E01azjD8fz2Lz8kHC9Ff-Xx-Aw@mail.gmail.com> (raw)
In-Reply-To: <b13eccdd-09a2-70d5-1c78-3c4dbf1aefe8@acm.org>

On Mon, Dec 30, 2019 at 8:48 PM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 2019-12-30 02:29, Jack Wang wrote:
> > + * InfiniBand Transport Layer
>
> Is RTRS an InfiniBand or an RDMA transport layer?
The later,  will fix.
>
> > +#define rtrs_prefix(obj) (obj->sessname)
>
> Is it really worth it to introduce a macro for accessing a single member
> of a single pointer?
maybe not, will remove.
>
> > + * InfiniBand Transport Layer
>
> Same question here: is RTRS an InfiniBand or an RDMA transport layer?
will fix.

>
> > +enum {
> > +     SERVICE_CON_QUEUE_DEPTH = 512,
>
> What is a service connection?
s/SERVICE_CON_QUEUE_DEPTH/CON_QUEUE_DEPTH/g, do you think
CON_QUEUE_DEPTH is better or just QUEUE_DEPTH?
>
> > +     /*
> > +      * With the current size of the tag allocated on the client, 4K
> > +      * is the maximum number of tags we can allocate.  This number is
> > +      * also used on the client to allocate the IU for the user connection
> > +      * to receive the RDMA addresses from the server.
> > +      */
>
> What does the word 'tag' mean in the context of the RTRS protocol?
should be struct rtrs_permit, will fix.
>
> > +struct rtrs_ib_dev;
>
> What does the "rtrs_ib_dev" data structure represent? Additionally, I
> think it's confusing that a single name has an "r" that refers to "RDMA"
> and "ib" that refers to InfiniBand.
Naming is hard, it's structure mainly to cache struct ib_device
pointer and ib_pd pointer.
more info can be found below, Roman did try to push it to upstream,
see comment below.
>
> > +struct rtrs_ib_dev_pool {
> > +     struct mutex            mutex;
> > +     struct list_head        list;
> > +     enum ib_pd_flags        pd_flags;
> > +     const struct rtrs_ib_dev_pool_ops *ops;
> > +};
>
> What is the purpose of an rtrs_ib_dev_pool and what does it contain?
The idea was documented in the patchset here:
https://www.spinics.net/lists/linux-rdma/msg64025.html
"'
This is an attempt to make a device pool API out of a common code,
which caches pair of ib_device and ib_pd pointers. I found 4 places,
where this common functionality can be replaced by some lib calls:
nvme, nvmet, iser and isert. Total deduplication gain in loc is not
quite significant, but eventually new ULP IB code can also require
the same device/pd pair cache, e.g. in our IBTRS module the same
code has to be repeated again, which was observed by Sagi and he
suggested to make a common helper function instead of producing
another copy.
'''

>
> > +struct rtrs_iu {
>
> A comment that explains what the "iu" abbreviation stands for would be
> welcome.
ok.
>
> > +/**
> > + * enum rtrs_msg_types - RTRS message types.
> > + * @RTRS_MSG_INFO_REQ:               Client additional info request to the server
> > + * @RTRS_MSG_INFO_RSP:               Server additional info response to the client
> > + * @RTRS_MSG_WRITE:          Client writes data per RDMA to server
> > + * @RTRS_MSG_READ:           Client requests data transfer from server
> > + * @RTRS_MSG_RKEY_RSP:               Server refreshed rkey for rbuf
> > + */
>
> What is "additional info" in this context?
We have a bit more documentation in rtrs/README in patch 14,
"'
3. After all connections of a path are established client sends to server the
RTRS_MSG_INFO_REQ message, containing the name of the session. This message
requests the address information from the server.

4. Server replies to the session info request message with RTRS_MSG_INFO_RSP,
which contains the addresses and keys of the RDMA buffers allocated for that
session.
"'
>
> > +/**
> > + * struct rtrs_msg_conn_req - Client connection request to the server
> > + * @magic:      RTRS magic
> > + * @version:    RTRS protocol version
> > + * @cid:        Current connection id
> > + * @cid_num:    Number of connections per session
> > + * @recon_cnt:          Reconnections counter
> > + * @sess_uuid:          UUID of a session (path)
> > + * @paths_uuid:         UUID of a group of sessions (paths)
> > + *
> > + * NOTE: max size 56 bytes, see man rdma_connect().
> > + */
> > +struct rtrs_msg_conn_req {
> > +     u8              __cma_version; /* Is set to 0 by cma.c in case of
> > +                                     * AF_IB, do not touch that.
> > +                                     */
> > +     u8              __ip_version;  /* On sender side that should be
> > +                                     * set to 0, or cma_save_ip_info()
> > +                                     * extract garbage and will fail.
> > +                                     */
>
> The above two fields and the comments next to it look suspicious to me.
> Does RTRS perhaps try to generate CMA-formatted messages without using
> the CMA to format these messages?
The problem is in cma_format_hdr over-writes the first byte for AF_IB
https://www.spinics.net/lists/linux-rdma/msg22397.html

No one fixes the problem since then.

>
> > +     u8              reserved[12];
>
> Please leave out the reserved data. If future versions of the protocol
> would need any of these bytes it is easy to add more data to this structure.
Sorry, we can't do that, as I explained in the past, we have code
running in production and
there are checks expecting the size the of message are the same, it
will make the transition
to upstream version in the future a lot harder if we change the size
of the controll message.
>
> > +/**
> > + * struct rtrs_msg_conn_rsp - Server connection response to the client
> > + * @magic:      RTRS magic
> > + * @version:    RTRS protocol version
> > + * @errno:      If rdma_accept() then 0, if rdma_reject() indicates error
> > + * @queue_depth:   max inflight messages (queue-depth) in this session
> > + * @max_io_size:   max io size server supports
> > + * @max_hdr_size:  max msg header size server supports
> > + *
> > + * NOTE: size is 56 bytes, max possible is 136 bytes, see man rdma_accept().
> > + */
> > +struct rtrs_msg_conn_rsp {
> > +     __le16          magic;
> > +     __le16          version;
> > +     __le16          errno;
> > +     __le16          queue_depth;
> > +     __le32          max_io_size;
> > +     __le32          max_hdr_size;
> > +     __le32          flags;
> > +     u8              reserved[36];
> > +};
>
> Same comment here: please leave out the "reserved[]" array. Sending a
> bunch of zero-bytes at the end of a message over the wire is not useful.
same here.
>
> > +static inline void rtrs_from_imm(u32 imm, u32 *type, u32 *payload)
> > +{
> > +     *payload = (imm & MAX_IMM_PAYL_MASK);
> > +     *type = (imm >> MAX_IMM_PAYL_BITS);
> > +}
>
> Please do not use parentheses when not necessary. Such superfluous
> parentheses namely hurt readability of the code.
ok, will remove.
>
> > +     type = (w_inval ? RTRS_IO_RSP_W_INV_IMM : RTRS_IO_RSP_IMM);
>
> Same comment here: I think the parentheses can be left out from the
> above statement.
ok, will remove
>
> > +static inline void rtrs_from_io_rsp_imm(u32 payload, u32 *msg_id, int *errno)
> > +{
> > +     /* 9 bits for errno, 19 bits for msg_id */
> > +     *msg_id = (payload & 0x7ffff);
>
> Are the parentheses in the above expression necessary?
will remove.
>
> > +     *errno = -(int)((payload >> 19) & 0x1ff);
>
> Is the '(int)' cast useful in the above expression? Can it be left out?
I think it's necessary, and make it more clear errno is a negative int
value, isn't it?

>
> > +#define STAT_ATTR(type, stat, print, reset)                          \
> > +STAT_STORE_FUNC(type, stat, reset)                                   \
> > +STAT_SHOW_FUNC(type, stat, print)                                    \
> > +static struct kobj_attribute stat##_attr =                           \
> > +             __ATTR(stat, 0644,                                      \
> > +                    stat##_show,                                     \
> > +                    stat##_store)
>
> Is the above use of __ATTR() perhaps an open-coded version of __ATTR_RW()?
right, will use __ATTR_RW() instead.
>
> Thanks,
>
> Bart.
Thanks Bart!

  reply index

Thread overview: 89+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-12-30 10:29 [PATCH v6 00/25] RTRS (former IBTRS) rdma transport library and RNBD (former IBNBD) rdma network block device Jack Wang
2019-12-30 10:29 ` [PATCH v6 01/25] sysfs: export sysfs_remove_file_self() Jack Wang
2019-12-30 10:29 ` [PATCH v6 02/25] rtrs: public interface header to establish RDMA connections Jack Wang
2019-12-30 19:25   ` Bart Van Assche
2020-01-02 13:35     ` Jinpu Wang
2020-01-02 16:36       ` Bart Van Assche
2020-01-02 16:47         ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 03/25] rtrs: private headers with rtrs protocol structs and helpers Jack Wang
2019-12-30 19:48   ` Bart Van Assche
2020-01-02 15:27     ` Jinpu Wang [this message]
2020-01-02 17:00       ` Bart Van Assche
2020-01-02 18:26         ` Jason Gunthorpe
2020-01-03 12:31           ` Jinpu Wang
2020-01-03 12:27         ` Jinpu Wang
2019-12-31  0:07   ` Bart Van Assche
2020-01-03 13:48     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 04/25] rtrs: core: lib functions shared between client and server modules Jack Wang
2019-12-30 22:25   ` Bart Van Assche
2020-01-07 12:22     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 05/25] rtrs: client: private header with client structs and functions Jack Wang
2019-12-30 22:51   ` Bart Van Assche
2020-01-07 12:39     ` Jinpu Wang
2019-12-30 23:03   ` Bart Van Assche
2020-01-07 12:39     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 06/25] rtrs: client: main functionality Jack Wang
2019-12-30 23:53   ` Bart Van Assche
2020-01-02 18:23     ` Jason Gunthorpe
2020-01-03 14:30     ` Jinpu Wang
2020-01-03 16:12       ` Bart Van Assche
2019-12-30 10:29 ` [PATCH v6 07/25] rtrs: client: statistics functions Jack Wang
2020-01-02 21:07   ` Bart Van Assche
2020-01-03 14:39     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 08/25] rtrs: client: sysfs interface functions Jack Wang
2020-01-02 21:14   ` Bart Van Assche
2020-01-03 14:59     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 09/25] rtrs: server: private header with server structs and functions Jack Wang
2020-01-02 21:24   ` Bart Van Assche
2020-01-08 16:33     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 10/25] rtrs: server: main functionality Jack Wang
2020-01-02 22:03   ` Bart Van Assche
2020-01-07 13:19     ` Jinpu Wang
2020-01-07 18:25       ` Jason Gunthorpe
2020-01-10 17:38         ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 11/25] rtrs: server: statistics functions Jack Wang
2020-01-02 22:02   ` Bart Van Assche
2020-01-08 12:55     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 12/25] rtrs: server: sysfs interface functions Jack Wang
2020-01-02 22:06   ` Bart Van Assche
2020-01-07 14:40     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 13/25] rtrs: include client and server modules into kernel compilation Jack Wang
2020-01-02 22:11   ` Bart Van Assche
2020-01-03 16:19     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 14/25] rtrs: a bit of documentation Jack Wang
2019-12-30 23:19   ` Bart Van Assche
2020-01-07 14:48     ` Jinpu Wang
2020-01-02 22:21   ` Bart Van Assche
2020-01-07 15:49     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 15/25] rnbd: private headers with rnbd protocol structs and helpers Jack Wang
2020-01-02 22:34   ` Bart Van Assche
2020-01-07 16:53     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 16/25] rnbd: client: private header with client structs and functions Jack Wang
2020-01-02 22:37   ` Bart Van Assche
2020-01-07 17:09     ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 17/25] rnbd: client: main functionality Jack Wang
2020-01-02 23:55   ` Bart Van Assche
2020-01-08 14:22     ` Jinpu Wang
2020-01-10 14:45     ` Jinpu Wang
2020-01-10 15:09       ` Roman Penyaev
2020-01-10 15:29         ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 18/25] rnbd: client: sysfs interface functions Jack Wang
2020-01-03  0:03   ` Bart Van Assche
2020-01-08 13:06     ` Jinpu Wang
2020-01-08 16:39       ` Bart Van Assche
2020-01-08 16:51         ` Jinpu Wang
2019-12-30 10:29 ` [PATCH v6 19/25] rnbd: server: private header with server structs and functions Jack Wang
2019-12-30 10:29 ` [PATCH v6 20/25] rnbd: server: main functionality Jack Wang
2019-12-30 10:29 ` [PATCH v6 21/25] rnbd: server: functionality for IO submission to file or block dev Jack Wang
2019-12-30 10:29 ` [PATCH v6 22/25] rnbd: server: sysfs interface functions Jack Wang
2019-12-30 10:29 ` [PATCH v6 23/25] rnbd: include client and server modules into kernel compilation Jack Wang
2019-12-30 10:29 ` [PATCH v6 24/25] rnbd: a bit of documentation Jack Wang
2019-12-30 10:29 ` [PATCH v6 25/25] MAINTAINERS: Add maintainers for RNBD/RTRS modules Jack Wang
2019-12-30 12:22   ` Gal Pressman
2020-01-02  8:41     ` Jinpu Wang
2019-12-31  0:11 ` [PATCH v6 00/25] RTRS (former IBTRS) rdma transport library and RNBD (former IBNBD) rdma network block device Bart Van Assche
2020-01-02  8:48   ` Jinpu Wang
2019-12-31  2:39 ` Bart Van Assche
2020-01-02  9:20   ` Jinpu Wang
2020-01-02 18:28   ` Jason Gunthorpe
2020-01-03 12:34     ` Jinpu Wang

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAMGffEmC3T9M+RmeOXX4ecE3E01azjD8fz2Lz8kHC9Ff-Xx-Aw@mail.gmail.com \
    --to=jinpu.wang@cloud.ionos.com \
    --cc=axboe@kernel.dk \
    --cc=bvanassche@acm.org \
    --cc=danil.kipnis@cloud.ionos.com \
    --cc=dledford@redhat.com \
    --cc=hch@infradead.org \
    --cc=jinpuwang@gmail.com \
    --cc=leon@kernel.org \
    --cc=linux-block@vger.kernel.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=rpenyaev@suse.de \
    --cc=sagi@grimberg.me \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-RDMA Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-rdma/0 linux-rdma/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-rdma linux-rdma/ https://lore.kernel.org/linux-rdma \
		linux-rdma@vger.kernel.org
	public-inbox-index linux-rdma

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-rdma


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git