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>,
Jason Gunthorpe <jgg@mellanox.com>,
Doug Ledford <dledford@redhat.com>,
Danil Kipnis <danil.kipnis@cloud.ionos.com>,
rpenyaev@suse.de, Roman Pen <roman.penyaev@profitbricks.com>
Subject: Re: [PATCH v4 10/25] ibtrs: server: main functionality
Date: Fri, 27 Sep 2019 17:03:39 +0200 [thread overview]
Message-ID: <CAMGffEmuY+ebhJz1iff7Cnb=qdHuhBaSs=DAKP_iKTOb2Ao2PA@mail.gmail.com> (raw)
In-Reply-To: <ab36427b-a737-9544-fbe8-cd53c0780994@acm.org>
On Tue, Sep 24, 2019 at 1:49 AM Bart Van Assche <bvanassche@acm.org> wrote:
>
> On 6/20/19 8:03 AM, Jack Wang wrote:
> > +module_param_named(max_chunk_size, max_chunk_size, int, 0444);
> > +MODULE_PARM_DESC(max_chunk_size,
> > + "Max size for each IO request, when change the unit is in byte"
> > + " (default: " __stringify(DEFAULT_MAX_CHUNK_SIZE_KB) "KB)");
>
> Where can I find the definition of DEFAULT_MAX_CHUNK_SIZE_KB?
oh, it's a typo, should be DEFAULT_MAX_CHUNK_SIZE.
>
> > +static char cq_affinity_list[256] = "";
>
> No empty initializers for file-scope variables please.
Is it guaranteed by the compiler, the file-scope variables will be
empty initialized?
>
> > + pr_info("cq_affinity_list changed to %*pbl\n",
> > + cpumask_pr_args(&cq_affinity_mask));
>
> Should this pr_info() call perhaps be changed into pr_debug()?
Because the setting could lead to performance drop, pr_info seems more
appropriate.
>
> > +static bool __ibtrs_srv_change_state(struct ibtrs_srv_sess *sess,
> > + enum ibtrs_srv_state new_state)
> > +{
> > + enum ibtrs_srv_state old_state;
> > + bool changed = false;
> > +
> > + old_state = sess->state;
> > + switch (new_state) {
>
> Please add a lockdep_assert_held() statement that checks whether calls
> of this function are serialized properly.
will look into it.
>
> > +/**
> > + * rdma_write_sg() - response on successful READ request
> > + */
> > +static int rdma_write_sg(struct ibtrs_srv_op *id)
> > +{
> > + struct ibtrs_srv_sess *sess = to_srv_sess(id->con->c.sess);
> > + dma_addr_t dma_addr = sess->dma_addr[id->msg_id];
> > + struct ibtrs_srv *srv = sess->srv;
> > + struct ib_send_wr inv_wr, imm_wr;
> > + struct ib_rdma_wr *wr = NULL;
> > + const struct ib_send_wr *bad_wr;
> > + enum ib_send_flags flags;
> > + size_t sg_cnt;
> > + int err, i, offset;
> > + bool need_inval;
> > + u32 rkey = 0;
> > +
> > + sg_cnt = le16_to_cpu(id->rd_msg->sg_cnt);
> > + need_inval = le16_to_cpu(id->rd_msg->flags) & IBTRS_MSG_NEED_INVAL_F;
> > + if (unlikely(!sg_cnt))
> > + return -EINVAL;
> > +
> > + offset = 0;
> > + for (i = 0; i < sg_cnt; i++) {
> > + struct ib_sge *list;
> > +
> > + wr = &id->tx_wr[i];
> > + list = &id->tx_sg[i];
> > + list->addr = dma_addr + offset;
> > + list->length = le32_to_cpu(id->rd_msg->desc[i].len);
> > +
> > + /* WR will fail with length error
> > + * if this is 0
> > + */
> > + if (unlikely(list->length == 0)) {
> > + ibtrs_err(sess, "Invalid RDMA-Write sg list length 0\n");
> > + return -EINVAL;
> > + }
> > +
> > + list->lkey = sess->s.dev->ib_pd->local_dma_lkey;
> > + offset += list->length;
> > +
> > + wr->wr.wr_cqe = &io_comp_cqe;
> > + wr->wr.sg_list = list;
> > + wr->wr.num_sge = 1;
> > + wr->remote_addr = le64_to_cpu(id->rd_msg->desc[i].addr);
> > + wr->rkey = le32_to_cpu(id->rd_msg->desc[i].key);
> > + if (rkey == 0)
> > + rkey = wr->rkey;
> > + else
> > + /* Only one key is actually used */
> > + WARN_ON_ONCE(rkey != wr->rkey);
> > +
> > + if (i < (sg_cnt - 1))
> > + wr->wr.next = &id->tx_wr[i + 1].wr;
> > + else if (need_inval)
> > + wr->wr.next = &inv_wr;
> > + else
> > + wr->wr.next = &imm_wr;
> > +
> > + wr->wr.opcode = IB_WR_RDMA_WRITE;
> > + wr->wr.ex.imm_data = 0;
> > + wr->wr.send_flags = 0;
> > + }
> > + /*
> > + * From time to time we have to post signalled sends,
> > + * or send queue will fill up and only QP reset can help.
> > + */
> > + flags = atomic_inc_return(&id->con->wr_cnt) % srv->queue_depth ?
> > + 0 : IB_SEND_SIGNALED;
> > +
> > + if (need_inval) {
> > + inv_wr.next = &imm_wr;
> > + inv_wr.wr_cqe = &io_comp_cqe;
> > + inv_wr.sg_list = NULL;
> > + inv_wr.num_sge = 0;
> > + inv_wr.opcode = IB_WR_SEND_WITH_INV;
> > + inv_wr.send_flags = 0;
> > + inv_wr.ex.invalidate_rkey = rkey;
> > + }
> > + imm_wr.next = NULL;
> > + imm_wr.wr_cqe = &io_comp_cqe;
> > + imm_wr.sg_list = NULL;
> > + imm_wr.num_sge = 0;
> > + imm_wr.opcode = IB_WR_RDMA_WRITE_WITH_IMM;
> > + imm_wr.send_flags = flags;
> > + imm_wr.ex.imm_data = cpu_to_be32(ibtrs_to_io_rsp_imm(id->msg_id,
> > + 0, need_inval));
> > +
> > + ib_dma_sync_single_for_device(sess->s.dev->ib_dev, dma_addr,
> > + offset, DMA_BIDIRECTIONAL);
> > +
> > + err = ib_post_send(id->con->c.qp, &id->tx_wr[0].wr, &bad_wr);
> > + if (unlikely(err))
> > + ibtrs_err(sess,
> > + "Posting RDMA-Write-Request to QP failed, err: %d\n",
> > + err);
> > +
> > + return err;
> > +}
>
> All other RDMA server implementations use rdma_rw_ctx_init() and
> rdma_rw_ctx_wrs(). Please use these functions in IBTRS too.
rdma_rw_ctx_* api doesn't support RDMA_WRITE_WITH_IMM, and
ibtrs mainly use RDMA_WRITE_WITH_IMM.
>
> > +static void ibtrs_srv_hb_err_handler(struct ibtrs_con *c, int err)
> > +{
> > + (void)err;
> > + close_sess(to_srv_sess(c->sess));
> > +}
>
> Is the (void)err statement really necessary?
No, will be removed.
>
> > +static int ibtrs_srv_rdma_init(struct ibtrs_srv_ctx *ctx, unsigned int port)
> > +{
> > + struct sockaddr_in6 sin = {
> > + .sin6_family = AF_INET6,
> > + .sin6_addr = IN6ADDR_ANY_INIT,
> > + .sin6_port = htons(port),
> > + };
> > + struct sockaddr_ib sib = {
> > + .sib_family = AF_IB,
> > + .sib_addr.sib_subnet_prefix = 0ULL,
> > + .sib_addr.sib_interface_id = 0ULL,
> > + .sib_sid = cpu_to_be64(RDMA_IB_IP_PS_IB | port),
> > + .sib_sid_mask = cpu_to_be64(0xffffffffffffffffULL),
> > + .sib_pkey = cpu_to_be16(0xffff),
> > + };
> > + struct rdma_cm_id *cm_ip, *cm_ib;
> > + int ret;
> > +
> > + /*
> > + * We accept both IPoIB and IB connections, so we need to keep
> > + * two cm id's, one for each socket type and port space.
> > + * If the cm initialization of one of the id's fails, we abort
> > + * everything.
> > + */
> > + cm_ip = ibtrs_srv_cm_init(ctx, (struct sockaddr *)&sin, RDMA_PS_TCP);
> > + if (unlikely(IS_ERR(cm_ip)))
> > + return PTR_ERR(cm_ip);
> > +
> > + cm_ib = ibtrs_srv_cm_init(ctx, (struct sockaddr *)&sib, RDMA_PS_IB);
> > + if (unlikely(IS_ERR(cm_ib))) {
> > + ret = PTR_ERR(cm_ib);
> > + goto free_cm_ip;
> > + }
> > +
> > + ctx->cm_id_ip = cm_ip;
> > + ctx->cm_id_ib = cm_ib;
> > +
> > + return 0;
> > +
> > +free_cm_ip:
> > + rdma_destroy_id(cm_ip);
> > +
> > + return ret;
> > +}
>
> Will the above work if CONFIG_IPV6=n?
I tested with CONFIG_IPV6=n, it compiles.
>
> > +static int __init ibtrs_server_init(void)
> > +{
> > + int err;
> > +
> > + if (!strlen(cq_affinity_list))
> > + init_cq_affinity();
>
> Is the above if-test useful? Can that if-test be left out?
You're right, will remove.
>
> Thanks,
>
> Bart.
Thanks!
next prev parent reply other threads:[~2019-09-27 15:03 UTC|newest]
Thread overview: 148+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-20 15:03 [PATCH v4 00/25] InfiniBand Transport (IBTRS) and Network Block Device (IBNBD) Jack Wang
2019-06-20 15:03 ` [PATCH v4 01/25] sysfs: export sysfs_remove_file_self() Jack Wang
2019-09-23 17:21 ` Bart Van Assche
2019-09-25 9:30 ` Danil Kipnis
2019-06-20 15:03 ` [PATCH v4 02/25] ibtrs: public interface header to establish RDMA connections Jack Wang
2019-09-23 17:44 ` Bart Van Assche
2019-09-25 10:20 ` Danil Kipnis
2019-09-25 15:38 ` Bart Van Assche
2019-06-20 15:03 ` [PATCH v4 03/25] ibtrs: private headers with IBTRS protocol structs and helpers Jack Wang
2019-09-23 22:50 ` Bart Van Assche
2019-09-25 21:45 ` Danil Kipnis
2019-09-25 21:57 ` Bart Van Assche
2019-09-27 8:56 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 04/25] ibtrs: core: lib functions shared between client and server modules Jack Wang
2019-09-23 23:03 ` Bart Van Assche
2019-09-27 10:13 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 05/25] ibtrs: client: private header with client structs and functions Jack Wang
2019-09-23 23:05 ` Bart Van Assche
2019-09-27 10:18 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 06/25] ibtrs: client: main functionality Jack Wang
2019-09-23 21:51 ` Bart Van Assche
2019-09-25 17:36 ` Danil Kipnis
2019-09-25 18:55 ` Bart Van Assche
2019-09-25 20:50 ` Danil Kipnis
2019-09-25 21:08 ` Bart Van Assche
2019-09-25 21:16 ` Bart Van Assche
2019-09-25 22:53 ` Danil Kipnis
2019-09-25 23:21 ` Bart Van Assche
2019-09-26 9:16 ` Danil Kipnis
2019-06-20 15:03 ` [PATCH v4 07/25] ibtrs: client: statistics functions Jack Wang
2019-09-23 23:15 ` Bart Van Assche
2019-09-27 12:00 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 08/25] ibtrs: client: sysfs interface functions Jack Wang
2019-06-20 15:03 ` [PATCH v4 09/25] ibtrs: server: private header with server structs and functions Jack Wang
2019-09-23 23:21 ` Bart Van Assche
2019-09-27 12:04 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 10/25] ibtrs: server: main functionality Jack Wang
2019-09-23 23:49 ` Bart Van Assche
2019-09-27 15:03 ` Jinpu Wang [this message]
2019-09-27 15:11 ` Bart Van Assche
2019-09-27 15:19 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 11/25] ibtrs: server: statistics functions Jack Wang
2019-09-23 23:56 ` Bart Van Assche
2019-10-02 15:15 ` Jinpu Wang
2019-10-02 15:42 ` Leon Romanovsky
2019-10-02 15:45 ` Jinpu Wang
2019-10-02 16:00 ` Leon Romanovsky
2019-06-20 15:03 ` [PATCH v4 12/25] ibtrs: server: sysfs interface functions Jack Wang
2019-09-24 0:00 ` Bart Van Assche
2019-10-02 15:11 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 13/25] ibtrs: include client and server modules into kernel compilation Jack Wang
2019-06-20 15:03 ` [PATCH v4 14/25] ibtrs: a bit of documentation Jack Wang
2019-06-20 15:03 ` [PATCH v4 15/25] ibnbd: private headers with IBNBD protocol structs and helpers Jack Wang
2019-09-13 22:10 ` Bart Van Assche
2019-09-15 14:30 ` Jinpu Wang
2019-09-16 5:27 ` Leon Romanovsky
2019-09-16 13:45 ` Bart Van Assche
2019-09-17 15:41 ` Leon Romanovsky
2019-09-17 15:52 ` Jinpu Wang
2019-09-16 7:08 ` Danil Kipnis
2019-09-16 14:57 ` Jinpu Wang
2019-09-16 17:25 ` Bart Van Assche
2019-09-17 12:27 ` Jinpu Wang
2019-09-16 15:39 ` Jinpu Wang
2019-09-18 15:26 ` Bart Van Assche
2019-09-18 16:11 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 16/25] ibnbd: client: private header with client structs and functions Jack Wang
2019-09-13 22:25 ` Bart Van Assche
2019-09-17 16:36 ` Jinpu Wang
2019-09-25 23:43 ` Danil Kipnis
2019-09-26 10:00 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 17/25] ibnbd: client: main functionality Jack Wang
2019-09-13 23:46 ` Bart Van Assche
2019-09-16 14:17 ` Danil Kipnis
2019-09-16 16:46 ` Bart Van Assche
2019-09-17 11:39 ` Danil Kipnis
2019-09-18 7:14 ` Danil Kipnis
2019-09-18 15:47 ` Bart Van Assche
2019-09-20 8:29 ` Danil Kipnis
2019-09-25 22:26 ` Danil Kipnis
2019-09-26 9:55 ` Roman Penyaev
2019-09-26 15:01 ` Bart Van Assche
2019-09-27 8:52 ` Roman Penyaev
2019-09-27 9:32 ` Danil Kipnis
2019-09-27 12:18 ` Danil Kipnis
2019-09-27 16:37 ` Bart Van Assche
2019-09-27 16:50 ` Roman Penyaev
2019-09-27 17:16 ` Bart Van Assche
2019-09-17 13:09 ` Jinpu Wang
2019-09-17 16:46 ` Bart Van Assche
2019-09-18 12:02 ` Jinpu Wang
2019-09-18 16:05 ` Jinpu Wang
2019-09-14 0:00 ` Bart Van Assche
2019-06-20 15:03 ` [PATCH v4 18/25] ibnbd: client: sysfs interface functions Jack Wang
2019-09-18 16:28 ` Bart Van Assche
2019-09-19 15:55 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 19/25] ibnbd: server: private header with server structs and functions Jack Wang
2019-06-20 15:03 ` [PATCH v4 20/25] ibnbd: server: main functionality Jack Wang
2019-09-18 17:41 ` Bart Van Assche
2019-09-20 7:36 ` Danil Kipnis
2019-09-20 15:42 ` Bart Van Assche
2019-09-23 15:19 ` Danil Kipnis
2019-06-20 15:03 ` [PATCH v4 21/25] ibnbd: server: functionality for IO submission to file or block dev Jack Wang
2019-09-18 21:46 ` Bart Van Assche
2019-09-26 14:04 ` Jinpu Wang
2019-09-26 15:11 ` Bart Van Assche
2019-09-26 15:25 ` Danil Kipnis
2019-09-26 15:29 ` Bart Van Assche
2019-09-26 15:38 ` Danil Kipnis
2019-09-26 15:42 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 22/25] ibnbd: server: sysfs interface functions Jack Wang
2019-06-20 15:03 ` [PATCH v4 23/25] ibnbd: include client and server modules into kernel compilation Jack Wang
2019-06-20 15:03 ` [PATCH v4 24/25] ibnbd: a bit of documentation Jack Wang
2019-09-13 23:58 ` Bart Van Assche
2019-09-18 12:22 ` Jinpu Wang
2019-06-20 15:03 ` [PATCH v4 25/25] MAINTAINERS: Add maintainer for IBNBD/IBTRS modules Jack Wang
2019-07-09 15:10 ` Leon Romanovsky
2019-07-09 15:18 ` Jinpu Wang
2019-07-09 15:51 ` Leon Romanovsky
2019-09-13 23:56 ` Bart Van Assche
2019-09-19 10:30 ` Jinpu Wang
2019-07-09 9:55 ` [PATCH v4 00/25] InfiniBand Transport (IBTRS) and Network Block Device (IBNBD) Danil Kipnis
2019-07-09 11:00 ` Leon Romanovsky
2019-07-09 11:17 ` Greg KH
2019-07-09 11:57 ` Jinpu Wang
2019-07-09 13:32 ` Leon Romanovsky
2019-07-09 15:39 ` Bart Van Assche
2019-07-09 11:37 ` Jinpu Wang
2019-07-09 12:06 ` Jason Gunthorpe
2019-07-09 13:15 ` Jinpu Wang
2019-07-09 13:19 ` Jason Gunthorpe
2019-07-09 14:17 ` Jinpu Wang
2019-07-09 21:27 ` Sagi Grimberg
2019-07-19 13:12 ` Danil Kipnis
2019-07-10 14:55 ` Danil Kipnis
2019-07-09 12:04 ` Jason Gunthorpe
2019-07-09 19:45 ` Sagi Grimberg
2019-07-10 13:55 ` Jason Gunthorpe
2019-07-10 16:25 ` Sagi Grimberg
2019-07-10 17:25 ` Jason Gunthorpe
2019-07-10 19:11 ` Sagi Grimberg
2019-07-11 7:27 ` Danil Kipnis
2019-07-11 8:54 ` Danil Kipnis
2019-07-12 0:22 ` Sagi Grimberg
2019-07-12 7:57 ` Jinpu Wang
2019-07-12 19:40 ` Sagi Grimberg
2019-07-15 11:21 ` Jinpu Wang
2019-07-12 10:58 ` Danil Kipnis
Reply instructions:
You may reply publicly 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='CAMGffEmuY+ebhJz1iff7Cnb=qdHuhBaSs=DAKP_iKTOb2Ao2PA@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=jgg@mellanox.com \
--cc=jinpuwang@gmail.com \
--cc=linux-block@vger.kernel.org \
--cc=linux-rdma@vger.kernel.org \
--cc=roman.penyaev@profitbricks.com \
--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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).