In the spdk_vhost_scsi_dev_remove() it takes a period of time
to remove all the tgts but before it is completed the scsi dev
has been freed. So don't free the scsi dev until all the tgts
have been removed.
Fix Github issue #932
Change-Id: Idf9293c70b8d5f82091db6dd5e018a5cb40eea36
Signed-off-by: JinYu <jin.yu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/464654
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Karol Latecki <karol.latecki@intel.com>
The dependencies between vhost and rte_vhost were not added during
earlier changes. This change moves the rte_vhost directory up to the
level of the other libraries and adds the proper dependencies for when
it is linked.
Change-Id: I089de1cd945062b64975a0011887700c0e38bb0f
Signed-off-by: Seth Howell <seth.howell@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/467700
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Paul Luse <paul.e.luse@intel.com>
Reviewed-by: Tomasz Zawadzki <tomasz.zawadzki@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
We used to call a dpdk function to do it, but using
a function for something that simple doesn't make sense.
The function also does its internal queue lookup by vid
and queue number, which could potentially fail, return an
error and technically require SPDK to handle it.
The function makes some sense for vhost-net applications
which don't touch vrings directly but rely on rte_vhost's
API for enqueueing/dequeuing mbufs. SPDK touches DPDK's
rings directly for the entire I/O handling, so it might
just as well for initialization.
This serves as cleanup.
Change-Id: Ifb44fa22ea5fc3633aa85f075aa1a5cd02f5423c
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466745
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Change the way we increase poll group reference counts
for round-robin scheduling.
So far we used to increase them whenever someone called
vhost_get_poll_group() and this worked fine for Vhost-Block
which picks a new poll group for each session. Vhost-SCSI,
however, picks only one poll group for all sessions on
a vhost device. This means that some threads will have
multiple Vhost-SCSI pollers but will still appear to the
vhost scheduler as if they had only one.
To fix it, increase poll group refcnt only when sessions
are really being started - in vhost_session_start_done().
Change-Id: I60f0d2101239e5a91138a5afd30c51dc1ccf7c2e
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466733
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Currently vhost_dev_foreach_session() accepts a single
callback function for both iterating through all active
sessions and for signaling the end of iteration (called
last time with vsession param == NULL). Now that the
final signal has completely different semantics and is
called on a specific thread, it makes sense to put it in
a separate function.
While here, remove the one-line description of
spdk_vhost_session_fn typepef. It wasn't helpful anyway.
Change-Id: I56b97180110874a813e666f964bb51c39a8ce6bb
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466732
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Currently vhost_dev_foreach_session() accepts a single
callback function for both iterating through all active
sessions and for signaling the end of iteration (called
last time with vsession param == NULL). Now that the
final signal has completely different semantics and is
called on a specific thread, it makes sense to put in
a separate function.
In this patch we prepare separate functions for the final
call, but still call them in the original callback. In
a separate patch we'll start passing both functions
directly to foreach_session().
Change-Id: I9f4338d9696f7bd15ca2d6655c6a3851569aff75
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466731
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
The function could never fail, so make it return void
rather than int. This serves as cleanup.
Change-Id: I16a857ecee8d162f546fd097acaa2e66d51ebffa
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466730
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Historically the callbacks from vhost_dev_foreach_session()
could be called with vdev argument == NULL, which would
mean that device was removed after enqueuing the event
and before consuming it. Now we keep track of pending
asynchronous operations on each vhost device and don't
allow removing it if there are any unconsumed events,
so the the vdev == NULL checks are redundant. Remove them.
Change-Id: I7aa3785080d20ed06e008c081d3f37a949228f5a
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466729
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Remove them all at once. spdk_ prefix should be
only applied to publicly exported functions.
Change-Id: Ib6d2bd0954ec5cb7c8cf253d79b9d3cd8aa0eeef
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466728
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
When rte_vhost tells us to start a session with given
vid, we lookup the corresponsing session object from
an spdk-internal session list and tell it to start
polling without even specifying any backend. The vsession->vdev->type
checks could only fail as a result of some spdk data
corruption, so replace those with just asserts now. This
code path could have never been hit in our tests anyway.
Change-Id: I97c6cbe7088f338b684d291c93cbc59c44cfdc4e
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466042
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Error messages are extremely chaotic, so unify them to
a single format:
<session name>: <error string>
Change-Id: I9b4c29321700b485e0e7eb71a73ea094cf02f000
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466041
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Error messages are extremely chaotic, so unify them to
a single format:
<session name>: <error string>
Some messages were slightly reworded to make them more clear.
I believe it would make sense to replace some of those ERRLOGs
with TRACELOGs, but that's not going to change now.
Change-Id: I32fa38b3bf26998b418b8b9e68c88ec5022c973f
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466040
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
We currently don't have any way to differentiate different
sessions e.g. in error messages. Whenever there's an error
in some session, we just print the device name.
We now introduce vsession->name with the following format:
<device name>s<dpdk connection id>
Note that it's still impossible to know exactly which
qemu process corresponds to which session in spdk, but
there's not much we could do in that matter right now.
In spdk we don't even have the accepted connection fd.
Change-Id: I666aa60c5e36bf3d56f68133042af2afc8cc5e85
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466039
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
This id is assigned from a global increment-only counter
whose type is uint64_t. I believe the original intent
was to use uint64_t for vsession->id as well, let's fix
it now to avoid wrapping errors.
Change-Id: I9cae87e6c74400590a1dc1b1f91d51e4a4c13499
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Acked-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466038
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
We've recently switched from manually calling eventfd_write()
to rte_vhost_vring_call(), which besides writing to the
eventfd, always calls a full memory barrier in the upstream
rte_vhost lib. With upstream rte_vhost we're actually
calling two memory barriers on I/O completion - one in
spdk code, one inside rte_vhost_vring_call().
The spdk barrier was only required for our internal rte_vhost
lib, whose rte_vhost_vring_call() implementation (that we
wrote) did not have such membarrier inside. So now we'll
add this membarrier there, and remove the same barrier
from spdk code.
This doesn't change any code flow for the internal rte_vhost
lib, but optimizes I/O path for the upstream version.
Change-Id: I68738d7feb9159f718b0e60ac7eed1fafd4836b9
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/466037
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Vitaliy Mysak <vitaliy.mysak@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
After recent changes, that function can not return
NULL anymore, so remove all redundant checks from
various SPDK libraries.
Change-Id: If80344b6fa81ad5f87a7086804dba221522cd7e2
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/464175
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Seth Howell <seth.howell@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
As spdk_jsonrpc_begin_result() is not allowed to return NULL we can
remove these checks. We didn't have any tests cases that goes this path
anyway.
Change-Id: I0894e76c0162591e550e70b172566b9060a6dd5f
Signed-off-by: Pawel Wodkowski <pawelx.wodkowski@intel.com>
Signed-off-by: Pawel Kaminski <pawelx.kaminski@intel.com>
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459199
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
We used to allocate a ctx whenever new event had to
be sent, but since all events in foreach_session are
always called in a chain, we could allocate one ctx
at the start and then re-initialize it before sending
each msg.
Change-Id: Ie5477b07242f0c6eb6dc2160055a829da8ba5d11
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459167
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
foreach_session() is used to e.g. close a bdev, where
for each session we close any io_channels and then,
on the final "finish" call, close the bdev descriptor.
The vhost init thread is the one that called
spdk_vhost_init() and also the same one that calls
all management APIs. One of those is for hotplugging
LUNs to vhost scsi targets, which practically results
in opening bdev descriptors.
By always scheduling that final foreach_session()
callback to the init thread, we end up with calling
spdk_bdev_close() always on the same thread which
called spdk_bdev_open(), which is actually a bdev
layer requirement.
Change-Id: I2338e15c63f93ef37dd4412dd677dee40d272ec2
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459166
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
We used to call potentially-asynchronous foreach_session()
in vdev initialization path and that was perfectly
fine because at that time there were no sessions created
and foreach_session() was always finishing synchronously.
We're about to refactor it to be always asynchronous, and
for this coalescing case it could complicate the init
error path. Once asynchronous thread msg is sent, we would
need to wait for it to complete and we just don't want to
do that. We want error handling to be simple.
Since we know there are no sessions at the time of vdev
creation, we just add a new function for setting coalescing
params just for vdev (and not for its sessions) and we
use that function in vdev init code.
Change-Id: I44d204d03b5040525e4871693678d4b4a0204e63
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459196
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Put it next to other functions in this call chain.
Change-Id: Ic621855b028f9bd110cdcda86b3a182369ec5e90
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459165
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Put it next to other functions in this call chain.
Change-Id: Ieafd91c6cfefec134594aec8671eb4efdac15dfe
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459164
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
spdk_ prefix should be only used on public API functions.
Change-Id: I663b107bd6b1c92c2c6263f2ec7c763d9812e7fe
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459163
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Despite its name, this function is defined as static
and is only used in one place, so inline it.
Change-Id: I4e217b3baae9b735761f5497f06b681a118860e9
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459162
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
The semaphore was a part of struct spdk_vhost_session_fn_ctx
so far, but since there's only one pthread waiting on that
semaphore and hence only one event using it, we could just
use a single global sem_t. Same thing with response code
for those callbacks - there's only one needed.
Going a step further, the function complete_session_event()
was removed - it would only operate on global variables now,
and its signature wouldn't make much sense after this
refactor, so it's been inlined.
This serves as cleanup.
Change-Id: I63ef41d7e1564fff5e785de101d887bc1014aad9
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459160
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Enforce spdk_vhost_fini() to be called on the same
thread which called spdk_vhost_init(). We'll also use
the newly added g_vhost_init_thread for other purposes
later on.
Change-Id: I99aebeda2d8ddaf42554aa422c32ed935634595f
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/459159
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
With all the pieces in place we can finally remove
the legacy cross thread messages from vhost.
We replace spdk_vhost_allocate_reactor() with
spdk_vhost_get_poll_group(). The returned poll_group
has to be passed to spdk_vhost_session_send_event(),
where it will be assigned to the session. After the
session it started, that poll group will be used for
all the internal vhost cross-thread messaging.
Change-Id: I17f13d3cc6e2b64e4b614c3ceb1eddb31056669b
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452207
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reported by clang:
rte_vhost_compat.c:114:36: error: taking address
of packed member 'payload' of class or structure
'vhost_user_msg' may result in an unaligned
pointer value.
To fix it, just remove the extra unaligned pointer
and inline all its accesses.
Change-Id: I7e4ab536b87ab02a4ea12c55d55a6e495c3091ca
Signed-off-by: Wojciech Malikowski <wojciech.malikowski@intel.com>
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/457559
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
struct ether_addr was renamed to struct rte_ether_addr
in latest DPDK master, but our internal fork of rte_vhost
still used the old name, which can be now a non-defined type.
Together with the struct, the RTE_ETHER_ADDR_LEN define
was renamed as well, so we'll now check if it's defined and
we'll manually define struct ether_addr to keep the old
rte_vhost working.
Change-Id: I78b8104ed3bfe03397881a94f0f8bee14f9efae8
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/457609
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
rte_vhost_vring_call() from upstream DPDK can read some
unitialized memory and crash if it's called on invalid
queue ids. The implementation in our internal rte_vhost
fork ends up wiritng to a random descriptor number, which
doesn't cause any crashes but is a bug nevertheless.
To fix it, just check if the queue is initialized before
interrupting it during the session start. It's not a hot
I/O path and there's no performance impact.
Change-Id: I830c1be98ef00d4ece9a6bd88cf79b9dfe29d2a9
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/457247
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
AIO backend requires aligned data buffers, and the maximum
IOVs supported in bdev module is defined to 32, there are
cases for Windows Guest which will send data segments more
than 32, SPDK can't process such cases, so here we can set
the 'seg_max' parameter based on bdev module capability.
Also set the maximum segment size for those requests.
Fix issue #625.
Change-Id: I0ff61e55872af17115c0b6b28425e70cb8769790
Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452378
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
The memory API has been refactored. It is not possible anymore to
register a memory region more than once. This has been introduced in
this patch: https://review.gerrithub.io/426085
In case of vhost with vvu transport, it often happens that two
consequtive vhost memory regions are mapped to virtual addresses that
lie within the same 2MB address range. This means that the vhost memory
regions may not be 2MB-aligned in the process virtual address space. As
a result, the `FLOOR_2MB()` of those addresses gives the same address.
Thus, we end up trying to register the same 2MB memory range twice.
This issue does not appear in case of AF_UNIX transport. Vhost memory
regions in case of AF_UNIX transport are hugepage backed. Therefore, the
mmapped virtual addresses of those memory regions are always
2MB-aligned. On the contrary, in case of vvu transport, the vhost memory
regions are segments of the PCI memory address space of the
virtio-vhost-user PCI device. This MMIO space is mapped in its entirety
by the DPDK vfio interface along with the other PCI BARs. Ultimately,
the vhost memory regions correspond to offsets in this mmapped PCI
memory region and thus there is no warranty that the mmapped virtual
addresses are 2MB-aligned.
This issue is fixed by skipping the already-registered 2MB memory
regions.
Change-Id: I62c9c257e6f172c894cd3454d2cbeee1986e6189
Signed-off-by: Nikos Dragazis <ndragazis@arrikto.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/441057
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
vring notification mechanism is transport-specific. At present, vhost
dataplane code in `lib/vhost/vhost.c` triggers guest notifications with
`eventfd_write()` system call. But this is an AF_UNIX specific
notification mechanism. This patch replaces `eventfd_write()` with the
existing generic `rte_vhost_vring_call()` function that is part of
DPDK's librte_vhost public API.
`rte_vhost_vring_call()` takes a vring_idx as an argument to associate
the `struct spdk_vhost_virtqueue` instance with the relevant `struct
vhost_virtqueue` instance. We introduce a new `vring_idx` field in
`struct spdk_vhost_virtqueue` to enable this association. This field is
initialized in `start_device()`. In addition, a stub for
`rte_vhost_vring_call()` is added in the vhost unit test file.
SPDK's internal `rte_vhost` copy will not be updated in order to support
the virtio-vhost-user transport. However, an `rte_vhost_vring_call()`
function is introduced in SPDK's `rte_vhost` in order to have a solid
API. This function is just a wrapper of `eventfd_write()`.
Change-Id: Ic93e25cd3f06e92f04766521bc850f1ee80b8ec8
Signed-off-by: Nikos Dragazis <ndragazis@arrikto.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/454373
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
when qemu connect to vhost, but don't send msg to vhost. We use
kill -15 to destroy vhost process. it will lead to deadlock.
(A)
* rte_vhost_driver_unregister()
* pthread_mutex_lock hold vhost_user.mutex (1)
* wait TAILQ_FIRST(&vsocket->conn_list) is NULL
(B)
* fdset_event_dispatch()
* vhost_user_read_cb() start
* vhost_user_msg_handler() start
* dev->notify_ops is NULL because qemu just connect, no message recv.
* vhost_driver_callback_get()
* pthread_mutex_lock hold vhost_user.mutex (2)
(A) & (B) deadlock
To avoid this scenes, when qemu connect in vhost_new_device()
initialize dev->notify_ops
Change-Id: Iaf699da41dfa3088cfc0f09688b50fada6b2c8d6
Signed-off-by: Tianyu yang <yangtianyu2@huawei.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/454832
Reviewed-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
We no longer have any assumptions about vhost memory regions
size being a 2MB multiple, so we can get rid of the security
check preventing some vhost sessions from being initialized.
It will be necessary for virtio-vhost-user, whose memory comes
from PCI BARs and its size may not be a 2MB multiple.
Change-Id: I48f9bc20f4c61aefdddf39ade875867148f0ed75
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/454879
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Currently, we translate each 2MB chunk to manually check
if it's contiguous with the previous one, but there are
rte_vhost APIs that do it way more efficiently.
rte_vhost_va_from_guest_pa() was introduced in DPDK 18.02,
but was backported to 17.11 as well, so we don't even need
any RTE_VERSION ifdefs to use it now. This function
calculates the remaining region size instead of trying to
translate subsequent 2MB chunks over and over.
The previous rte_vhost_gpa_to_vva() was deprecated a long
time ago and after this patch we no longer make any use of
it.
DPDK usages of this new function check if the translated
memory region has 0 length, which seems very silly, but
let's just do it in SPDK as well.
Change-Id: Ifae8daa5f810b5a2ba1524958ad2399af700b532
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/454878
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Now that sessions have a separate flag to check if the
pollers are started, we can set the lcore field on any
thread we want. We currently assign it from within the
session thread to spdk_env_get_current_core(), but we
won't be able to use an equivalent get_current_poll_group()
function after we switch to poll groups. We will only
have a poll group object inside spdk_vhost_session_send_event(),
so that's where we move the lcore assignment for now.
Change-Id: Ib5fb37ec488de80e9d79432120c81500c297b608
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452395
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
We used to rely on lcore >= 0 for sessions that are
started (have their pollers running) and in order to
prevent data races, that lcore field had to be set from
the same thread that runs the pollers, directly after
registering/unregistering them. The lcore was always
set to spdk_env_get_current_core(), but we won't be able
to use an equivalent get_current_poll_group() function
after we switch to poll groups. We will have a poll group
object only inside spdk_vhost_session_send_event() that's
called from the DPDK rte_vhost thread.
In order to change the lcore field (or a poll group one)
from spdk_vhost_session_send_event(), we'll need a separate
field to maintain the started/stopped status that's only
going to be modified from the session's thread.
Change-Id: Idb09cae3c4715eebb20282aad203987b26be707b
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452394
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Prepare to switch to spdk_thread_send_msg() which
accepts only one context parameter.
Change-Id: Iea3e8d1e715957d9b3fea12e969f29084a2948dc
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452393
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
The goal is to remove legacy event messages from vhost.
The new message passing API accepts thread objects instead
of lcore numbers and poll groups are meant to simplify
the transition.
Eventually we'd like vhost to spawn its own threads and
do message passing only within those, but SPDK libraries
can't spawn their own threads just yet. As a stopgap, vhost
will now maintain a list of all available threads (in form
of "poll groups" to mimic nvmf) and will start pollers on
them using its own round robin scheduler.
This patch only adds the poll groups list, it doesn't
change any existing functionality.
Change-Id: I89cc5da5df3612827c6fc9015f03c94b5f4a10ad
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452206
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Prepare vhost lib init to be asynchronous. We'll need
it for setting up the upcoming poll groups.
Change-Id: I3c66b3f17f8635d4b705dd988393431193938971
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452205
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Put all shutdown functions in a single place. This also
lets us remove one forward declaration.
Change-Id: I8c8c602e67e3dafd3cd5e80bc9dd90f23381711e
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452392
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Switch to the new spdk_thread_send_msg() API instead.
Change-Id: I810465cc49d5c4ef23e04953aa29d369f48f68b1
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/452391
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
Reviewed-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
We do technically support initiators without eventq or
controlq, but the lun hotplug/hotremove path expected the
eventq to be always present.
This was causing vhost to randomly crash in the fuzz tests.
Specifically, the crash happened if lun hotplug was handled
while a VM was in the middle of switching from BIOS to OS.
We fix it by checking if eventq is set before putting
any event there. LUN hotplug and hotremove won't work
without an eventq, but the entire session will be restarted
after new queues are initialized. This will make the VM
retrieve all up-to-date luns after OS initialization is
complete.
Change-Id: I5d28cbedad8fb2a35ede5a491aeb7fdc52faad06
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/451789
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
This is the end of the patch series. After this patch,
delete_target_node RPC will wait for the completion of
removal of the SCSI device and then free the iSCSI target.
SCSI device holds passed callback and calls it in free_dev().
free_dev() is ensured to be called after all iSCSI sessions
are closed. So iSCSI target resource can be freed safely
after that.
Change-Id: I25921b4014207092b7b3845dfeae58bcdffa2edc
Signed-off-by: Shuhei Matsumoto <shuhei.matsumoto.xt@hitachi.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/450607
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
spdk_dma_malloc() is not required here, as the device
object is neither DMA-able nor shared between processes.
The device structures used to be aligned to cache line
size, but that's just a leftover from before sessions
were introduced. The device object is just a generic
device information that can be accessed from any thread
holding the proper mutex. The hot data used in the I/O
path sits in the session structure, which is now allocated
with posix_memalloc() to ensure proper alignment.
Vhost NVMe is an exception, as the device struct is used
as hot I/O data for the one and only session it supports,
so it's also allocated with posix_memalloc().
While here, also allocate various vhost buffers using
spdk_zmalloc() instead of spdk_dma_zmalloc(), as
spdk_dma_*malloc() is about to be deprecated.
Change-Id: Ic7f63185639b7b98dc1ef756166c826a0af87b44
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/450551
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Ben Walker <benjamin.walker@intel.com>
The previous patches described as optimizations also
fixed some issues. They seem sufficient to cover all
the error cases, but the real source of the problem
lies in foreach_session() initiated by the device backend,
which can use sessions that were never seen by the
backend.
The backends are only notified when a session is
*started*, but foreach_session() iterates through
all the sessions - even those that were never started.
Vhost SCSI, for example, in the foreach_session() callbacks
used to expect svsession->svdev to be always set, but
that field is only set when the session gets started.
A perfect solution would to introduce a new backend
callback to be called on new connection. Vhost SCSI
could set e.g. svsession->svdev inside. For now we go
with much easier solution that prevents sessions from
being used in foreach-session() unless they were
started at least once. (...and e.g. got their ->svdev set)
Change-Id: Ida30a1f27f99977360d08a71a64fc92931b25b75
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/449394
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Before SCSI target is removed, all vhost sessions need
to drain their pending I/O and put their I/O channels.
After a session puts it channel, it sends an async
notification to the entire vhost device. The device
will check if there are any other sessions still
referencing the SCSI target and if not - it will
continue removing the spdk_scsi_dev object. There may
be multiple sessions sending those async events at the
same time, and while we do protect from removing the
same spdk_scsi_dev twice, we can still remove
a different spdk_scsi_dev that was hot-attached in the
meantime with the same target ID.
1. SCSI target hotremove (e.g. via RPC or bdev hotremove)
/ \
/ \
session A session B
drain I/O drain I/O
| |
v |
done v
send event done
\ send event*
\
All sessions have detached the SCSI target, remove
it from the entire vhost device. From this point
a new target can be hot-attached (e.g. via RPC).
2. Attach a SCSI target with with same target ID.
3. Hotremove event* from the previous SCSI target gets
finally executed. SCSI target with that ID is
occupied (again) and may be hotremoved by mistake.
The role of that hotremove event is just to kick the
vhost device and make it remove any scsi targets that
can be removed, so add a check preventing it from
removing devices in states other than REMOVING.
Change-Id: Ia1cc7cae797fd8859d485e63f0ef37aeac2945d0
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/449990
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
Always unset the VHOST_SCSI_DEV_REMOVED status on
session stop, so that we won't send hotremove SCSI
sense codes after e.g. a VM gets rebooted. The VM
should generally enumerate the SCSI devices again
in such case. We already unset the REMOVED status
for devices which were still attached at the time
of the session stop, but the devices hotremoved
before the session stop retained their REMOVED
status, giving us inconsistent behavior.
Change-Id: I7c5876e29f4bdc99cc060f1d891e24ac57051f37
Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/449709
Tested-by: SPDK CI Jenkins <sys_sgci@intel.com>
Reviewed-by: Jim Harris <james.r.harris@intel.com>
Reviewed-by: Changpeng Liu <changpeng.liu@intel.com>
