[stgt] [PATCH 1/1] nonblocking epoll_wait loop, sched events, ISER/IB polling

[Pete Wyckoff]

nezhinsky at gmail.com 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 at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html