nezhinsky@xxxxxxxxx wrote on Thu, 18 Sep 2008 21:38 +0300:
> This patch introduces custom events scheduler, non-blocking
> epoll_wait when events are pending, delaying IB completions
> notifications, that leads to significant reduction in interrupts rate
> for iser/ib, while adding flexibility to tgtd event processing scheme.
I like the idea and am impressed by the results. Here are some
patch comments.
First, put all of the text in 0/1 into the changelog here. It
explains very well why we want this, and shows performance numbers
to prove the worth.
> diff --git a/usr/iscsi/iscsi_rdma.c b/usr/iscsi/iscsi_rdma.c
> index 46e6ea8..35b0f13 100644
> --- a/usr/iscsi/iscsi_rdma.c
> +++ b/usr/iscsi/iscsi_rdma.c
> @@ -144,6 +144,8 @@ struct conn_info {
> /* but count so we can drain CQ on close */
> int recvl_posted;
>
> + struct tgt_event tx_sched;
> +
Tomo will not like how you use spaces instead of tabs. You may want
to run scripts/checkpatch.pl, as borrowed from the linux developers,
and fixup everything it complains about. Aspects like commas
without space after them will also be caught by this script.
[..]
> +static void iser_poll_cq_normal(struct iser_device *dev)
> +{
> + int ret;
> +
> + ret = iser_poll_cq(dev,8);
> + if (ret < 0)
> + exit(1);
Please eprintf() then exit. Also why 8? Should be a #define
somewhere? Similar for the 4 down in _drain.
> + if (ret == 0) {
> + ret = ibv_req_notify_cq(dev->cq, 0);
> + if (ret) {
> + eprintf("ibv_req_notify_cq: %s\n", strerror(ret));
> + exit(1);
> + }
> + dev->poll_sched.sched_handler = iser_sched_drain_cq;
> + }
> + else
> + dev->poll_sched.sched_handler = iser_sched_poll_cq;
> +
> + tgt_add_sched_event(&dev->poll_sched);
> +}
> +
> +static void iser_poll_cq_drain(struct iser_device *dev)
> +{
> + int ret;
> +
> + ret = iser_poll_cq(dev,4);
> + if (ret < 0)
> + exit(1);
> +
> + dev->poll_sched.sched_handler = iser_sched_poll_cq;
> + if (ret == 0) {
> + ret = ibv_req_notify_cq(dev->cq, 0);
> + if (ret) {
> + eprintf("ibv_req_notify_cq: %s\n", strerror(ret));
> + exit(1);
> + }
> + }
> +}
> +
> +static void iser_sched_poll_cq(struct tgt_event *tev)
> +{
> + struct iser_device *dev = tev->data;
> + iser_poll_cq_normal(dev);
> +}
> +
> +static void iser_sched_drain_cq(struct tgt_event *tev)
> +{
> + struct iser_device *dev = tev->data;
> + iser_poll_cq_drain(dev);
> +}
It sure took me a long time to (maybe?) understand this logic.
You're trying to switch between active polling and blocking on the
IB CQ. First fixup the naming. normal == poll_cq, but drain ==
drain_cq. A problem with "drain" is that word is used when shutting
down a connection. How about "iser_check_cq_poll" and
"iser_check_cq_block" and do as appropriate with the other functions
starting with those names? Comments would help too.
The rest of the code I think looks fine, assuming all the style
issues are cleaned up. Are you willing to do that and send the mail
again?
-- Pete
--
To unsubscribe from this list: send the line "unsubscribe stgt" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
