Hi David,
Thanks for comments. Sorry about the delay. Replies inline.
On 9/21/21 11:53, David Gibson wrote:
On Wed, Jul 07, 2021 at 09:57:21PM -0500, Shivaprasad G Bhat wrote:
The patch adds support for the SCM flush hcall for the nvdimm devices.
To be available for exploitation by guest through the next patch.
The hcall expects the semantics such that the flush to return
with one of H_LONG_BUSY when the operation is expected to take longer
time along with a continue_token. The hcall to be called again providing
the continue_token to get the status. So, all fresh requests are put into
a 'pending' list and flush worker is submitted to the thread pool. The
thread pool completion callbacks move the requests to 'completed' list,
which are cleaned up after reporting to guest in subsequent hcalls t
<snip>
@@ -30,6 +31,7 @@
#include "hw/ppc/fdt.h"
#include "qemu/range.h"
#include "hw/ppc/spapr_numa.h"
+#include "block/thread-pool.h"
/* DIMM health bitmap bitmap indicators. Taken from kernel's papr_scm.c */
/* SCM device is unable to persist memory contents */
@@ -375,6 +377,243 @@ static target_ulong h_scm_bind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
return H_SUCCESS;
}
+static uint64_t flush_token;
Better to put this in the machine state structure than a global.
Moved it to device state itself as suggested, the states list is per
device now.
+static int flush_worker_cb(void *opaque)
+{
+ int ret = H_SUCCESS;
+ SpaprNVDIMMDeviceFlushState *state = opaque;
+
+ /* flush raw backing image */
+
<snip>
+ !QLIST_EMPTY(&spapr->completed_flush_states));
+}
+
+static int spapr_nvdimm_post_load(void *opaque, int version_id)
+{
+ SpaprMachineState *spapr = (SpaprMachineState *)opaque;
+ SpaprNVDIMMDeviceFlushState *state, *next;
+ PCDIMMDevice *dimm;
+ HostMemoryBackend *backend = NULL;
+ ThreadPool *pool = aio_get_thread_pool(qemu_get_aio_context());
+ SpaprDrc *drc;
+
+ QLIST_FOREACH_SAFE(state, &spapr->completed_flush_states, node, next) {
I don't think you need FOREACH_SAFE here. You're not removing entries
from the loop body. If you're trying to protect against concurrent
removals, I don't think FOREACH_SAFE is sufficient, you'll need an
actual lock (but I think it's already protected by the BQL).
Changing here, below and also at spapr_nvdimm_get_flush_status() while
traversing the pending list. Verified all these invocations are called
with BQL.
+ if (flush_token < state->continue_token) {
+ flush_token = state->continue_token;
+ }
+ }
+
+ QLIST_FOREACH_SAFE(state, &spapr->pending_flush_states, node, next) {
Sane comments here.
+ if (flush_token < state->continue_token) {
+ flush_token = state->continue_token;
+ }
+
+ drc = spapr_drc_by_index(state->drcidx);
+ dimm = PC_DIMM(drc->dev);
+ backend = MEMORY_BACKEND(dimm->hostmem);
+ state->backend_fd = memory_region_get_fd(&backend->mr);
+
+ thread_pool_submit_aio(pool, flush_worker_cb, state,
+ spapr_nvdimm_flush_completion_cb, state);
+ }
+
+ return 0;
+}
+
+const VMStateDescription vmstate_spapr_nvdimm_states = {
+ .name = "spapr_nvdimm_states",
+ .version_id = 1,
+ .minimum_version_id = 1,
+ .needed = spapr_nvdimm_states_needed,
+ .post_load = spapr_nvdimm_post_load,
+ .fields = (VMStateField[]) {
+ VMSTATE_QLIST_V(completed_flush_states, SpaprMachineState, 1,
+ vmstate_spapr_nvdimm_flush_state,
+ SpaprNVDIMMDeviceFlushState, node),
+ VMSTATE_QLIST_V(pending_flush_states, SpaprMachineState, 1,
+ vmstate_spapr_nvdimm_flush_state,
+ SpaprNVDIMMDeviceFlushState, node),
+ VMSTATE_END_OF_LIST()
+ },
+};
+
+/*
+ * Assign a token and reserve it for the new flush state.
+ */
+static SpaprNVDIMMDeviceFlushState *spapr_nvdimm_init_new_flush_state(
+ SpaprMachineState *spapr)
+{
+ SpaprNVDIMMDeviceFlushState *state;
+
+ state = g_malloc0(sizeof(*state));
+
+ flush_token++;
+ /* Token zero is presumed as no job pending. Handle the overflow to zero */
+ if (flush_token == 0) {
+ flush_token++;
Hmm... strictly speaking, this isn't safe. It's basically never going
to happen in practice, but in theory there's nothing preventing
continue_token 1 still being outstanding when the flush_token counter
overflows.
Come to think of it, since it's a uint64_t, I think an actual overflow
is also never going to happen in practice. Maybe we should just
assert() on overflow, and fix it in the unlikely event that we ever
discover a case where it could happen.
Have added the assert on overflow.
+ }
+ state->continue_token = flush_token;
+
+ QLIST_INSERT_HEAD(&spapr->pending_flush_states, state, node);
+
+ return state;
+}
+
+/*
+ *
Thanks!
