On Mon, Nov 4, 2013 at 2:13 PM, Hitoshi Mitake <mitake.hitoshi@xxxxxxxxx> wrote:
> From: Hitoshi Mitake <mitake.hitoshi@xxxxxxxxxxxxx>
>
> Current tgtd sends and receives iSCSI PDUs in its main event
> loop. This design can cause bottleneck when many iSCSI clients connect
> to single tgtd process. For example, we need multiple tgtd processes
> for utilizing fast network like 10 GbE because typical single
> processor core isn't fast enough for processing bunch of requests.
>
> This patch lets tgtd send/receive iSCSI PDUs and check digests in its
> worker threads. After applying this patch, the bottleneck in the main
> event loop is removed and the performance is improved.
>
> The improvement can be seen even if tgtd and iSCSI initiator are
> running on a single host. Below is a snippet of fio result on my
> laptop. The workload is 128MB random RW. Backingstore is sheepdog.
>
> original tgtd:
> read : io=65392KB, bw=4445.2KB/s, iops=1111, runt= 14711msec
> write: io=65680KB, bw=4464.8KB/s, iops=1116, runt= 14711msec
>
> tgtd with this patch:
> read : io=65392KB, bw=5098.9KB/s, iops=1274, runt= 12825msec
> write: io=65680KB, bw=5121.3KB/s, iops=1280, runt= 12825msec
>
> This change will be more effective when a number of iSCSI clients
> increases. I'd like to hear your comments on this change.
>
> Signed-off-by: Hitoshi Mitake <mitake.hitoshi@xxxxxxxxxxxxx>
> ---
> usr/iscsi/iscsi_tcp.c | 291 +++++++++++++++++++++++++++++++++++++++++++++++---
> usr/iscsi/iscsid.c | 61 +++++++----
> usr/iscsi/iscsid.h | 4 +
> 3 files changed, 322 insertions(+), 34 deletions(-)
ping?
>
> diff --git a/usr/iscsi/iscsi_tcp.c b/usr/iscsi/iscsi_tcp.c
> index 4a9532a..062f299 100644
> --- a/usr/iscsi/iscsi_tcp.c
> +++ b/usr/iscsi/iscsi_tcp.c
> @@ -31,6 +31,7 @@
> #include <netinet/tcp.h>
> #include <sys/epoll.h>
> #include <sys/socket.h>
> +#include <pthread.h>
>
> #include "iscsid.h"
> #include "tgtd.h"
> @@ -48,6 +49,20 @@ static long nop_ttt;
> static int listen_fds[8];
> static struct iscsi_transport iscsi_tcp;
>
> +enum iscsi_tcp_work_state {
> + ISCSI_TCP_WORK_INIT,
> + ISCSI_TCP_WORK_RX,
> + ISCSI_TCP_WORK_TX,
> + ISCSI_TCP_WORK_TX_EAGAIN,
> +};
> +
> +struct iscsi_tcp_work {
> + /* list: connected to iscsi_tcp_work_list or iscsi_tcp_finished_list */
> + struct list_head list;
> +
> + enum iscsi_tcp_work_state state;
> +};
> +
> struct iscsi_tcp_connection {
> int fd;
>
> @@ -59,13 +74,198 @@ struct iscsi_tcp_connection {
> long ttt;
>
> struct iscsi_connection iscsi_conn;
> + struct iscsi_tcp_work work;
> +
> + int used_in_worker_thread;
> };
>
> +static LIST_HEAD(iscsi_tcp_work_list);
> +static pthread_mutex_t iscsi_tcp_work_mutex = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t iscsi_tcp_work_cond = PTHREAD_COND_INITIALIZER;
> +
> +static LIST_HEAD(iscsi_tcp_work_finished_list);
> +static pthread_mutex_t iscsi_tcp_work_finished_mutex =
> + PTHREAD_MUTEX_INITIALIZER;
> +
> +static int iscsi_tcp_work_done_fds[2];
> +
> +static pthread_mutex_t iscsi_tcp_worker_startup_mutex =
> + PTHREAD_MUTEX_INITIALIZER;
> +
> +static int iscsi_tcp_worker_stop;
> +
> +static pthread_t *iscsi_tcp_worker_threads;
> +
> +static void iscsi_tcp_work_done_handler(int fd, int events, void *data)
> +{
> + LIST_HEAD(list);
> + struct iscsi_tcp_work *work;
> + struct iscsi_connection *conn;
> + struct iscsi_tcp_connection *tcp_conn;
> + int ret;
> + char dummy = 0;
> +
> + ret = read(fd, &dummy, sizeof(dummy));
> + if (ret != sizeof(dummy)) {
> + eprintf("iscsi tcp work error: %m\n");
> + exit(1);
> + }
> +
> + pthread_mutex_lock(&iscsi_tcp_work_finished_mutex);
> + list_splice_init(&iscsi_tcp_work_finished_list, &list);
> + pthread_mutex_unlock(&iscsi_tcp_work_finished_mutex);
> +
> + while (!list_empty(&list)) {
> + work = list_first_entry(&list, struct iscsi_tcp_work, list);
> + list_del(&work->list);
> +
> + tcp_conn =
> + container_of(work, struct iscsi_tcp_connection, work);
> + conn = &tcp_conn->iscsi_conn;
> +
> + tcp_conn->used_in_worker_thread = 0;
> +
> + ret = tgt_event_add(tcp_conn->fd,
> + work->state == ISCSI_TCP_WORK_TX ?
> + EPOLLOUT : EPOLLIN,
> + iscsi_tcp_event_handler, conn);
> + if (ret) {
> + eprintf("failed to add event: %m\n");
> + exit(1);
> + }
> +
> + switch (work->state) {
> + case ISCSI_TCP_WORK_RX:
> + if (conn->state != STATE_CLOSE)
> + iscsi_tcp_work_rx_done_fn(conn);
> + break;
> + case ISCSI_TCP_WORK_TX:
> + iscsi_tcp_work_tx_done_fn(conn);
> + break;
> + case ISCSI_TCP_WORK_TX_EAGAIN:
> + /* do nothing */
> + break;
> + default:
> + eprintf("invalid state of iscsi work tcp: %d\n",
> + work->state);
> + exit(1);
> + }
> +
> + if (conn->state == STATE_CLOSE) {
> + dprintf("connection closed %p\n", conn);
> + conn_close(conn);
> + } else
> + work->state = ISCSI_TCP_WORK_INIT;
> + }
> +}
> +
> +static void *iscsi_tcp_worker_fn(void *arg)
> +{
> + sigset_t set;
> + struct iscsi_tcp_work *work;
> + struct iscsi_connection *conn;
> + struct iscsi_tcp_connection *tcp_conn;
> + int ret;
> + char dummy = 0;
> +
> + sigfillset(&set);
> + sigprocmask(SIG_BLOCK, &set, NULL);
> +
> + pthread_mutex_lock(&iscsi_tcp_worker_startup_mutex);
> + pthread_mutex_unlock(&iscsi_tcp_worker_startup_mutex);
> +
> + dprintf("starting iscsi tcp worker thread: %lu\n", pthread_self());
> +
> + while (!iscsi_tcp_worker_stop) {
> + pthread_mutex_lock(&iscsi_tcp_work_mutex);
> +retest:
> + if (list_empty(&iscsi_tcp_work_list)) {
> + pthread_cond_wait(&iscsi_tcp_work_cond,
> + &iscsi_tcp_work_mutex);
> +
> + if (iscsi_tcp_worker_stop) {
> + pthread_mutex_unlock(&iscsi_tcp_work_mutex);
> + pthread_exit(NULL);
> + }
> +
> + goto retest;
> + }
> +
> + work = list_first_entry(&iscsi_tcp_work_list,
> + struct iscsi_tcp_work, list);
> +
> + list_del(&work->list);
> + pthread_mutex_unlock(&iscsi_tcp_work_mutex);
> +
> + tcp_conn =
> + container_of(work, struct iscsi_tcp_connection, work);
> + conn = &tcp_conn->iscsi_conn;
> +
> + switch (work->state) {
> + case ISCSI_TCP_WORK_RX:
> + do {
> + iscsi_rx_handler(conn);
> + } while (!is_conn_rx_end(conn)
> + && conn->state != STATE_CLOSE);
> +
> + break;
> + case ISCSI_TCP_WORK_TX:
> + do {
> + ret = iscsi_tx_handler(conn);
> + if (ret == -EAGAIN) {
> + /* no data to send */
> + work->state = ISCSI_TCP_WORK_TX_EAGAIN;
> + break;
> + }
> + } while (!is_conn_tx_end(conn)
> + && conn->state != STATE_CLOSE);
> +
> + break;
> + default:
> + eprintf("invalid state of iscsi tcp work: %d\n",
> + work->state);
> + exit(1);
> + }
> +
> + pthread_mutex_lock(&iscsi_tcp_work_finished_mutex);
> + list_add_tail(&work->list, &iscsi_tcp_work_finished_list);
> + pthread_mutex_unlock(&iscsi_tcp_work_finished_mutex);
> +
> + ret = write(iscsi_tcp_work_done_fds[1], &dummy,
> + sizeof(dummy));
> + if (ret != sizeof(dummy)) {
> + eprintf("iscsi tcp work error: %m\n");
> + exit(1);
> + }
> + }
> +
> + pthread_exit(NULL);
> +}
> +
> static inline struct iscsi_tcp_connection *TCP_CONN(struct iscsi_connection *conn)
> {
> return container_of(conn, struct iscsi_tcp_connection, iscsi_conn);
> }
>
> +static void queue_iscsi_tcp_work(struct iscsi_connection *conn)
> +{
> + struct iscsi_tcp_connection *tcp_conn = TCP_CONN(conn);
> + struct iscsi_tcp_work *work = &tcp_conn->work;
> +
> + /*
> + * Delete from main event loop temporaly, because the fd is used by
> + * worker threads.
> + */
> + tcp_conn->used_in_worker_thread = 1;
> + tgt_event_del(tcp_conn->fd);
> +
> + pthread_mutex_lock(&iscsi_tcp_work_mutex);
> + list_add_tail(&work->list, &iscsi_tcp_work_list);
> + pthread_mutex_unlock(&iscsi_tcp_work_mutex);
> +
> + pthread_cond_signal(&iscsi_tcp_work_cond);
> +}
> +
> static struct tgt_work nop_work;
>
> /* all iscsi connections */
> @@ -102,6 +302,9 @@ static void iscsi_tcp_nop_work_handler(void *data)
> if (tcp_conn->nop_tick > 0)
> continue;
>
> + if (tcp_conn->used_in_worker_thread)
> + continue;
> +
> tcp_conn->nop_tick = tcp_conn->nop_interval;
>
> tcp_conn->nop_inflight_count++;
> @@ -241,6 +444,9 @@ static void accept_connection(int afd, int events, void *data)
> if (!tcp_conn)
> goto out;
>
> + INIT_LIST_HEAD(&tcp_conn->work.list);
> + tcp_conn->work.state = ISCSI_TCP_WORK_INIT;
> +
> conn = &tcp_conn->iscsi_conn;
>
> ret = conn_init(conn);
> @@ -273,20 +479,20 @@ out:
> static void iscsi_tcp_event_handler(int fd, int events, void *data)
> {
> struct iscsi_connection *conn = (struct iscsi_connection *) data;
> + struct iscsi_tcp_connection *tcp_conn = TCP_CONN(conn);
> + struct iscsi_tcp_work *work = &tcp_conn->work;
>
> - if (events & EPOLLIN)
> - iscsi_rx_handler(conn);
> -
> - if (conn->state == STATE_CLOSE)
> - dprintf("connection closed\n");
> + if (work->state != ISCSI_TCP_WORK_INIT) {
> + eprintf("invalid state of work: %d\n", work->state);
> + exit(1);
> + }
>
> - if (conn->state != STATE_CLOSE && events & EPOLLOUT)
> - iscsi_tx_handler(conn);
> + if (events & EPOLLIN)
> + work->state = ISCSI_TCP_WORK_RX;
> + else if (events & EPOLLOUT)
> + work->state = ISCSI_TCP_WORK_TX;
>
> - if (conn->state == STATE_CLOSE) {
> - dprintf("connection closed %p\n", conn);
> - conn_close(conn);
> - }
> + queue_iscsi_tcp_work(conn);
> }
>
> int iscsi_tcp_init_portal(char *addr, int port, int tpgt)
> @@ -423,6 +629,8 @@ int iscsi_delete_portal(char *addr, int port)
>
> static int iscsi_tcp_init(void)
> {
> + int ret = 0, i;
> +
> /* If we were passed any portals on the command line */
> if (portal_arguments)
> iscsi_param_parse_portals(portal_arguments, 1, 0);
> @@ -440,17 +648,76 @@ static int iscsi_tcp_init(void)
> nop_work.data = &nop_work;
> add_work(&nop_work, 1);
>
> - return 0;
> + ret = pipe(iscsi_tcp_work_done_fds);
> + if (ret < 0) {
> + eprintf("failed to create pipe for tcp work: %m\n");
> + return -1;
> + }
> +
> + ret = tgt_event_add(iscsi_tcp_work_done_fds[0], EPOLLIN,
> + iscsi_tcp_work_done_handler, NULL);
> + if (ret < 0) {
> + eprintf("failed to register iscsi_tcp_work_done_handler():"\
> + " %m\n");
> + ret = -1;
> + goto close_done_fds;
> + }
> +
> + iscsi_tcp_worker_threads = calloc(nr_iothreads, sizeof(pthread_t));
> + if (!iscsi_tcp_worker_threads) {
> + eprintf("failed to allocate memory for pthread identifier:"\
> + " %m\n");
> + ret = -1;
> +
> + goto close_done_fds;
> + }
> +
> + pthread_mutex_lock(&iscsi_tcp_worker_startup_mutex);
> + for (i = 0; i < nr_iothreads; i++) {
> + ret = pthread_create(&iscsi_tcp_worker_threads[i], NULL,
> + iscsi_tcp_worker_fn, NULL);
> + if (ret) {
> + eprintf("creating worker thread failed: %m\n");
> + ret = -1;
> +
> + goto terminate_workers;
> + }
> + }
> +
> + pthread_mutex_unlock(&iscsi_tcp_worker_startup_mutex);
> + goto out;
> +
> +terminate_workers:
> + iscsi_tcp_worker_stop = 1;
> + pthread_mutex_unlock(&iscsi_tcp_worker_startup_mutex);
> +
> + for (; i <= 0; i--)
> + pthread_join(iscsi_tcp_worker_threads[i], NULL);
> +
> +close_done_fds:
> + close(iscsi_tcp_work_done_fds[0]);
> + close(iscsi_tcp_work_done_fds[1]);
> +
> +out:
> + return ret;
> }
>
> static void iscsi_tcp_exit(void)
> {
> + int i;
> +
> struct iscsi_portal *portal, *ptmp;
>
> list_for_each_entry_safe(portal, ptmp, &iscsi_portals_list,
> iscsi_portal_siblings) {
> iscsi_delete_portal(portal->addr, portal->port);
> }
> +
> + iscsi_tcp_worker_stop = 1;
> + for (i = 0; i < nr_iothreads; i++) {
> + pthread_cond_signal(&iscsi_tcp_work_cond);
> + pthread_join(iscsi_tcp_worker_threads[i], NULL);
> + }
> }
>
> static int iscsi_tcp_conn_login_complete(struct iscsi_connection *conn)
> diff --git a/usr/iscsi/iscsid.c b/usr/iscsi/iscsid.c
> index 30bd13f..dfa49ac 100644
> --- a/usr/iscsi/iscsid.c
> +++ b/usr/iscsi/iscsid.c
> @@ -76,6 +76,16 @@ enum {
> IOSTATE_TX_END,
> };
>
> +int is_conn_rx_end(struct iscsi_connection *conn)
> +{
> + return conn->rx_iostate == IOSTATE_RX_END;
> +}
> +
> +int is_conn_tx_end(struct iscsi_connection *conn)
> +{
> + return conn->tx_iostate == IOSTATE_TX_END;
> +}
> +
> void conn_read_pdu(struct iscsi_connection *conn)
> {
> conn->rx_iostate = IOSTATE_RX_BHS;
> @@ -2014,7 +2024,6 @@ static int iscsi_task_tx_start(struct iscsi_connection *conn)
>
> nodata:
> dprintf("no more data\n");
> - conn->tp->ep_event_modify(conn, EPOLLIN);
> return -EAGAIN;
> }
>
> @@ -2051,7 +2060,6 @@ void iscsi_rx_handler(struct iscsi_connection *conn)
> int ret = 0, hdigest, ddigest;
> uint32_t crc;
>
> -
> if (conn->state == STATE_SCSI) {
> struct param *p = conn->session_param;
> hdigest = p[ISCSI_PARAM_HDRDGST_EN].val & DIGEST_CRC32C;
> @@ -2182,9 +2190,8 @@ again:
> exit(1);
> }
>
> - if (ret < 0 ||
> - conn->rx_iostate != IOSTATE_RX_END ||
> - conn->state == STATE_CLOSE)
> + if (ret < 0 || conn->rx_iostate != IOSTATE_RX_END
> + || conn->state == STATE_CLOSE)
> return;
>
> if (conn->rx_size) {
> @@ -2192,20 +2199,6 @@ again:
> conn->rx_size);
> exit(1);
> }
> -
> - if (conn->state == STATE_SCSI) {
> - ret = iscsi_task_rx_done(conn);
> - if (ret)
> - conn->state = STATE_CLOSE;
> - else
> - conn_read_pdu(conn);
> - } else {
> - conn_write_pdu(conn);
> - conn->tp->ep_event_modify(conn, EPOLLOUT);
> - ret = cmnd_execute(conn);
> - if (ret)
> - conn->state = STATE_CLOSE;
> - }
> }
>
> static int do_send(struct iscsi_connection *conn, int next_state)
> @@ -2373,6 +2366,33 @@ again:
> finish:
> cmnd_finish(conn);
>
> +out:
> + return ret;
> +}
> +
> +void iscsi_tcp_work_rx_done_fn(struct iscsi_connection *conn)
> +{
> + int ret;
> +
> + if (conn->state == STATE_SCSI) {
> + ret = iscsi_task_rx_done(conn);
> + if (ret)
> + conn->state = STATE_CLOSE;
> + else
> + conn_read_pdu(conn);
> + } else {
> + conn_write_pdu(conn);
> + conn->tp->ep_event_modify(conn, EPOLLOUT);
> + ret = cmnd_execute(conn);
> + if (ret)
> + conn->state = STATE_CLOSE;
> + }
> +}
> +
> +void iscsi_tcp_work_tx_done_fn(struct iscsi_connection *conn)
> +{
> + int ret;
> +
> switch (conn->state) {
> case STATE_KERNEL:
> ret = conn_take_fd(conn);
> @@ -2395,9 +2415,6 @@ finish:
> conn->tp->ep_event_modify(conn, EPOLLIN);
> break;
> }
> -
> -out:
> - return ret;
> }
>
> int iscsi_transportid(int tid, uint64_t itn_id, char *buf, int size)
> diff --git a/usr/iscsi/iscsid.h b/usr/iscsi/iscsid.h
> index c7f6801..e716154 100644
> --- a/usr/iscsi/iscsid.h
> +++ b/usr/iscsi/iscsid.h
> @@ -336,6 +336,10 @@ extern int iscsi_param_parse_portals(char *p, int do_add, int do_delete);
> extern void iscsi_update_conn_stats_rx(struct iscsi_connection *conn, int size, int opcode);
> extern void iscsi_update_conn_stats_tx(struct iscsi_connection *conn, int size, int opcode);
> extern void iscsi_rsp_set_residual(struct iscsi_cmd_rsp *rsp, struct scsi_cmd *scmd);
> +extern void iscsi_tcp_work_rx_done_fn(struct iscsi_connection *conn);
> +extern void iscsi_tcp_work_tx_done_fn(struct iscsi_connection *conn);
> +extern int is_conn_rx_end(struct iscsi_connection *conn);
> +extern int is_conn_tx_end(struct iscsi_connection *conn);
>
> /* iscsid.c iscsi_task */
> extern void iscsi_free_task(struct iscsi_task *task);
> --
> 1.8.1.2
>
--
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
