Date: Thu, 7 Jan 2016 16:18:33 +0000 (UTC) From: Jim Harris <jimharris@FreeBSD.org> To: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: svn commit: r293328 - head/sys/dev/nvme Message-ID: <201601071618.u07GIXdd054147@repo.freebsd.org>
next in thread | raw e-mail | index | archive | help
Author: jimharris Date: Thu Jan 7 16:18:32 2016 New Revision: 293328 URL: https://svnweb.freebsd.org/changeset/base/293328 Log: nvme: do not revert o single I/O queue when per-CPU queues not possible Previously nvme(4) would revert to a signle I/O queue if it could not allocate enought interrupt vectors or NVMe submission/completion queues to have one I/O queue per core. This patch determines how to utilize a smaller number of available interrupt vectors, and assigns (as closely as possible) an equal number of cores to each associated I/O queue. MFC after: 3 days Sponsored by: Intel Modified: head/sys/dev/nvme/nvme.c head/sys/dev/nvme/nvme_ctrlr.c head/sys/dev/nvme/nvme_private.h Modified: head/sys/dev/nvme/nvme.c ============================================================================== --- head/sys/dev/nvme/nvme.c Thu Jan 7 16:12:42 2016 (r293327) +++ head/sys/dev/nvme/nvme.c Thu Jan 7 16:18:32 2016 (r293328) @@ -270,8 +270,6 @@ nvme_attach(device_t dev) return (status); } - nvme_sysctl_initialize_ctrlr(ctrlr); - pci_enable_busmaster(dev); ctrlr->config_hook.ich_func = nvme_ctrlr_start_config_hook; Modified: head/sys/dev/nvme/nvme_ctrlr.c ============================================================================== --- head/sys/dev/nvme/nvme_ctrlr.c Thu Jan 7 16:12:42 2016 (r293327) +++ head/sys/dev/nvme/nvme_ctrlr.c Thu Jan 7 16:18:32 2016 (r293328) @@ -42,6 +42,12 @@ __FBSDID("$FreeBSD$"); #include "nvme_private.h" +/* + * Used for calculating number of CPUs to assign to each core and number of I/O + * queues to allocate per controller. + */ +#define NVME_CEILING(num, div) ((((num) - 1) / (div)) + 1) + static void nvme_ctrlr_construct_and_submit_aer(struct nvme_controller *ctrlr, struct nvme_async_event_request *aer); static void nvme_ctrlr_setup_interrupts(struct nvme_controller *ctrlr); @@ -141,6 +147,13 @@ nvme_ctrlr_construct_io_qpairs(struct nv */ num_trackers = min(num_trackers, (num_entries-1)); + /* + * This was calculated previously when setting up interrupts, but + * a controller could theoretically support fewer I/O queues than + * MSI-X vectors. So calculate again here just to be safe. + */ + ctrlr->num_cpus_per_ioq = NVME_CEILING(mp_ncpus, ctrlr->num_io_queues); + ctrlr->ioq = malloc(ctrlr->num_io_queues * sizeof(struct nvme_qpair), M_NVME, M_ZERO | M_WAITOK); @@ -161,8 +174,13 @@ nvme_ctrlr_construct_io_qpairs(struct nv num_trackers, ctrlr); + /* + * Do not bother binding interrupts if we only have one I/O + * interrupt thread for this controller. + */ if (ctrlr->num_io_queues > 1) - bus_bind_intr(ctrlr->dev, qpair->res, i); + bus_bind_intr(ctrlr->dev, qpair->res, + i * ctrlr->num_cpus_per_ioq); } return (0); @@ -307,8 +325,15 @@ nvme_ctrlr_hw_reset(struct nvme_controll int i; nvme_admin_qpair_disable(&ctrlr->adminq); - for (i = 0; i < ctrlr->num_io_queues; i++) - nvme_io_qpair_disable(&ctrlr->ioq[i]); + /* + * I/O queues are not allocated before the initial HW + * reset, so do not try to disable them. Use is_initialized + * to determine if this is the initial HW reset. + */ + if (ctrlr->is_initialized) { + for (i = 0; i < ctrlr->num_io_queues; i++) + nvme_io_qpair_disable(&ctrlr->ioq[i]); + } DELAY(100*1000); @@ -364,7 +389,7 @@ static int nvme_ctrlr_set_num_qpairs(struct nvme_controller *ctrlr) { struct nvme_completion_poll_status status; - int cq_allocated, i, sq_allocated; + int cq_allocated, sq_allocated; status.done = FALSE; nvme_ctrlr_cmd_set_num_queues(ctrlr, ctrlr->num_io_queues, @@ -385,25 +410,12 @@ nvme_ctrlr_set_num_qpairs(struct nvme_co cq_allocated = (status.cpl.cdw0 >> 16) + 1; /* - * Check that the controller was able to allocate the number of - * queues we requested. If not, revert to one IO queue pair. + * Controller may allocate more queues than we requested, + * so use the minimum of the number requested and what was + * actually allocated. */ - if (sq_allocated < ctrlr->num_io_queues || - cq_allocated < ctrlr->num_io_queues) { - - /* - * Destroy extra IO queue pairs that were created at - * controller construction time but are no longer - * needed. This will only happen when a controller - * supports fewer queues than MSI-X vectors. This - * is not the normal case, but does occur with the - * Chatham prototype board. - */ - for (i = 1; i < ctrlr->num_io_queues; i++) - nvme_io_qpair_destroy(&ctrlr->ioq[i]); - - ctrlr->num_io_queues = 1; - } + ctrlr->num_io_queues = min(ctrlr->num_io_queues, sq_allocated); + ctrlr->num_io_queues = min(ctrlr->num_io_queues, cq_allocated); return (0); } @@ -687,9 +699,20 @@ static void nvme_ctrlr_start(void *ctrlr_arg) { struct nvme_controller *ctrlr = ctrlr_arg; + uint32_t old_num_io_queues; int i; - nvme_qpair_reset(&ctrlr->adminq); + /* + * Only reset adminq here when we are restarting the + * controller after a reset. During initialization, + * we have already submitted admin commands to get + * the number of I/O queues supported, so cannot reset + * the adminq again here. + */ + if (ctrlr->is_resetting) { + nvme_qpair_reset(&ctrlr->adminq); + } + for (i = 0; i < ctrlr->num_io_queues; i++) nvme_qpair_reset(&ctrlr->ioq[i]); @@ -700,11 +723,25 @@ nvme_ctrlr_start(void *ctrlr_arg) return; } + /* + * The number of qpairs are determined during controller initialization, + * including using NVMe SET_FEATURES/NUMBER_OF_QUEUES to determine the + * HW limit. We call SET_FEATURES again here so that it gets called + * after any reset for controllers that depend on the driver to + * explicit specify how many queues it will use. This value should + * never change between resets, so panic if somehow that does happen. + */ + old_num_io_queues = ctrlr->num_io_queues; if (nvme_ctrlr_set_num_qpairs(ctrlr) != 0) { nvme_ctrlr_fail(ctrlr); return; } + if (old_num_io_queues != ctrlr->num_io_queues) { + panic("num_io_queues changed from %u to %u", old_num_io_queues, + ctrlr->num_io_queues); + } + if (nvme_ctrlr_create_qpairs(ctrlr) != 0) { nvme_ctrlr_fail(ctrlr); return; @@ -727,7 +764,16 @@ nvme_ctrlr_start_config_hook(void *arg) { struct nvme_controller *ctrlr = arg; - nvme_ctrlr_start(ctrlr); + nvme_qpair_reset(&ctrlr->adminq); + nvme_admin_qpair_enable(&ctrlr->adminq); + + if (nvme_ctrlr_set_num_qpairs(ctrlr) == 0 && + nvme_ctrlr_construct_io_qpairs(ctrlr) == 0) + nvme_ctrlr_start(ctrlr); + else + nvme_ctrlr_fail(ctrlr); + + nvme_sysctl_initialize_ctrlr(ctrlr); config_intrhook_disestablish(&ctrlr->config_hook); ctrlr->is_initialized = 1; @@ -780,6 +826,7 @@ nvme_ctrlr_configure_intx(struct nvme_co ctrlr->msix_enabled = 0; ctrlr->num_io_queues = 1; + ctrlr->num_cpus_per_ioq = mp_ncpus; ctrlr->rid = 0; ctrlr->res = bus_alloc_resource_any(ctrlr->dev, SYS_RES_IRQ, &ctrlr->rid, RF_SHAREABLE | RF_ACTIVE); @@ -932,6 +979,7 @@ nvme_ctrlr_setup_interrupts(struct nvme_ device_t dev; int per_cpu_io_queues; int num_vectors_requested, num_vectors_allocated; + int num_vectors_available; dev = ctrlr->dev; per_cpu_io_queues = 1; @@ -940,52 +988,55 @@ nvme_ctrlr_setup_interrupts(struct nvme_ ctrlr->force_intx = 0; TUNABLE_INT_FETCH("hw.nvme.force_intx", &ctrlr->force_intx); - if (ctrlr->force_intx || pci_msix_count(dev) < 2) { + /* + * FreeBSD currently cannot allocate more than about 190 vectors at + * boot, meaning that systems with high core count and many devices + * requesting per-CPU interrupt vectors will not get their full + * allotment. So first, try to allocate as many as we may need to + * understand what is available, then immediately release them. + * Then figure out how many of those we will actually use, based on + * assigning an equal number of cores to each I/O queue. + */ + + /* One vector for per core I/O queue, plus one vector for admin queue. */ + num_vectors_available = min(pci_msix_count(dev), mp_ncpus + 1); + if (pci_alloc_msix(dev, &num_vectors_available) != 0) { + num_vectors_available = 0; + } + pci_release_msi(dev); + + if (ctrlr->force_intx || num_vectors_available < 2) { nvme_ctrlr_configure_intx(ctrlr); return; } - ctrlr->msix_enabled = 1; - if (per_cpu_io_queues) - ctrlr->num_io_queues = mp_ncpus; + ctrlr->num_cpus_per_ioq = NVME_CEILING(mp_ncpus, num_vectors_available + 1); else - ctrlr->num_io_queues = 1; + ctrlr->num_cpus_per_ioq = mp_ncpus; - /* One vector per IO queue, plus one vector for admin queue. */ + ctrlr->num_io_queues = NVME_CEILING(mp_ncpus, ctrlr->num_cpus_per_ioq); num_vectors_requested = ctrlr->num_io_queues + 1; - - if (pci_msix_count(dev) < num_vectors_requested) { - ctrlr->num_io_queues = 1; - num_vectors_requested = 2; /* one for admin, one for I/O */ - } - num_vectors_allocated = num_vectors_requested; + + /* + * Now just allocate the number of vectors we need. This should + * succeed, since we previously called pci_alloc_msix() + * successfully returning at least this many vectors, but just to + * be safe, if something goes wrong just revert to INTx. + */ if (pci_alloc_msix(dev, &num_vectors_allocated) != 0) { nvme_ctrlr_configure_intx(ctrlr); return; } if (num_vectors_allocated < num_vectors_requested) { - if (num_vectors_allocated < 2) { - pci_release_msi(dev); - nvme_ctrlr_configure_intx(ctrlr); - return; - } - - ctrlr->num_io_queues = 1; - /* - * Release whatever vectors were allocated, and just - * reallocate the two needed for the admin and single - * I/O qpair. - */ - num_vectors_allocated = 2; pci_release_msi(dev); - if (pci_alloc_msix(dev, &num_vectors_allocated) != 0) - panic("could not reallocate any vectors\n"); - if (num_vectors_allocated != 2) - panic("could not reallocate 2 vectors\n"); + nvme_ctrlr_configure_intx(ctrlr); + return; } + + ctrlr->msix_enabled = 1; } int @@ -1034,10 +1085,6 @@ nvme_ctrlr_construct(struct nvme_control ctrlr->max_xfer_size = NVME_MAX_XFER_SIZE; nvme_ctrlr_construct_admin_qpair(ctrlr); - status = nvme_ctrlr_construct_io_qpairs(ctrlr); - - if (status != 0) - return (status); ctrlr->cdev = make_dev(&nvme_ctrlr_cdevsw, device_get_unit(dev), UID_ROOT, GID_WHEEL, 0600, "nvme%d", device_get_unit(dev)); @@ -1149,11 +1196,7 @@ nvme_ctrlr_submit_io_request(struct nvme { struct nvme_qpair *qpair; - if (ctrlr->num_io_queues > 1) - qpair = &ctrlr->ioq[curcpu]; - else - qpair = &ctrlr->ioq[0]; - + qpair = &ctrlr->ioq[curcpu / ctrlr->num_cpus_per_ioq]; nvme_qpair_submit_request(qpair, req); } Modified: head/sys/dev/nvme/nvme_private.h ============================================================================== --- head/sys/dev/nvme/nvme_private.h Thu Jan 7 16:12:42 2016 (r293327) +++ head/sys/dev/nvme/nvme_private.h Thu Jan 7 16:18:32 2016 (r293328) @@ -265,6 +265,7 @@ struct nvme_controller { uint32_t enable_aborts; uint32_t num_io_queues; + uint32_t num_cpus_per_ioq; /* Fields for tracking progress during controller initialization. */ struct intr_config_hook config_hook;
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?201601071618.u07GIXdd054147>