From: "Wodkowski, PawelX" <pawelx.wodkowski@intel.com>
To: "Liu, Changpeng" <changpeng.liu@intel.com>,
"dev@dpdk.org" <dev@dpdk.org>
Cc: "Liu, Changpeng" <changpeng.liu@intel.com>,
"Harris, James R" <james.r.harris@intel.com>,
"Zedlewski, Piotr" <piotr.zedlewski@intel.com>
Subject: Re: [dpdk-dev] [PATCH v3] examples/vhost: introduce a new vhost-user-scsi sample application
Date: Wed, 12 Jul 2017 14:49:38 +0000 [thread overview]
Message-ID: <F6F2A6264E145F47A18AB6DF8E87425D52D8D326@IRSMSX102.ger.corp.intel.com> (raw)
In-Reply-To: <1499491297-17800-1-git-send-email-changpeng.liu@intel.com>
Hi, sorry for late review but just spotted this patch.
Please see my comments
> diff --git a/examples/vhost_scsi/scsi.c b/examples/vhost_scsi/scsi.c
> new file mode 100644
> index 0000000..08dfe61
> --- /dev/null
> +++ b/examples/vhost_scsi/scsi.c
> @@ -0,0 +1,507 @@
> +static int
> +vhost_hex2bin(char ch)
> +{
> + if ((ch >= '0') && (ch <= '9'))
> + return ch - '0';
> + ch = tolower(ch);
> + if ((ch >= 'a') && (ch <= 'f'))
> + return ch - 'a' + 10;
> + return (int)ch;
> +}
consider using strtol()
> +
> +int
> +vhost_bdev_process_scsi_commands(struct vhost_block_dev *bdev,
> + struct vhost_scsi_task *task)
> +{
> + int len;
> + uint8_t *data;
> + uint64_t fmt_lun = 0;
> + const uint8_t *lun;
> + uint8_t *cdb = (uint8_t *)task->req->cdb;
> +
> + lun = (const uint8_t *)task->req->lun;
> + /* only 1 LUN supported */
> + if (lun[0] != 1 || lun[1] >= 1)
> + return -1;
> +
> + switch (cdb[0]) {
> + case SPC_INQUIRY:
> + len = vhost_bdev_scsi_inquiry_command(bdev, task);
> + task->data_len = len;
> + break;
> + case SPC_REPORT_LUNS:
> + data = (uint8_t *)task->iovs[0].iov_base;
> + fmt_lun |= (0x0ULL & 0x00ffULL) << 48;
Please provide a description for this magical formula for non-SCSI proficient community.
> + to_be64((void *)&data[8], fmt_lun);
> + to_be32((void *)data, 8);
> + task->data_len = 16;
> + scsi_task_set_status(task, SCSI_STATUS_GOOD, 0, 0, 0);
> + break;
> + case SPC_MODE_SELECT_6:
> + case SPC_MODE_SELECT_10:
> + /* TODO: Add SCSI Commands support */
> + scsi_task_set_status(task, SCSI_STATUS_GOOD, 0, 0, 0);
> + break;
> + case SPC_MODE_SENSE_6:
> + case SPC_MODE_SENSE_10:
> + /* TODO: Add SCSI Commands support */
> + scsi_task_set_status(task, SCSI_STATUS_GOOD, 0, 0, 0);
> + break;
If those cases are mandatory they should be implemented if not removing TODO would be better than informing that
there is something to do.
> + case SPC_TEST_UNIT_READY:
> + scsi_task_set_status(task, SCSI_STATUS_GOOD, 0, 0, 0);
> + break;
> + default:
> + len = vhost_bdev_scsi_process_block(bdev, task);
> + task->data_len = len;
> + }
> +
> + return 0;
> +}
> diff --git a/examples/vhost_scsi/scsi_spec.h b/examples/vhost_scsi/scsi_spec.h
> new file mode 100644
> index 0000000..0474d92
> --- /dev/null
> +++ b/examples/vhost_scsi/scsi_spec.h
> @@ -0,0 +1,488 @@
Most of the scsi_spec.h file is unused, please remove unnecessary parts.
> diff --git a/examples/vhost_scsi/vhost_scsi.c
> b/examples/vhost_scsi/vhost_scsi.c
> new file mode 100644
> index 0000000..160c2e0
> --- /dev/null
> +++ b/examples/vhost_scsi/vhost_scsi.c
> @@ -0,0 +1,472 @@
> +
> +#define VIRTIO_SCSI_FEATURES ((1 << VIRTIO_F_NOTIFY_ON_EMPTY) |\
> + (1 << VIRTIO_RING_F_EVENT_IDX) |\
Am I missing something or this example app doesn't support EVENT_IDX?
> + (1 << VIRTIO_SCSI_F_INOUT) |\
> + (1 << VIRTIO_SCSI_F_CHANGE))
> +
> +/* Path to folder where character device will be created. Can be set by user.
> */
> +static char dev_pathname[PATH_MAX] = "";
> +
> +static struct vhost_scsi_ctrlr *g_vhost_ctrlr;
> +static int g_should_stop;
> +static sem_t exit_sem;
> +
> +#define NUM_OF_SCSI_QUEUES 3
> +
> +static struct vhost_scsi_ctrlr *
> +vhost_scsi_ctrlr_find(__rte_unused const char *ctrlr_name)
> +{
> + /* currently we only support 1 socket file fd */
So remove this function.
> + return g_vhost_ctrlr;
> +}
> +
> +static uint64_t gpa_to_vva(int vid, uint64_t gpa)
> +{
> + char path[PATH_MAX];
> + struct vhost_scsi_ctrlr *ctrlr;
> + int ret = 0;
> +
> + ret = rte_vhost_get_ifname(vid, path, PATH_MAX);
> + if (ret) {
> + fprintf(stderr, "Cannot get socket name\n");
> + assert(ret != 0);
> + }
> +
> + ctrlr = vhost_scsi_ctrlr_find(path);
> + if (!ctrlr) {
> + fprintf(stderr, "Controller is not ready\n");
> + assert(ctrlr != NULL);
> + }
> +
> + assert(ctrlr->mem != NULL);
> +
> + return rte_vhost_gpa_to_vva(ctrlr->mem, gpa);
> +}
> +
Replace vid in vhost_block_dev by pointer to struct vhost_scsi_ctrlr
This way you will not need this whole function at all.
> +static struct vring_desc *
> +descriptor_get_next(struct vring_desc *vq_desc, struct vring_desc
> *cur_desc)
> +{
> + return &vq_desc[cur_desc->next];
> +}
> +
> +static bool
> +descriptor_has_next(struct vring_desc *cur_desc)
> +{
> + return !!(cur_desc->flags & VRING_DESC_F_NEXT);
> +}
> +
> +static bool
> +descriptor_is_wr(struct vring_desc *cur_desc)
> +{
> + return !!(cur_desc->flags & VRING_DESC_F_WRITE);
> +}
I have an impression that all those functions should go to library instead of example application.
> +
> +static struct vhost_block_dev *
> +vhost_scsi_bdev_construct(const char *bdev_name, const char
> *bdev_serial,
> + uint32_t blk_size, uint64_t blk_cnt,
> + bool wce_enable)
> +{
> + struct vhost_block_dev *bdev;
> +
> + bdev = rte_zmalloc(NULL, sizeof(*bdev), RTE_CACHE_LINE_SIZE);
> + if (!bdev)
> + return NULL;
> +
> + strncpy(bdev->name, bdev_name, sizeof(bdev->name));
> + strncpy(bdev->product_name, bdev_serial, sizeof(bdev-
> >product_name));
> + bdev->blocklen = blk_size;
> + bdev->blockcnt = blk_cnt;
> + bdev->write_cache = wce_enable;
write_cache is not used and should be removed.
> +
> + /* use memory as disk storage space */
> + bdev->data = rte_zmalloc(NULL, blk_cnt * blk_size, 0);
> + if (!bdev->data) {
> + fprintf(stderr, "no enough reseverd huge memory for
> disk\n");
> + return NULL;
> + }
> +
> + return bdev;
> +}
> +
> +static void
> +process_requestq(struct vhost_scsi_ctrlr *ctrlr, uint32_t q_idx)
> +{
> + int ret;
> + struct vhost_scsi_queue *scsi_vq;
> + struct rte_vhost_vring *vq;
> +
> + scsi_vq = &ctrlr->bdev->queues[q_idx];
> + vq = &scsi_vq->vq;
> + ret = rte_vhost_get_vhost_vring(ctrlr->bdev->vid, q_idx, vq);
> + assert(ret == 0);
> +
> + while (vq->avail->idx != scsi_vq->last_used_idx) {
> + int req_idx;
> + uint16_t last_idx;
> + struct vhost_scsi_task *task;
> +
> + last_idx = scsi_vq->last_used_idx & (vq->size - 1);
> + req_idx = vq->avail->ring[last_idx];
> +
> + task = rte_zmalloc(NULL, sizeof(*task), 0);
> + assert(task != NULL);
> +
> + task->ctrlr = ctrlr;
> + task->bdev = ctrlr->bdev;
> + task->vq = vq;
> + task->req_idx = req_idx;
> + task->desc = &task->vq->desc[task->req_idx];
> +
> + /* does not support indirect descriptors */
> + assert((task->desc->flags & VRING_DESC_F_INDIRECT) == 0);
> + scsi_vq->last_used_idx++;
> +
> + task->req = (void *)gpa_to_vva(task->bdev->vid,
> + task->desc->addr);
> +
> + task->desc = descriptor_get_next(task->vq->desc, task-
> >desc);
> + if (!descriptor_has_next(task->desc)) {
> + task->dxfer_dir = SCSI_DIR_NONE;
> + task->resp = (void *)gpa_to_vva(task->bdev->vid,
> + task->desc->addr);
> +
> + } else if (!descriptor_is_wr(task->desc)) {
> + task->dxfer_dir = SCSI_DIR_TO_DEV;
> + vhost_process_write_payload_chain(task);
> + } else {
> + task->dxfer_dir = SCSI_DIR_FROM_DEV;
> + vhost_process_read_payload_chain(task);
> + }
> +
> + ret = vhost_bdev_process_scsi_commands(ctrlr->bdev,
> task);
> + if (ret) {
> + /* invalid response */
> + task->resp->response =
> VIRTIO_SCSI_S_BAD_TARGET;
> + } else {
> + /* successfully */
> + task->resp->response = VIRTIO_SCSI_S_OK;
> + task->resp->status = 0;
You need to fill resp->resid field here.
> + }
> + submit_completion(task);
> + rte_free(task);
> + }
> +}
> +
> +/* Main framework for processing IOs */
> +static void *
> +ctrlr_worker(void *arg)
> +{
> + uint32_t idx, num;
> + struct vhost_scsi_ctrlr *ctrlr = (struct vhost_scsi_ctrlr *)arg;
> + cpu_set_t cpuset;
> + pthread_t thread;
> +
> + thread = pthread_self();
> + CPU_ZERO(&cpuset);
> + CPU_SET(0, &cpuset);
> + pthread_setaffinity_np(thread, sizeof(cpu_set_t), &cpuset);
> +
> + num = rte_vhost_get_vring_num(ctrlr->bdev->vid);
> + fprintf(stdout, "Ctrlr Worker Thread Started with %u Vring\n", num);
> +
> + if (num != NUM_OF_SCSI_QUEUES) {
Again, how many queues doeas this app support 8 or NUM_OF_SCSI_QUEUES?
> + fprintf(stderr, "Only 1 IO queue are supported\n");
> + exit(0);
> + }
> +
> + while (!g_should_stop && ctrlr->bdev != NULL) {
> + /* At least 3 vrings, currently only can support 1 IO queue
> + * Queue 2 for IO queue, does not support TMF and hotplug
> + * for the example application now
> + */
> + for (idx = 2; idx < num; idx++)
> + process_requestq(ctrlr, idx);
> + }
> +
> + fprintf(stdout, "Ctrlr Worker Thread Exiting\n");
> + sem_post(&exit_sem);
> + return NULL;
> +}
> +
> +
> +static struct vhost_scsi_ctrlr *
> +vhost_scsi_ctrlr_construct(const char *ctrlr_name)
> +{
> + int ret;
> + struct vhost_scsi_ctrlr *ctrlr;
> + char *path;
> + char cwd[PATH_MAX];
> +
> + /* always use current directory */
> + path = getcwd(cwd, PATH_MAX);
> + if (!path) {
> + fprintf(stderr, "Cannot get current working directory\n");
> + return NULL;
> + }
> + snprintf(dev_pathname, sizeof(dev_pathname), "%s/%s", path,
> ctrlr_name);
> +
> + if (access(dev_pathname, F_OK) != -1) {
> + if (unlink(dev_pathname) != 0)
> + rte_exit(EXIT_FAILURE, "Cannot remove %s.\n",
> + dev_pathname);
> + }
> +
> + fprintf(stdout, "socket file: %s\n", dev_pathname);
> +
> + if (rte_vhost_driver_register(dev_pathname, 0) != 0) {
> + fprintf(stderr, "socket %s already exists\n", dev_pathname);
> + return NULL;
> + }
> +
> + ret = rte_vhost_driver_set_features(dev_pathname,
> VIRTIO_SCSI_FEATURES);
> + if (ret) {
> + fprintf(stderr, "Set vhost driver features failed\n");
> + return NULL;
> + }
> +
> + ctrlr = rte_zmalloc(NULL, sizeof(*ctrlr), RTE_CACHE_LINE_SIZE);
> + if (!ctrlr)
> + return NULL;
> +
> + ctrlr->name = strdup(ctrlr_name);
name is never used and also never free.
> +
> + rte_vhost_driver_callback_register(dev_pathname,
> + &vhost_scsi_device_ops);
> +
> + return ctrlr;
> +}
> +
> +static void
> +signal_handler(__rte_unused int signum)
> +{
Some message would be good to inform about successful exit.
> + if (access(dev_pathname, F_OK) == 0)
> + unlink(dev_pathname);
> + exit(0);
> +}
> +
> diff --git a/examples/vhost_scsi/vhost_scsi.h b/examples/vhost_scsi/vhost_scsi.h
> new file mode 100644
> index 0000000..b5340cc
> --- /dev/null
> +++ b/examples/vhost_scsi/vhost_scsi.h
> @@ -0,0 +1,250 @@
Do we need this file at all? I think it can got to .c file
> +
> +#include <rte_vhost.h>
> +
> +static inline uint16_t
> +from_be16(const void *ptr)
> +{
> + const uint8_t *tmp = (const uint8_t *)ptr;
> +
> + return (((uint16_t)tmp[0] << 8) | tmp[1]);
> +}
> +
Why implementing this instead of using existing macros from rte_byteorder.h?
> +
> +struct vaddr_region {
> + void *vaddr;
> + uint64_t len;
> +};
I don't see any usage of this struct.
> +
> +struct vhost_scsi_queue {
> + struct rte_vhost_vring vq;
> + uint16_t last_avail_idx;
> + uint16_t last_used_idx;
> +};
> +
> +struct vhost_block_dev {
> + /** ID for vhost library. */
> + int vid;
Don't think this is right place for vid. As mentioned, pointer to struct vhost_scsi_ctrlr would be better here.
> + /** Queues for the block device */
> + struct vhost_scsi_queue queues[8];
You define 8 queues here but in vhost_scsi.c NUM_OF_SCSI_QUEUES is 3.
Maybe move definition of NUM_OF_SCSI_QUEUES to .h file and use it here instead of '8'
> + /** Unique name for this block device. */
> + char name[64];
> +
> + /** Unique product name for this kind of block device. */
> + char product_name[256];
> +
> + /** Size in bytes of a logical block for the backend */
> + uint32_t blocklen;
> +
> + /** Number of blocks */
> + uint64_t blockcnt;
> +
> + /** write cache enabled, not used at the moment */
> + int write_cache;
So can be removed.
> +
> + /** use memory as disk storage space */
> + uint8_t *data;
> +};
> +
Thanks
Pawel
next prev parent reply other threads:[~2017-07-12 14:49 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-06-29 9:28 [dpdk-dev] [PATCH] " Changpeng Liu
2017-07-07 0:50 ` Yuanhan Liu
2017-07-07 2:00 ` Liu, Changpeng
2017-07-07 2:09 ` Yuanhan Liu
2017-07-08 4:14 ` [dpdk-dev] [PATCH v2] " Changpeng Liu
2017-07-07 4:48 ` Yuanhan Liu
2017-07-07 4:54 ` Liu, Changpeng
2017-07-08 5:21 ` [dpdk-dev] [PATCH v3] " Changpeng Liu
2017-07-07 5:07 ` Yuanhan Liu
2017-07-07 8:40 ` Hemant Agrawal
2017-07-07 9:44 ` Yuanhan Liu
2017-07-07 12:21 ` Thomas Monjalon
2017-07-08 2:53 ` Yuanhan Liu
2017-07-09 16:42 ` Thomas Monjalon
2017-07-12 14:49 ` Wodkowski, PawelX [this message]
2017-07-15 8:20 ` [dpdk-dev] [PATCH v4] " Changpeng Liu
2017-07-18 14:24 ` Thomas Monjalon
2017-07-19 0:20 ` Liu, Changpeng
2017-07-19 4:18 ` Thomas Monjalon
2017-07-19 4:24 ` Liu, Changpeng
2017-07-20 9:16 ` [dpdk-dev] [PATCH v5] " Changpeng Liu
2017-07-19 19:48 ` Yuanhan Liu
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=F6F2A6264E145F47A18AB6DF8E87425D52D8D326@IRSMSX102.ger.corp.intel.com \
--to=pawelx.wodkowski@intel.com \
--cc=changpeng.liu@intel.com \
--cc=dev@dpdk.org \
--cc=james.r.harris@intel.com \
--cc=piotr.zedlewski@intel.com \
/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).