* [RFC Qemu PATCH v2 0/2] spapr: nvdimm: Asynchronus flush hcall support
@ 2020-11-30 15:16 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2020-11-30 15:16 UTC (permalink / raw)
To: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc
Cc: shivaprasadbhat, bharata, linux-nvdimm, linuxppc-dev, kvm-ppc,
aneesh.kumar
The nvdimm devices are expected to ensure write persistent during power
failure kind of scenarios.
The libpmem has architecture specific instructions like dcbf on power
to flush the cache data to backend nvdimm device during normal writes.
Qemu - virtual nvdimm devices are memory mapped. The dcbf in the guest
doesn't traslate to actual flush to the backend file on the host in case
of file backed vnvdimms. This is addressed by virtio-pmem in case of x86_64
by making asynchronous flushes.
On PAPR, issue is addressed by adding a new hcall to
request for an explicit asynchronous flush requests from the guest ndctl
driver when the backend nvdimm cannot ensure write persistence with dcbf
alone. So, the approach here is to convey when the asynchronous flush is
required in a device tree property. The guest makes the hcall when the
property is found, instead of relying on dcbf.
The first patch adds the necessary asynchronous hcall support infrastructure
code at the DRC level. Second patch implements the hcall using the
infrastructure.
Hcall semantics are in review and not final.
A new device property sync-dax is added to the nvdimm device. When the
sync-dax is off(default), the asynchronous hcalls will be called.
With respect to save from new qemu to restore on old qemu, having the
sync-dax by default off(when not specified) causes IO errors in guests as
the async-hcall would not be supported on old qemu. The new hcall
implementation being supported only on the new pseries machine version,
the current machine version checks may be sufficient to prevent
such migration. Please suggest what should be done.
The below demonstration shows the map_sync behavior with sync-dax on & off.
(https://github.com/avocado-framework-tests/avocado-misc-tests/blob/master/memory/ndctl.py.data/map_sync.c)
The pmem0 is from nvdimm with With sync-dax=on, and pmem1 is from nvdimm with syn-dax=off, mounted as
/dev/pmem0 on /mnt1 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
/dev/pmem1 on /mnt2 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
[root@atest-guest ~]# ./mapsync /mnt1/newfile ----> When sync-dax=off
[root@atest-guest ~]# ./mapsync /mnt2/newfile ----> when sync-dax=on
Failed to mmap with Operation not supported
---
v1 - https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06330.html
Changes from v1
- Fixed a missed-out unlock
- using QLIST_FOREACH instead of QLIST_FOREACH_SAFE while generating token
Shivaprasad G Bhat (2):
spapr: drc: Add support for async hcalls at the drc level
spapr: nvdimm: Implement async flush hcalls
hw/mem/nvdimm.c | 1
hw/ppc/spapr_drc.c | 146 ++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/spapr_nvdimm.c | 79 ++++++++++++++++++++++++
include/hw/mem/nvdimm.h | 10 +++
include/hw/ppc/spapr.h | 3 +
include/hw/ppc/spapr_drc.h | 25 ++++++++
6 files changed, 263 insertions(+), 1 deletion(-)
--
Signature
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 33+ messages in thread
* [RFC Qemu PATCH v2 0/2] spapr: nvdimm: Asynchronus flush hcall support
@ 2020-11-30 15:16 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2020-11-30 15:16 UTC (permalink / raw)
To: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc
Cc: linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
The nvdimm devices are expected to ensure write persistent during power
failure kind of scenarios.
The libpmem has architecture specific instructions like dcbf on power
to flush the cache data to backend nvdimm device during normal writes.
Qemu - virtual nvdimm devices are memory mapped. The dcbf in the guest
doesn't traslate to actual flush to the backend file on the host in case
of file backed vnvdimms. This is addressed by virtio-pmem in case of x86_64
by making asynchronous flushes.
On PAPR, issue is addressed by adding a new hcall to
request for an explicit asynchronous flush requests from the guest ndctl
driver when the backend nvdimm cannot ensure write persistence with dcbf
alone. So, the approach here is to convey when the asynchronous flush is
required in a device tree property. The guest makes the hcall when the
property is found, instead of relying on dcbf.
The first patch adds the necessary asynchronous hcall support infrastructure
code at the DRC level. Second patch implements the hcall using the
infrastructure.
Hcall semantics are in review and not final.
A new device property sync-dax is added to the nvdimm device. When the
sync-dax is off(default), the asynchronous hcalls will be called.
With respect to save from new qemu to restore on old qemu, having the
sync-dax by default off(when not specified) causes IO errors in guests as
the async-hcall would not be supported on old qemu. The new hcall
implementation being supported only on the new pseries machine version,
the current machine version checks may be sufficient to prevent
such migration. Please suggest what should be done.
The below demonstration shows the map_sync behavior with sync-dax on & off.
(https://github.com/avocado-framework-tests/avocado-misc-tests/blob/master/memory/ndctl.py.data/map_sync.c)
The pmem0 is from nvdimm with With sync-dax=on, and pmem1 is from nvdimm with syn-dax=off, mounted as
/dev/pmem0 on /mnt1 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
/dev/pmem1 on /mnt2 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
[root@atest-guest ~]# ./mapsync /mnt1/newfile ----> When sync-dax=off
[root@atest-guest ~]# ./mapsync /mnt2/newfile ----> when sync-dax=on
Failed to mmap with Operation not supported
---
v1 - https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06330.html
Changes from v1
- Fixed a missed-out unlock
- using QLIST_FOREACH instead of QLIST_FOREACH_SAFE while generating token
Shivaprasad G Bhat (2):
spapr: drc: Add support for async hcalls at the drc level
spapr: nvdimm: Implement async flush hcalls
hw/mem/nvdimm.c | 1
hw/ppc/spapr_drc.c | 146 ++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/spapr_nvdimm.c | 79 ++++++++++++++++++++++++
include/hw/mem/nvdimm.h | 10 +++
include/hw/ppc/spapr.h | 3 +
include/hw/ppc/spapr_drc.h | 25 ++++++++
6 files changed, 263 insertions(+), 1 deletion(-)
--
Signature
^ permalink raw reply [flat|nested] 33+ messages in thread
* [RFC Qemu PATCH v2 0/2] spapr: nvdimm: Asynchronus flush hcall support
@ 2020-11-30 15:16 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2020-11-30 15:16 UTC (permalink / raw)
To: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc
Cc: shivaprasadbhat, bharata, linux-nvdimm, linuxppc-dev, kvm-ppc,
aneesh.kumar
The nvdimm devices are expected to ensure write persistent during power
failure kind of scenarios.
The libpmem has architecture specific instructions like dcbf on power
to flush the cache data to backend nvdimm device during normal writes.
Qemu - virtual nvdimm devices are memory mapped. The dcbf in the guest
doesn't traslate to actual flush to the backend file on the host in case
of file backed vnvdimms. This is addressed by virtio-pmem in case of x86_64
by making asynchronous flushes.
On PAPR, issue is addressed by adding a new hcall to
request for an explicit asynchronous flush requests from the guest ndctl
driver when the backend nvdimm cannot ensure write persistence with dcbf
alone. So, the approach here is to convey when the asynchronous flush is
required in a device tree property. The guest makes the hcall when the
property is found, instead of relying on dcbf.
The first patch adds the necessary asynchronous hcall support infrastructure
code at the DRC level. Second patch implements the hcall using the
infrastructure.
Hcall semantics are in review and not final.
A new device property sync-dax is added to the nvdimm device. When the
sync-dax is off(default), the asynchronous hcalls will be called.
With respect to save from new qemu to restore on old qemu, having the
sync-dax by default off(when not specified) causes IO errors in guests as
the async-hcall would not be supported on old qemu. The new hcall
implementation being supported only on the new pseries machine version,
the current machine version checks may be sufficient to prevent
such migration. Please suggest what should be done.
The below demonstration shows the map_sync behavior with sync-dax on & off.
(https://github.com/avocado-framework-tests/avocado-misc-tests/blob/master/memory/ndctl.py.data/map_sync.c)
The pmem0 is from nvdimm with With sync-dax=on, and pmem1 is from nvdimm with syn-dax=off, mounted as
/dev/pmem0 on /mnt1 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize2k,noquota)
/dev/pmem1 on /mnt2 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize2k,noquota)
[root@atest-guest ~]# ./mapsync /mnt1/newfile ----> When sync-dax=off
[root@atest-guest ~]# ./mapsync /mnt2/newfile ----> when sync-dax=on
Failed to mmap with Operation not supported
---
v1 - https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06330.html
Changes from v1
- Fixed a missed-out unlock
- using QLIST_FOREACH instead of QLIST_FOREACH_SAFE while generating token
Shivaprasad G Bhat (2):
spapr: drc: Add support for async hcalls at the drc level
spapr: nvdimm: Implement async flush hcalls
hw/mem/nvdimm.c | 1
hw/ppc/spapr_drc.c | 146 ++++++++++++++++++++++++++++++++++++++++++++
hw/ppc/spapr_nvdimm.c | 79 ++++++++++++++++++++++++
include/hw/mem/nvdimm.h | 10 +++
include/hw/ppc/spapr.h | 3 +
include/hw/ppc/spapr_drc.h | 25 ++++++++
6 files changed, 263 insertions(+), 1 deletion(-)
--
Signature
^ permalink raw reply [flat|nested] 33+ messages in thread
* [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
2020-11-30 15:16 ` Shivaprasad G Bhat
(?)
@ 2020-11-30 15:16 ` Shivaprasad G Bhat
-1 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2020-11-30 15:16 UTC (permalink / raw)
To: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc
Cc: shivaprasadbhat, bharata, linux-nvdimm, linuxppc-dev, kvm-ppc,
aneesh.kumar
The patch adds support for async hcalls at the DRC level for the
spapr devices. To be used by spapr-scm devices in the patch/es to follow.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
include/hw/ppc/spapr_drc.h | 25 +++++++
2 files changed, 174 insertions(+)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 77718cde1f..4ecd04f686 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -15,6 +15,7 @@
#include "qapi/qmp/qnull.h"
#include "cpu.h"
#include "qemu/cutils.h"
+#include "qemu/guest-random.h"
#include "hw/ppc/spapr_drc.h"
#include "qom/object.h"
#include "migration/vmstate.h"
@@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
spapr_drc_release(drc);
}
+
+/*
+ * @drc : device DRC targetting which the async hcalls to be made.
+ *
+ * All subsequent requests to run/query the status should use the
+ * unique token returned here.
+ */
+uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
+{
+ Error *err = NULL;
+ uint64_t token;
+ SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
+
+ state = g_malloc0(sizeof(*state));
+ state->pending = true;
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+retry:
+ if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
+ error_report_err(err);
+ g_free(state);
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+ return 0;
+ }
+
+ if (!token) /* Token should be non-zero */
+ goto retry;
+
+ if (!QLIST_EMPTY(&drc->async_hcall_states)) {
+ QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
+ if (tmp->continue_token == token) {
+ /* If the token already in use, get a new one */
+ goto retry;
+ }
+ }
+ }
+
+ state->continue_token = token;
+ QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
+
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+
+ return state->continue_token;
+}
+
+static void *spapr_drc_async_hcall_runner(void *opaque)
+{
+ int response = -1;
+ SpaprDrcDeviceAsyncHCallState *state = opaque;
+
+ /*
+ * state is freed only after this thread finishes(after pthread_join()),
+ * don't worry about it becoming NULL.
+ */
+
+ response = state->func(state->data);
+
+ state->hcall_ret = response;
+ state->pending = 0;
+
+ return NULL;
+}
+
+/*
+ * @drc : device DRC targetting which the async hcalls to be made.
+ * token : The continue token to be used for tracking as recived from
+ * spapr_drc_get_new_async_hcall_token
+ * @func() : the worker function which needs to be executed asynchronously
+ * @data : data to be passed to the asynchronous function. Worker is supposed
+ * to free/cleanup the data that is passed here
+ */
+void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
+ SpaprDrcAsyncHcallWorkerFunc *func, void *data)
+{
+ SpaprDrcDeviceAsyncHCallState *state;
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+ QLIST_FOREACH(state, &drc->async_hcall_states, node) {
+ if (state->continue_token == token) {
+ state->func = func;
+ state->data = data;
+ qemu_thread_create(&state->thread, "sPAPR Async HCALL",
+ spapr_drc_async_hcall_runner, state,
+ QEMU_THREAD_JOINABLE);
+ break;
+ }
+ }
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+}
+
+/*
+ * spapr_drc_finish_async_hcalls
+ * Waits for all pending async requests to complete
+ * thier execution and free the states
+ */
+static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
+{
+ SpaprDrcDeviceAsyncHCallState *state, *next;
+
+ if (QLIST_EMPTY(&drc->async_hcall_states)) {
+ return;
+ }
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+ QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
+ qemu_thread_join(&state->thread);
+ QLIST_REMOVE(state, node);
+ g_free(state);
+ }
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+}
+
+/*
+ * spapr_drc_get_async_hcall_status
+ * Fetches the status of the hcall worker and returns H_BUSY
+ * if the worker is still running.
+ */
+int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
+{
+ int ret = H_PARAMETER;
+ SpaprDrcDeviceAsyncHCallState *state, *node;
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+ QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
+ if (state->continue_token == token) {
+ if (state->pending) {
+ ret = H_BUSY;
+ break;
+ } else {
+ ret = state->hcall_ret;
+ qemu_thread_join(&state->thread);
+ QLIST_REMOVE(state, node);
+ g_free(state);
+ break;
+ }
+ }
+ }
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+
+ return ret;
+}
+
void spapr_drc_reset(SpaprDrc *drc)
{
SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
@@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
drc->ccs_offset = -1;
drc->ccs_depth = -1;
}
+ spapr_drc_finish_async_hcalls(drc);
}
static bool spapr_drc_unplug_requested_needed(void *opaque)
@@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
drc->owner = owner;
prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
spapr_drc_index(drc));
+
object_property_add_child(owner, prop_name, OBJECT(drc));
object_unref(OBJECT(drc));
qdev_realize(DEVICE(drc), NULL, NULL);
@@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
object_property_add(obj, "fdt", "struct", prop_get_fdt,
NULL, NULL, NULL);
drc->state = drck->empty_state;
+
+ qemu_mutex_init(&drc->async_hcall_states_lock);
+ QLIST_INIT(&drc->async_hcall_states);
+
}
static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 165b281496..77f6e4386c 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -18,6 +18,7 @@
#include "sysemu/runstate.h"
#include "hw/qdev-core.h"
#include "qapi/error.h"
+#include "block/thread-pool.h"
#define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
#define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
@@ -168,6 +169,21 @@ typedef enum {
SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
} SpaprDrcState;
+typedef struct SpaprDrc SpaprDrc;
+
+typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
+typedef struct SpaprDrcDeviceAsyncHCallState {
+ uint64_t continue_token;
+ bool pending;
+
+ int hcall_ret;
+ SpaprDrcAsyncHcallWorkerFunc *func;
+ void *data;
+
+ QemuThread thread;
+
+ QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
+} SpaprDrcDeviceAsyncHCallState;
typedef struct SpaprDrc {
/*< private >*/
DeviceState parent;
@@ -182,6 +198,10 @@ typedef struct SpaprDrc {
int ccs_offset;
int ccs_depth;
+ /* async hcall states */
+ QemuMutex async_hcall_states_lock;
+ QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
+
/* device pointer, via link property */
DeviceState *dev;
bool unplug_requested;
@@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
/* Returns true if a hot plug/unplug request is pending */
bool spapr_drc_transient(SpaprDrc *drc);
+uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
+void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
+ SpaprDrcAsyncHcallWorkerFunc, void *data);
+int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
+
static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
{
return drc->unplug_requested;
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2020-11-30 15:16 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2020-11-30 15:16 UTC (permalink / raw)
To: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc
Cc: linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
The patch adds support for async hcalls at the DRC level for the
spapr devices. To be used by spapr-scm devices in the patch/es to follow.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
include/hw/ppc/spapr_drc.h | 25 +++++++
2 files changed, 174 insertions(+)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 77718cde1f..4ecd04f686 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -15,6 +15,7 @@
#include "qapi/qmp/qnull.h"
#include "cpu.h"
#include "qemu/cutils.h"
+#include "qemu/guest-random.h"
#include "hw/ppc/spapr_drc.h"
#include "qom/object.h"
#include "migration/vmstate.h"
@@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
spapr_drc_release(drc);
}
+
+/*
+ * @drc : device DRC targetting which the async hcalls to be made.
+ *
+ * All subsequent requests to run/query the status should use the
+ * unique token returned here.
+ */
+uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
+{
+ Error *err = NULL;
+ uint64_t token;
+ SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
+
+ state = g_malloc0(sizeof(*state));
+ state->pending = true;
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+retry:
+ if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
+ error_report_err(err);
+ g_free(state);
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+ return 0;
+ }
+
+ if (!token) /* Token should be non-zero */
+ goto retry;
+
+ if (!QLIST_EMPTY(&drc->async_hcall_states)) {
+ QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
+ if (tmp->continue_token == token) {
+ /* If the token already in use, get a new one */
+ goto retry;
+ }
+ }
+ }
+
+ state->continue_token = token;
+ QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
+
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+
+ return state->continue_token;
+}
+
+static void *spapr_drc_async_hcall_runner(void *opaque)
+{
+ int response = -1;
+ SpaprDrcDeviceAsyncHCallState *state = opaque;
+
+ /*
+ * state is freed only after this thread finishes(after pthread_join()),
+ * don't worry about it becoming NULL.
+ */
+
+ response = state->func(state->data);
+
+ state->hcall_ret = response;
+ state->pending = 0;
+
+ return NULL;
+}
+
+/*
+ * @drc : device DRC targetting which the async hcalls to be made.
+ * token : The continue token to be used for tracking as recived from
+ * spapr_drc_get_new_async_hcall_token
+ * @func() : the worker function which needs to be executed asynchronously
+ * @data : data to be passed to the asynchronous function. Worker is supposed
+ * to free/cleanup the data that is passed here
+ */
+void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
+ SpaprDrcAsyncHcallWorkerFunc *func, void *data)
+{
+ SpaprDrcDeviceAsyncHCallState *state;
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+ QLIST_FOREACH(state, &drc->async_hcall_states, node) {
+ if (state->continue_token == token) {
+ state->func = func;
+ state->data = data;
+ qemu_thread_create(&state->thread, "sPAPR Async HCALL",
+ spapr_drc_async_hcall_runner, state,
+ QEMU_THREAD_JOINABLE);
+ break;
+ }
+ }
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+}
+
+/*
+ * spapr_drc_finish_async_hcalls
+ * Waits for all pending async requests to complete
+ * thier execution and free the states
+ */
+static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
+{
+ SpaprDrcDeviceAsyncHCallState *state, *next;
+
+ if (QLIST_EMPTY(&drc->async_hcall_states)) {
+ return;
+ }
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+ QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
+ qemu_thread_join(&state->thread);
+ QLIST_REMOVE(state, node);
+ g_free(state);
+ }
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+}
+
+/*
+ * spapr_drc_get_async_hcall_status
+ * Fetches the status of the hcall worker and returns H_BUSY
+ * if the worker is still running.
+ */
+int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
+{
+ int ret = H_PARAMETER;
+ SpaprDrcDeviceAsyncHCallState *state, *node;
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+ QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
+ if (state->continue_token == token) {
+ if (state->pending) {
+ ret = H_BUSY;
+ break;
+ } else {
+ ret = state->hcall_ret;
+ qemu_thread_join(&state->thread);
+ QLIST_REMOVE(state, node);
+ g_free(state);
+ break;
+ }
+ }
+ }
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+
+ return ret;
+}
+
void spapr_drc_reset(SpaprDrc *drc)
{
SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
@@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
drc->ccs_offset = -1;
drc->ccs_depth = -1;
}
+ spapr_drc_finish_async_hcalls(drc);
}
static bool spapr_drc_unplug_requested_needed(void *opaque)
@@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
drc->owner = owner;
prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
spapr_drc_index(drc));
+
object_property_add_child(owner, prop_name, OBJECT(drc));
object_unref(OBJECT(drc));
qdev_realize(DEVICE(drc), NULL, NULL);
@@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
object_property_add(obj, "fdt", "struct", prop_get_fdt,
NULL, NULL, NULL);
drc->state = drck->empty_state;
+
+ qemu_mutex_init(&drc->async_hcall_states_lock);
+ QLIST_INIT(&drc->async_hcall_states);
+
}
static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 165b281496..77f6e4386c 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -18,6 +18,7 @@
#include "sysemu/runstate.h"
#include "hw/qdev-core.h"
#include "qapi/error.h"
+#include "block/thread-pool.h"
#define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
#define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
@@ -168,6 +169,21 @@ typedef enum {
SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
} SpaprDrcState;
+typedef struct SpaprDrc SpaprDrc;
+
+typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
+typedef struct SpaprDrcDeviceAsyncHCallState {
+ uint64_t continue_token;
+ bool pending;
+
+ int hcall_ret;
+ SpaprDrcAsyncHcallWorkerFunc *func;
+ void *data;
+
+ QemuThread thread;
+
+ QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
+} SpaprDrcDeviceAsyncHCallState;
typedef struct SpaprDrc {
/*< private >*/
DeviceState parent;
@@ -182,6 +198,10 @@ typedef struct SpaprDrc {
int ccs_offset;
int ccs_depth;
+ /* async hcall states */
+ QemuMutex async_hcall_states_lock;
+ QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
+
/* device pointer, via link property */
DeviceState *dev;
bool unplug_requested;
@@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
/* Returns true if a hot plug/unplug request is pending */
bool spapr_drc_transient(SpaprDrc *drc);
+uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
+void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
+ SpaprDrcAsyncHcallWorkerFunc, void *data);
+int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
+
static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
{
return drc->unplug_requested;
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2020-11-30 15:16 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2020-11-30 15:16 UTC (permalink / raw)
To: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc
Cc: shivaprasadbhat, bharata, linux-nvdimm, linuxppc-dev, kvm-ppc,
aneesh.kumar
The patch adds support for async hcalls at the DRC level for the
spapr devices. To be used by spapr-scm devices in the patch/es to follow.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
include/hw/ppc/spapr_drc.h | 25 +++++++
2 files changed, 174 insertions(+)
diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
index 77718cde1f..4ecd04f686 100644
--- a/hw/ppc/spapr_drc.c
+++ b/hw/ppc/spapr_drc.c
@@ -15,6 +15,7 @@
#include "qapi/qmp/qnull.h"
#include "cpu.h"
#include "qemu/cutils.h"
+#include "qemu/guest-random.h"
#include "hw/ppc/spapr_drc.h"
#include "qom/object.h"
#include "migration/vmstate.h"
@@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
spapr_drc_release(drc);
}
+
+/*
+ * @drc : device DRC targetting which the async hcalls to be made.
+ *
+ * All subsequent requests to run/query the status should use the
+ * unique token returned here.
+ */
+uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
+{
+ Error *err = NULL;
+ uint64_t token;
+ SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
+
+ state = g_malloc0(sizeof(*state));
+ state->pending = true;
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+retry:
+ if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
+ error_report_err(err);
+ g_free(state);
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+ return 0;
+ }
+
+ if (!token) /* Token should be non-zero */
+ goto retry;
+
+ if (!QLIST_EMPTY(&drc->async_hcall_states)) {
+ QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
+ if (tmp->continue_token = token) {
+ /* If the token already in use, get a new one */
+ goto retry;
+ }
+ }
+ }
+
+ state->continue_token = token;
+ QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
+
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+
+ return state->continue_token;
+}
+
+static void *spapr_drc_async_hcall_runner(void *opaque)
+{
+ int response = -1;
+ SpaprDrcDeviceAsyncHCallState *state = opaque;
+
+ /*
+ * state is freed only after this thread finishes(after pthread_join()),
+ * don't worry about it becoming NULL.
+ */
+
+ response = state->func(state->data);
+
+ state->hcall_ret = response;
+ state->pending = 0;
+
+ return NULL;
+}
+
+/*
+ * @drc : device DRC targetting which the async hcalls to be made.
+ * token : The continue token to be used for tracking as recived from
+ * spapr_drc_get_new_async_hcall_token
+ * @func() : the worker function which needs to be executed asynchronously
+ * @data : data to be passed to the asynchronous function. Worker is supposed
+ * to free/cleanup the data that is passed here
+ */
+void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
+ SpaprDrcAsyncHcallWorkerFunc *func, void *data)
+{
+ SpaprDrcDeviceAsyncHCallState *state;
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+ QLIST_FOREACH(state, &drc->async_hcall_states, node) {
+ if (state->continue_token = token) {
+ state->func = func;
+ state->data = data;
+ qemu_thread_create(&state->thread, "sPAPR Async HCALL",
+ spapr_drc_async_hcall_runner, state,
+ QEMU_THREAD_JOINABLE);
+ break;
+ }
+ }
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+}
+
+/*
+ * spapr_drc_finish_async_hcalls
+ * Waits for all pending async requests to complete
+ * thier execution and free the states
+ */
+static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
+{
+ SpaprDrcDeviceAsyncHCallState *state, *next;
+
+ if (QLIST_EMPTY(&drc->async_hcall_states)) {
+ return;
+ }
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+ QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
+ qemu_thread_join(&state->thread);
+ QLIST_REMOVE(state, node);
+ g_free(state);
+ }
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+}
+
+/*
+ * spapr_drc_get_async_hcall_status
+ * Fetches the status of the hcall worker and returns H_BUSY
+ * if the worker is still running.
+ */
+int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
+{
+ int ret = H_PARAMETER;
+ SpaprDrcDeviceAsyncHCallState *state, *node;
+
+ qemu_mutex_lock(&drc->async_hcall_states_lock);
+ QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
+ if (state->continue_token = token) {
+ if (state->pending) {
+ ret = H_BUSY;
+ break;
+ } else {
+ ret = state->hcall_ret;
+ qemu_thread_join(&state->thread);
+ QLIST_REMOVE(state, node);
+ g_free(state);
+ break;
+ }
+ }
+ }
+ qemu_mutex_unlock(&drc->async_hcall_states_lock);
+
+ return ret;
+}
+
void spapr_drc_reset(SpaprDrc *drc)
{
SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
@@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
drc->ccs_offset = -1;
drc->ccs_depth = -1;
}
+ spapr_drc_finish_async_hcalls(drc);
}
static bool spapr_drc_unplug_requested_needed(void *opaque)
@@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
drc->owner = owner;
prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
spapr_drc_index(drc));
+
object_property_add_child(owner, prop_name, OBJECT(drc));
object_unref(OBJECT(drc));
qdev_realize(DEVICE(drc), NULL, NULL);
@@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
object_property_add(obj, "fdt", "struct", prop_get_fdt,
NULL, NULL, NULL);
drc->state = drck->empty_state;
+
+ qemu_mutex_init(&drc->async_hcall_states_lock);
+ QLIST_INIT(&drc->async_hcall_states);
+
}
static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
index 165b281496..77f6e4386c 100644
--- a/include/hw/ppc/spapr_drc.h
+++ b/include/hw/ppc/spapr_drc.h
@@ -18,6 +18,7 @@
#include "sysemu/runstate.h"
#include "hw/qdev-core.h"
#include "qapi/error.h"
+#include "block/thread-pool.h"
#define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
#define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
@@ -168,6 +169,21 @@ typedef enum {
SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
} SpaprDrcState;
+typedef struct SpaprDrc SpaprDrc;
+
+typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
+typedef struct SpaprDrcDeviceAsyncHCallState {
+ uint64_t continue_token;
+ bool pending;
+
+ int hcall_ret;
+ SpaprDrcAsyncHcallWorkerFunc *func;
+ void *data;
+
+ QemuThread thread;
+
+ QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
+} SpaprDrcDeviceAsyncHCallState;
typedef struct SpaprDrc {
/*< private >*/
DeviceState parent;
@@ -182,6 +198,10 @@ typedef struct SpaprDrc {
int ccs_offset;
int ccs_depth;
+ /* async hcall states */
+ QemuMutex async_hcall_states_lock;
+ QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
+
/* device pointer, via link property */
DeviceState *dev;
bool unplug_requested;
@@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
/* Returns true if a hot plug/unplug request is pending */
bool spapr_drc_transient(SpaprDrc *drc);
+uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
+void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
+ SpaprDrcAsyncHcallWorkerFunc, void *data);
+int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
+
static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
{
return drc->unplug_requested;
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [RFC Qemu PATCH v2 2/2] spapr: nvdimm: Implement async flush hcalls
2020-11-30 15:16 ` Shivaprasad G Bhat
(?)
@ 2020-11-30 15:17 ` Shivaprasad G Bhat
-1 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2020-11-30 15:17 UTC (permalink / raw)
To: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc
Cc: shivaprasadbhat, bharata, linux-nvdimm, linuxppc-dev, kvm-ppc,
aneesh.kumar
When the persistent memory beacked by a file, a cpu cache flush instruction
is not sufficient to ensure the stores are correctly flushed to the media.
The patch implements the async hcalls for flush operation on demand from the
guest kernel.
The device option sync-dax is by default off and enables explicit asynchronous
flush requests from guest. It can be disabled by setting syn-dax=on.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
hw/mem/nvdimm.c | 1 +
hw/ppc/spapr_nvdimm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++
include/hw/mem/nvdimm.h | 10 ++++++
include/hw/ppc/spapr.h | 3 +-
4 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 03c2201b56..37a4db0135 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -220,6 +220,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
static Property nvdimm_properties[] = {
DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
+ DEFINE_PROP_BOOL(NVDIMM_SYNC_DAX_PROP, NVDIMMDevice, sync_dax, false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5e..557e36aa98 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -22,6 +22,7 @@
* THE SOFTWARE.
*/
#include "qemu/osdep.h"
+#include "qemu/cutils.h"
#include "qapi/error.h"
#include "hw/ppc/spapr_drc.h"
#include "hw/ppc/spapr_nvdimm.h"
@@ -155,6 +156,11 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
"operating-system")));
_FDT(fdt_setprop(fdt, child_offset, "ibm,cache-flush-required", NULL, 0));
+ if (!nvdimm->sync_dax) {
+ _FDT(fdt_setprop(fdt, child_offset, "ibm,async-flush-required",
+ NULL, 0));
+ }
+
return child_offset;
}
@@ -370,6 +376,78 @@ static target_ulong h_scm_bind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
return H_SUCCESS;
}
+typedef struct SCMAsyncFlushData {
+ int fd;
+ uint64_t token;
+} SCMAsyncFlushData;
+
+static int flush_worker_cb(void *opaque)
+{
+ int ret = H_SUCCESS;
+ SCMAsyncFlushData *req_data = opaque;
+
+ /* flush raw backing image */
+ if (qemu_fdatasync(req_data->fd) < 0) {
+ error_report("papr_scm: Could not sync nvdimm to backend file: %s",
+ strerror(errno));
+ ret = H_HARDWARE;
+ }
+
+ g_free(req_data);
+
+ return ret;
+}
+
+static target_ulong h_scm_async_flush(PowerPCCPU *cpu, SpaprMachineState *spapr,
+ target_ulong opcode, target_ulong *args)
+{
+ int ret;
+ uint32_t drc_index = args[0];
+ uint64_t continue_token = args[1];
+ SpaprDrc *drc = spapr_drc_by_index(drc_index);
+ PCDIMMDevice *dimm;
+ HostMemoryBackend *backend = NULL;
+ SCMAsyncFlushData *req_data = NULL;
+
+ if (!drc || !drc->dev ||
+ spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) {
+ return H_PARAMETER;
+ }
+
+ if (continue_token != 0) {
+ ret = spapr_drc_get_async_hcall_status(drc, continue_token);
+ if (ret == H_BUSY) {
+ args[0] = continue_token;
+ return H_LONG_BUSY_ORDER_1_SEC;
+ }
+
+ return ret;
+ }
+
+ dimm = PC_DIMM(drc->dev);
+ backend = MEMORY_BACKEND(dimm->hostmem);
+
+ req_data = g_malloc0(sizeof(SCMAsyncFlushData));
+ req_data->fd = memory_region_get_fd(&backend->mr);
+
+ continue_token = spapr_drc_get_new_async_hcall_token(drc);
+ if (!continue_token) {
+ g_free(req_data);
+ return H_P2;
+ }
+ req_data->token = continue_token;
+
+ spapr_drc_run_async_hcall(drc, continue_token, &flush_worker_cb, req_data);
+
+ ret = spapr_drc_get_async_hcall_status(drc, continue_token);
+ if (ret == H_BUSY) {
+ args[0] = req_data->token;
+ return ret;
+ }
+
+ return ret;
+}
+
static target_ulong h_scm_unbind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
@@ -486,6 +564,7 @@ static void spapr_scm_register_types(void)
spapr_register_hypercall(H_SCM_BIND_MEM, h_scm_bind_mem);
spapr_register_hypercall(H_SCM_UNBIND_MEM, h_scm_unbind_mem);
spapr_register_hypercall(H_SCM_UNBIND_ALL, h_scm_unbind_all);
+ spapr_register_hypercall(H_SCM_ASYNC_FLUSH, h_scm_async_flush);
}
type_init(spapr_scm_register_types)
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index c699842dd0..9e8795766e 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -51,6 +51,7 @@ OBJECT_DECLARE_TYPE(NVDIMMDevice, NVDIMMClass, NVDIMM)
#define NVDIMM_LABEL_SIZE_PROP "label-size"
#define NVDIMM_UUID_PROP "uuid"
#define NVDIMM_UNARMED_PROP "unarmed"
+#define NVDIMM_SYNC_DAX_PROP "sync-dax"
struct NVDIMMDevice {
/* private */
@@ -85,6 +86,15 @@ struct NVDIMMDevice {
*/
bool unarmed;
+ /*
+ * On PPC64,
+ * The 'off' value results in the async-flush-required property set
+ * in the device tree for pseries machines. When 'off', the guest
+ * initiates explicity flush requests to the backend device ensuring
+ * write persistence.
+ */
+ bool sync_dax;
+
/*
* The PPC64 - spapr requires each nvdimm device have a uuid.
*/
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 2e89e36cfb..6d7110b7dc 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -535,8 +535,9 @@ struct SpaprMachineState {
#define H_SCM_BIND_MEM 0x3EC
#define H_SCM_UNBIND_MEM 0x3F0
#define H_SCM_UNBIND_ALL 0x3FC
+#define H_SCM_ASYNC_FLUSH 0x4A0
-#define MAX_HCALL_OPCODE H_SCM_UNBIND_ALL
+#define MAX_HCALL_OPCODE H_SCM_ASYNC_FLUSH
/* The hcalls above are standardized in PAPR and implemented by pHyp
* as well.
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [RFC Qemu PATCH v2 2/2] spapr: nvdimm: Implement async flush hcalls
@ 2020-11-30 15:17 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2020-11-30 15:17 UTC (permalink / raw)
To: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc
Cc: linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
When the persistent memory beacked by a file, a cpu cache flush instruction
is not sufficient to ensure the stores are correctly flushed to the media.
The patch implements the async hcalls for flush operation on demand from the
guest kernel.
The device option sync-dax is by default off and enables explicit asynchronous
flush requests from guest. It can be disabled by setting syn-dax=on.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
hw/mem/nvdimm.c | 1 +
hw/ppc/spapr_nvdimm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++
include/hw/mem/nvdimm.h | 10 ++++++
include/hw/ppc/spapr.h | 3 +-
4 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 03c2201b56..37a4db0135 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -220,6 +220,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
static Property nvdimm_properties[] = {
DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
+ DEFINE_PROP_BOOL(NVDIMM_SYNC_DAX_PROP, NVDIMMDevice, sync_dax, false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5e..557e36aa98 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -22,6 +22,7 @@
* THE SOFTWARE.
*/
#include "qemu/osdep.h"
+#include "qemu/cutils.h"
#include "qapi/error.h"
#include "hw/ppc/spapr_drc.h"
#include "hw/ppc/spapr_nvdimm.h"
@@ -155,6 +156,11 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
"operating-system")));
_FDT(fdt_setprop(fdt, child_offset, "ibm,cache-flush-required", NULL, 0));
+ if (!nvdimm->sync_dax) {
+ _FDT(fdt_setprop(fdt, child_offset, "ibm,async-flush-required",
+ NULL, 0));
+ }
+
return child_offset;
}
@@ -370,6 +376,78 @@ static target_ulong h_scm_bind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
return H_SUCCESS;
}
+typedef struct SCMAsyncFlushData {
+ int fd;
+ uint64_t token;
+} SCMAsyncFlushData;
+
+static int flush_worker_cb(void *opaque)
+{
+ int ret = H_SUCCESS;
+ SCMAsyncFlushData *req_data = opaque;
+
+ /* flush raw backing image */
+ if (qemu_fdatasync(req_data->fd) < 0) {
+ error_report("papr_scm: Could not sync nvdimm to backend file: %s",
+ strerror(errno));
+ ret = H_HARDWARE;
+ }
+
+ g_free(req_data);
+
+ return ret;
+}
+
+static target_ulong h_scm_async_flush(PowerPCCPU *cpu, SpaprMachineState *spapr,
+ target_ulong opcode, target_ulong *args)
+{
+ int ret;
+ uint32_t drc_index = args[0];
+ uint64_t continue_token = args[1];
+ SpaprDrc *drc = spapr_drc_by_index(drc_index);
+ PCDIMMDevice *dimm;
+ HostMemoryBackend *backend = NULL;
+ SCMAsyncFlushData *req_data = NULL;
+
+ if (!drc || !drc->dev ||
+ spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) {
+ return H_PARAMETER;
+ }
+
+ if (continue_token != 0) {
+ ret = spapr_drc_get_async_hcall_status(drc, continue_token);
+ if (ret == H_BUSY) {
+ args[0] = continue_token;
+ return H_LONG_BUSY_ORDER_1_SEC;
+ }
+
+ return ret;
+ }
+
+ dimm = PC_DIMM(drc->dev);
+ backend = MEMORY_BACKEND(dimm->hostmem);
+
+ req_data = g_malloc0(sizeof(SCMAsyncFlushData));
+ req_data->fd = memory_region_get_fd(&backend->mr);
+
+ continue_token = spapr_drc_get_new_async_hcall_token(drc);
+ if (!continue_token) {
+ g_free(req_data);
+ return H_P2;
+ }
+ req_data->token = continue_token;
+
+ spapr_drc_run_async_hcall(drc, continue_token, &flush_worker_cb, req_data);
+
+ ret = spapr_drc_get_async_hcall_status(drc, continue_token);
+ if (ret == H_BUSY) {
+ args[0] = req_data->token;
+ return ret;
+ }
+
+ return ret;
+}
+
static target_ulong h_scm_unbind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
@@ -486,6 +564,7 @@ static void spapr_scm_register_types(void)
spapr_register_hypercall(H_SCM_BIND_MEM, h_scm_bind_mem);
spapr_register_hypercall(H_SCM_UNBIND_MEM, h_scm_unbind_mem);
spapr_register_hypercall(H_SCM_UNBIND_ALL, h_scm_unbind_all);
+ spapr_register_hypercall(H_SCM_ASYNC_FLUSH, h_scm_async_flush);
}
type_init(spapr_scm_register_types)
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index c699842dd0..9e8795766e 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -51,6 +51,7 @@ OBJECT_DECLARE_TYPE(NVDIMMDevice, NVDIMMClass, NVDIMM)
#define NVDIMM_LABEL_SIZE_PROP "label-size"
#define NVDIMM_UUID_PROP "uuid"
#define NVDIMM_UNARMED_PROP "unarmed"
+#define NVDIMM_SYNC_DAX_PROP "sync-dax"
struct NVDIMMDevice {
/* private */
@@ -85,6 +86,15 @@ struct NVDIMMDevice {
*/
bool unarmed;
+ /*
+ * On PPC64,
+ * The 'off' value results in the async-flush-required property set
+ * in the device tree for pseries machines. When 'off', the guest
+ * initiates explicity flush requests to the backend device ensuring
+ * write persistence.
+ */
+ bool sync_dax;
+
/*
* The PPC64 - spapr requires each nvdimm device have a uuid.
*/
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 2e89e36cfb..6d7110b7dc 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -535,8 +535,9 @@ struct SpaprMachineState {
#define H_SCM_BIND_MEM 0x3EC
#define H_SCM_UNBIND_MEM 0x3F0
#define H_SCM_UNBIND_ALL 0x3FC
+#define H_SCM_ASYNC_FLUSH 0x4A0
-#define MAX_HCALL_OPCODE H_SCM_UNBIND_ALL
+#define MAX_HCALL_OPCODE H_SCM_ASYNC_FLUSH
/* The hcalls above are standardized in PAPR and implemented by pHyp
* as well.
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [RFC Qemu PATCH v2 2/2] spapr: nvdimm: Implement async flush hcalls
@ 2020-11-30 15:17 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2020-11-30 15:17 UTC (permalink / raw)
To: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc
Cc: shivaprasadbhat, bharata, linux-nvdimm, linuxppc-dev, kvm-ppc,
aneesh.kumar
When the persistent memory beacked by a file, a cpu cache flush instruction
is not sufficient to ensure the stores are correctly flushed to the media.
The patch implements the async hcalls for flush operation on demand from the
guest kernel.
The device option sync-dax is by default off and enables explicit asynchronous
flush requests from guest. It can be disabled by setting syn-dax=on.
Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
hw/mem/nvdimm.c | 1 +
hw/ppc/spapr_nvdimm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++
include/hw/mem/nvdimm.h | 10 ++++++
include/hw/ppc/spapr.h | 3 +-
4 files changed, 92 insertions(+), 1 deletion(-)
diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
index 03c2201b56..37a4db0135 100644
--- a/hw/mem/nvdimm.c
+++ b/hw/mem/nvdimm.c
@@ -220,6 +220,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
static Property nvdimm_properties[] = {
DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
+ DEFINE_PROP_BOOL(NVDIMM_SYNC_DAX_PROP, NVDIMMDevice, sync_dax, false),
DEFINE_PROP_END_OF_LIST(),
};
diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
index a833a63b5e..557e36aa98 100644
--- a/hw/ppc/spapr_nvdimm.c
+++ b/hw/ppc/spapr_nvdimm.c
@@ -22,6 +22,7 @@
* THE SOFTWARE.
*/
#include "qemu/osdep.h"
+#include "qemu/cutils.h"
#include "qapi/error.h"
#include "hw/ppc/spapr_drc.h"
#include "hw/ppc/spapr_nvdimm.h"
@@ -155,6 +156,11 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
"operating-system")));
_FDT(fdt_setprop(fdt, child_offset, "ibm,cache-flush-required", NULL, 0));
+ if (!nvdimm->sync_dax) {
+ _FDT(fdt_setprop(fdt, child_offset, "ibm,async-flush-required",
+ NULL, 0));
+ }
+
return child_offset;
}
@@ -370,6 +376,78 @@ static target_ulong h_scm_bind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
return H_SUCCESS;
}
+typedef struct SCMAsyncFlushData {
+ int fd;
+ uint64_t token;
+} SCMAsyncFlushData;
+
+static int flush_worker_cb(void *opaque)
+{
+ int ret = H_SUCCESS;
+ SCMAsyncFlushData *req_data = opaque;
+
+ /* flush raw backing image */
+ if (qemu_fdatasync(req_data->fd) < 0) {
+ error_report("papr_scm: Could not sync nvdimm to backend file: %s",
+ strerror(errno));
+ ret = H_HARDWARE;
+ }
+
+ g_free(req_data);
+
+ return ret;
+}
+
+static target_ulong h_scm_async_flush(PowerPCCPU *cpu, SpaprMachineState *spapr,
+ target_ulong opcode, target_ulong *args)
+{
+ int ret;
+ uint32_t drc_index = args[0];
+ uint64_t continue_token = args[1];
+ SpaprDrc *drc = spapr_drc_by_index(drc_index);
+ PCDIMMDevice *dimm;
+ HostMemoryBackend *backend = NULL;
+ SCMAsyncFlushData *req_data = NULL;
+
+ if (!drc || !drc->dev ||
+ spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) {
+ return H_PARAMETER;
+ }
+
+ if (continue_token != 0) {
+ ret = spapr_drc_get_async_hcall_status(drc, continue_token);
+ if (ret = H_BUSY) {
+ args[0] = continue_token;
+ return H_LONG_BUSY_ORDER_1_SEC;
+ }
+
+ return ret;
+ }
+
+ dimm = PC_DIMM(drc->dev);
+ backend = MEMORY_BACKEND(dimm->hostmem);
+
+ req_data = g_malloc0(sizeof(SCMAsyncFlushData));
+ req_data->fd = memory_region_get_fd(&backend->mr);
+
+ continue_token = spapr_drc_get_new_async_hcall_token(drc);
+ if (!continue_token) {
+ g_free(req_data);
+ return H_P2;
+ }
+ req_data->token = continue_token;
+
+ spapr_drc_run_async_hcall(drc, continue_token, &flush_worker_cb, req_data);
+
+ ret = spapr_drc_get_async_hcall_status(drc, continue_token);
+ if (ret = H_BUSY) {
+ args[0] = req_data->token;
+ return ret;
+ }
+
+ return ret;
+}
+
static target_ulong h_scm_unbind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
target_ulong opcode, target_ulong *args)
{
@@ -486,6 +564,7 @@ static void spapr_scm_register_types(void)
spapr_register_hypercall(H_SCM_BIND_MEM, h_scm_bind_mem);
spapr_register_hypercall(H_SCM_UNBIND_MEM, h_scm_unbind_mem);
spapr_register_hypercall(H_SCM_UNBIND_ALL, h_scm_unbind_all);
+ spapr_register_hypercall(H_SCM_ASYNC_FLUSH, h_scm_async_flush);
}
type_init(spapr_scm_register_types)
diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
index c699842dd0..9e8795766e 100644
--- a/include/hw/mem/nvdimm.h
+++ b/include/hw/mem/nvdimm.h
@@ -51,6 +51,7 @@ OBJECT_DECLARE_TYPE(NVDIMMDevice, NVDIMMClass, NVDIMM)
#define NVDIMM_LABEL_SIZE_PROP "label-size"
#define NVDIMM_UUID_PROP "uuid"
#define NVDIMM_UNARMED_PROP "unarmed"
+#define NVDIMM_SYNC_DAX_PROP "sync-dax"
struct NVDIMMDevice {
/* private */
@@ -85,6 +86,15 @@ struct NVDIMMDevice {
*/
bool unarmed;
+ /*
+ * On PPC64,
+ * The 'off' value results in the async-flush-required property set
+ * in the device tree for pseries machines. When 'off', the guest
+ * initiates explicity flush requests to the backend device ensuring
+ * write persistence.
+ */
+ bool sync_dax;
+
/*
* The PPC64 - spapr requires each nvdimm device have a uuid.
*/
diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
index 2e89e36cfb..6d7110b7dc 100644
--- a/include/hw/ppc/spapr.h
+++ b/include/hw/ppc/spapr.h
@@ -535,8 +535,9 @@ struct SpaprMachineState {
#define H_SCM_BIND_MEM 0x3EC
#define H_SCM_UNBIND_MEM 0x3F0
#define H_SCM_UNBIND_ALL 0x3FC
+#define H_SCM_ASYNC_FLUSH 0x4A0
-#define MAX_HCALL_OPCODE H_SCM_UNBIND_ALL
+#define MAX_HCALL_OPCODE H_SCM_ASYNC_FLUSH
/* The hcalls above are standardized in PAPR and implemented by pHyp
* as well.
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
2020-11-30 15:16 ` Shivaprasad G Bhat
(?)
@ 2020-12-21 12:08 ` Greg Kurz
-1 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 12:08 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc,
linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
Hi Shiva,
On Mon, 30 Nov 2020 09:16:39 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> The patch adds support for async hcalls at the DRC level for the
> spapr devices. To be used by spapr-scm devices in the patch/es to follow.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
The overall idea looks good but I think you should consider using
a thread pool to implement it. See below.
> hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_drc.h | 25 +++++++
> 2 files changed, 174 insertions(+)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 77718cde1f..4ecd04f686 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -15,6 +15,7 @@
> #include "qapi/qmp/qnull.h"
> #include "cpu.h"
> #include "qemu/cutils.h"
> +#include "qemu/guest-random.h"
> #include "hw/ppc/spapr_drc.h"
> #include "qom/object.h"
> #include "migration/vmstate.h"
> @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> spapr_drc_release(drc);
> }
>
> +
> +/*
> + * @drc : device DRC targetting which the async hcalls to be made.
> + *
> + * All subsequent requests to run/query the status should use the
> + * unique token returned here.
> + */
> +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> +{
> + Error *err = NULL;
> + uint64_t token;
> + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> +
> + state = g_malloc0(sizeof(*state));
> + state->pending = true;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> +retry:
> + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> + error_report_err(err);
> + g_free(state);
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> + return 0;
> + }
> +
> + if (!token) /* Token should be non-zero */
> + goto retry;
> +
> + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> + if (tmp->continue_token == token) {
> + /* If the token already in use, get a new one */
> + goto retry;
> + }
> + }
> + }
> +
> + state->continue_token = token;
> + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> +
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +
> + return state->continue_token;
> +}
> +
> +static void *spapr_drc_async_hcall_runner(void *opaque)
> +{
> + int response = -1;
> + SpaprDrcDeviceAsyncHCallState *state = opaque;
> +
> + /*
> + * state is freed only after this thread finishes(after pthread_join()),
> + * don't worry about it becoming NULL.
> + */
> +
> + response = state->func(state->data);
> +
> + state->hcall_ret = response;
> + state->pending = 0;
> +
> + return NULL;
> +}
> +
> +/*
> + * @drc : device DRC targetting which the async hcalls to be made.
> + * token : The continue token to be used for tracking as recived from
> + * spapr_drc_get_new_async_hcall_token
> + * @func() : the worker function which needs to be executed asynchronously
> + * @data : data to be passed to the asynchronous function. Worker is supposed
> + * to free/cleanup the data that is passed here
It'd be cleaner to pass a completion callback and have free/cleanup handled there.
> + */
> +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> +{
> + SpaprDrcDeviceAsyncHCallState *state;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> + if (state->continue_token == token) {
> + state->func = func;
> + state->data = data;
> + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> + spapr_drc_async_hcall_runner, state,
> + QEMU_THREAD_JOINABLE);
qemu_thread_create() exits on failure, it shouldn't be called on
a guest triggerable path, eg. a buggy guest could call it up to
the point that pthread_create() returns EAGAIN.
Please use a thread pool (see thread_pool_submit_aio()). This takes care
of all the thread housekeeping for you in a safe way, and it provides a
completion callback API. The implementation could then be just about
having two lists: one for pending requests (fed here) and one for
completed requests (fed by the completion callback).
> + break;
> + }
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +}
> +
> +/*
> + * spapr_drc_finish_async_hcalls
> + * Waits for all pending async requests to complete
> + * thier execution and free the states
> + */
> +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> +{
> + SpaprDrcDeviceAsyncHCallState *state, *next;
> +
> + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> + return;
> + }
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> + qemu_thread_join(&state->thread);
With a thread-pool, you'd just need to aio_poll() until the pending list
is empty and then clear the completed list.
> + QLIST_REMOVE(state, node);
> + g_free(state);
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +}
> +
> +/*
> + * spapr_drc_get_async_hcall_status
> + * Fetches the status of the hcall worker and returns H_BUSY
> + * if the worker is still running.
> + */
> +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> +{
> + int ret = H_PARAMETER;
> + SpaprDrcDeviceAsyncHCallState *state, *node;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> + if (state->continue_token == token) {
> + if (state->pending) {
> + ret = H_BUSY;
> + break;
> + } else {
> + ret = state->hcall_ret;
> + qemu_thread_join(&state->thread);
Like for qemu_thread_create(), the guest shouldn't be responsible for
thread housekeeping. Getting the hcall status should just be about
finding the token in the pending or completed lists.
> + QLIST_REMOVE(state, node);
> + g_free(state);
> + break;
> + }
> + }
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +
> + return ret;
> +}
> +
> void spapr_drc_reset(SpaprDrc *drc)
> {
> SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> drc->ccs_offset = -1;
> drc->ccs_depth = -1;
> }
> + spapr_drc_finish_async_hcalls(drc);
> }
>
> static bool spapr_drc_unplug_requested_needed(void *opaque)
> @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> drc->owner = owner;
> prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> spapr_drc_index(drc));
> +
Unrelated change.
> object_property_add_child(owner, prop_name, OBJECT(drc));
> object_unref(OBJECT(drc));
> qdev_realize(DEVICE(drc), NULL, NULL);
> @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> object_property_add(obj, "fdt", "struct", prop_get_fdt,
> NULL, NULL, NULL);
> drc->state = drck->empty_state;
> +
> + qemu_mutex_init(&drc->async_hcall_states_lock);
> + QLIST_INIT(&drc->async_hcall_states);
> +
Empty line not needed.
> }
>
> static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 165b281496..77f6e4386c 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -18,6 +18,7 @@
> #include "sysemu/runstate.h"
> #include "hw/qdev-core.h"
> #include "qapi/error.h"
> +#include "block/thread-pool.h"
>
> #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> @@ -168,6 +169,21 @@ typedef enum {
> SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> } SpaprDrcState;
>
> +typedef struct SpaprDrc SpaprDrc;
> +
> +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> +typedef struct SpaprDrcDeviceAsyncHCallState {
> + uint64_t continue_token;
> + bool pending;
> +
> + int hcall_ret;
> + SpaprDrcAsyncHcallWorkerFunc *func;
> + void *data;
> +
> + QemuThread thread;
> +
> + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> +} SpaprDrcDeviceAsyncHCallState;
> typedef struct SpaprDrc {
> /*< private >*/
> DeviceState parent;
> @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> int ccs_offset;
> int ccs_depth;
>
> + /* async hcall states */
> + QemuMutex async_hcall_states_lock;
> + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> +
> /* device pointer, via link property */
> DeviceState *dev;
> bool unplug_requested;
> @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> /* Returns true if a hot plug/unplug request is pending */
> bool spapr_drc_transient(SpaprDrc *drc);
>
> +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> + SpaprDrcAsyncHcallWorkerFunc, void *data);
> +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> +
> static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> {
> return drc->unplug_requested;
>
>
>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2020-12-21 12:08 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 12:08 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev, david
Hi Shiva,
On Mon, 30 Nov 2020 09:16:39 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> The patch adds support for async hcalls at the DRC level for the
> spapr devices. To be used by spapr-scm devices in the patch/es to follow.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
The overall idea looks good but I think you should consider using
a thread pool to implement it. See below.
> hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_drc.h | 25 +++++++
> 2 files changed, 174 insertions(+)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 77718cde1f..4ecd04f686 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -15,6 +15,7 @@
> #include "qapi/qmp/qnull.h"
> #include "cpu.h"
> #include "qemu/cutils.h"
> +#include "qemu/guest-random.h"
> #include "hw/ppc/spapr_drc.h"
> #include "qom/object.h"
> #include "migration/vmstate.h"
> @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> spapr_drc_release(drc);
> }
>
> +
> +/*
> + * @drc : device DRC targetting which the async hcalls to be made.
> + *
> + * All subsequent requests to run/query the status should use the
> + * unique token returned here.
> + */
> +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> +{
> + Error *err = NULL;
> + uint64_t token;
> + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> +
> + state = g_malloc0(sizeof(*state));
> + state->pending = true;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> +retry:
> + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> + error_report_err(err);
> + g_free(state);
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> + return 0;
> + }
> +
> + if (!token) /* Token should be non-zero */
> + goto retry;
> +
> + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> + if (tmp->continue_token == token) {
> + /* If the token already in use, get a new one */
> + goto retry;
> + }
> + }
> + }
> +
> + state->continue_token = token;
> + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> +
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +
> + return state->continue_token;
> +}
> +
> +static void *spapr_drc_async_hcall_runner(void *opaque)
> +{
> + int response = -1;
> + SpaprDrcDeviceAsyncHCallState *state = opaque;
> +
> + /*
> + * state is freed only after this thread finishes(after pthread_join()),
> + * don't worry about it becoming NULL.
> + */
> +
> + response = state->func(state->data);
> +
> + state->hcall_ret = response;
> + state->pending = 0;
> +
> + return NULL;
> +}
> +
> +/*
> + * @drc : device DRC targetting which the async hcalls to be made.
> + * token : The continue token to be used for tracking as recived from
> + * spapr_drc_get_new_async_hcall_token
> + * @func() : the worker function which needs to be executed asynchronously
> + * @data : data to be passed to the asynchronous function. Worker is supposed
> + * to free/cleanup the data that is passed here
It'd be cleaner to pass a completion callback and have free/cleanup handled there.
> + */
> +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> +{
> + SpaprDrcDeviceAsyncHCallState *state;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> + if (state->continue_token == token) {
> + state->func = func;
> + state->data = data;
> + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> + spapr_drc_async_hcall_runner, state,
> + QEMU_THREAD_JOINABLE);
qemu_thread_create() exits on failure, it shouldn't be called on
a guest triggerable path, eg. a buggy guest could call it up to
the point that pthread_create() returns EAGAIN.
Please use a thread pool (see thread_pool_submit_aio()). This takes care
of all the thread housekeeping for you in a safe way, and it provides a
completion callback API. The implementation could then be just about
having two lists: one for pending requests (fed here) and one for
completed requests (fed by the completion callback).
> + break;
> + }
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +}
> +
> +/*
> + * spapr_drc_finish_async_hcalls
> + * Waits for all pending async requests to complete
> + * thier execution and free the states
> + */
> +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> +{
> + SpaprDrcDeviceAsyncHCallState *state, *next;
> +
> + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> + return;
> + }
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> + qemu_thread_join(&state->thread);
With a thread-pool, you'd just need to aio_poll() until the pending list
is empty and then clear the completed list.
> + QLIST_REMOVE(state, node);
> + g_free(state);
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +}
> +
> +/*
> + * spapr_drc_get_async_hcall_status
> + * Fetches the status of the hcall worker and returns H_BUSY
> + * if the worker is still running.
> + */
> +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> +{
> + int ret = H_PARAMETER;
> + SpaprDrcDeviceAsyncHCallState *state, *node;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> + if (state->continue_token == token) {
> + if (state->pending) {
> + ret = H_BUSY;
> + break;
> + } else {
> + ret = state->hcall_ret;
> + qemu_thread_join(&state->thread);
Like for qemu_thread_create(), the guest shouldn't be responsible for
thread housekeeping. Getting the hcall status should just be about
finding the token in the pending or completed lists.
> + QLIST_REMOVE(state, node);
> + g_free(state);
> + break;
> + }
> + }
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +
> + return ret;
> +}
> +
> void spapr_drc_reset(SpaprDrc *drc)
> {
> SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> drc->ccs_offset = -1;
> drc->ccs_depth = -1;
> }
> + spapr_drc_finish_async_hcalls(drc);
> }
>
> static bool spapr_drc_unplug_requested_needed(void *opaque)
> @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> drc->owner = owner;
> prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> spapr_drc_index(drc));
> +
Unrelated change.
> object_property_add_child(owner, prop_name, OBJECT(drc));
> object_unref(OBJECT(drc));
> qdev_realize(DEVICE(drc), NULL, NULL);
> @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> object_property_add(obj, "fdt", "struct", prop_get_fdt,
> NULL, NULL, NULL);
> drc->state = drck->empty_state;
> +
> + qemu_mutex_init(&drc->async_hcall_states_lock);
> + QLIST_INIT(&drc->async_hcall_states);
> +
Empty line not needed.
> }
>
> static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 165b281496..77f6e4386c 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -18,6 +18,7 @@
> #include "sysemu/runstate.h"
> #include "hw/qdev-core.h"
> #include "qapi/error.h"
> +#include "block/thread-pool.h"
>
> #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> @@ -168,6 +169,21 @@ typedef enum {
> SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> } SpaprDrcState;
>
> +typedef struct SpaprDrc SpaprDrc;
> +
> +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> +typedef struct SpaprDrcDeviceAsyncHCallState {
> + uint64_t continue_token;
> + bool pending;
> +
> + int hcall_ret;
> + SpaprDrcAsyncHcallWorkerFunc *func;
> + void *data;
> +
> + QemuThread thread;
> +
> + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> +} SpaprDrcDeviceAsyncHCallState;
> typedef struct SpaprDrc {
> /*< private >*/
> DeviceState parent;
> @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> int ccs_offset;
> int ccs_depth;
>
> + /* async hcall states */
> + QemuMutex async_hcall_states_lock;
> + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> +
> /* device pointer, via link property */
> DeviceState *dev;
> bool unplug_requested;
> @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> /* Returns true if a hot plug/unplug request is pending */
> bool spapr_drc_transient(SpaprDrc *drc);
>
> +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> + SpaprDrcAsyncHcallWorkerFunc, void *data);
> +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> +
> static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> {
> return drc->unplug_requested;
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2020-12-21 12:08 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 12:08 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc,
linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
Hi Shiva,
On Mon, 30 Nov 2020 09:16:39 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> The patch adds support for async hcalls at the DRC level for the
> spapr devices. To be used by spapr-scm devices in the patch/es to follow.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
The overall idea looks good but I think you should consider using
a thread pool to implement it. See below.
> hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> include/hw/ppc/spapr_drc.h | 25 +++++++
> 2 files changed, 174 insertions(+)
>
> diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> index 77718cde1f..4ecd04f686 100644
> --- a/hw/ppc/spapr_drc.c
> +++ b/hw/ppc/spapr_drc.c
> @@ -15,6 +15,7 @@
> #include "qapi/qmp/qnull.h"
> #include "cpu.h"
> #include "qemu/cutils.h"
> +#include "qemu/guest-random.h"
> #include "hw/ppc/spapr_drc.h"
> #include "qom/object.h"
> #include "migration/vmstate.h"
> @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> spapr_drc_release(drc);
> }
>
> +
> +/*
> + * @drc : device DRC targetting which the async hcalls to be made.
> + *
> + * All subsequent requests to run/query the status should use the
> + * unique token returned here.
> + */
> +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> +{
> + Error *err = NULL;
> + uint64_t token;
> + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> +
> + state = g_malloc0(sizeof(*state));
> + state->pending = true;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> +retry:
> + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> + error_report_err(err);
> + g_free(state);
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> + return 0;
> + }
> +
> + if (!token) /* Token should be non-zero */
> + goto retry;
> +
> + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> + if (tmp->continue_token = token) {
> + /* If the token already in use, get a new one */
> + goto retry;
> + }
> + }
> + }
> +
> + state->continue_token = token;
> + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> +
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +
> + return state->continue_token;
> +}
> +
> +static void *spapr_drc_async_hcall_runner(void *opaque)
> +{
> + int response = -1;
> + SpaprDrcDeviceAsyncHCallState *state = opaque;
> +
> + /*
> + * state is freed only after this thread finishes(after pthread_join()),
> + * don't worry about it becoming NULL.
> + */
> +
> + response = state->func(state->data);
> +
> + state->hcall_ret = response;
> + state->pending = 0;
> +
> + return NULL;
> +}
> +
> +/*
> + * @drc : device DRC targetting which the async hcalls to be made.
> + * token : The continue token to be used for tracking as recived from
> + * spapr_drc_get_new_async_hcall_token
> + * @func() : the worker function which needs to be executed asynchronously
> + * @data : data to be passed to the asynchronous function. Worker is supposed
> + * to free/cleanup the data that is passed here
It'd be cleaner to pass a completion callback and have free/cleanup handled there.
> + */
> +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> +{
> + SpaprDrcDeviceAsyncHCallState *state;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> + if (state->continue_token = token) {
> + state->func = func;
> + state->data = data;
> + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> + spapr_drc_async_hcall_runner, state,
> + QEMU_THREAD_JOINABLE);
qemu_thread_create() exits on failure, it shouldn't be called on
a guest triggerable path, eg. a buggy guest could call it up to
the point that pthread_create() returns EAGAIN.
Please use a thread pool (see thread_pool_submit_aio()). This takes care
of all the thread housekeeping for you in a safe way, and it provides a
completion callback API. The implementation could then be just about
having two lists: one for pending requests (fed here) and one for
completed requests (fed by the completion callback).
> + break;
> + }
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +}
> +
> +/*
> + * spapr_drc_finish_async_hcalls
> + * Waits for all pending async requests to complete
> + * thier execution and free the states
> + */
> +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> +{
> + SpaprDrcDeviceAsyncHCallState *state, *next;
> +
> + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> + return;
> + }
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> + qemu_thread_join(&state->thread);
With a thread-pool, you'd just need to aio_poll() until the pending list
is empty and then clear the completed list.
> + QLIST_REMOVE(state, node);
> + g_free(state);
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +}
> +
> +/*
> + * spapr_drc_get_async_hcall_status
> + * Fetches the status of the hcall worker and returns H_BUSY
> + * if the worker is still running.
> + */
> +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> +{
> + int ret = H_PARAMETER;
> + SpaprDrcDeviceAsyncHCallState *state, *node;
> +
> + qemu_mutex_lock(&drc->async_hcall_states_lock);
> + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> + if (state->continue_token = token) {
> + if (state->pending) {
> + ret = H_BUSY;
> + break;
> + } else {
> + ret = state->hcall_ret;
> + qemu_thread_join(&state->thread);
Like for qemu_thread_create(), the guest shouldn't be responsible for
thread housekeeping. Getting the hcall status should just be about
finding the token in the pending or completed lists.
> + QLIST_REMOVE(state, node);
> + g_free(state);
> + break;
> + }
> + }
> + }
> + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> +
> + return ret;
> +}
> +
> void spapr_drc_reset(SpaprDrc *drc)
> {
> SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> drc->ccs_offset = -1;
> drc->ccs_depth = -1;
> }
> + spapr_drc_finish_async_hcalls(drc);
> }
>
> static bool spapr_drc_unplug_requested_needed(void *opaque)
> @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> drc->owner = owner;
> prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> spapr_drc_index(drc));
> +
Unrelated change.
> object_property_add_child(owner, prop_name, OBJECT(drc));
> object_unref(OBJECT(drc));
> qdev_realize(DEVICE(drc), NULL, NULL);
> @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> object_property_add(obj, "fdt", "struct", prop_get_fdt,
> NULL, NULL, NULL);
> drc->state = drck->empty_state;
> +
> + qemu_mutex_init(&drc->async_hcall_states_lock);
> + QLIST_INIT(&drc->async_hcall_states);
> +
Empty line not needed.
> }
>
> static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> index 165b281496..77f6e4386c 100644
> --- a/include/hw/ppc/spapr_drc.h
> +++ b/include/hw/ppc/spapr_drc.h
> @@ -18,6 +18,7 @@
> #include "sysemu/runstate.h"
> #include "hw/qdev-core.h"
> #include "qapi/error.h"
> +#include "block/thread-pool.h"
>
> #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> @@ -168,6 +169,21 @@ typedef enum {
> SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> } SpaprDrcState;
>
> +typedef struct SpaprDrc SpaprDrc;
> +
> +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> +typedef struct SpaprDrcDeviceAsyncHCallState {
> + uint64_t continue_token;
> + bool pending;
> +
> + int hcall_ret;
> + SpaprDrcAsyncHcallWorkerFunc *func;
> + void *data;
> +
> + QemuThread thread;
> +
> + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> +} SpaprDrcDeviceAsyncHCallState;
> typedef struct SpaprDrc {
> /*< private >*/
> DeviceState parent;
> @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> int ccs_offset;
> int ccs_depth;
>
> + /* async hcall states */
> + QemuMutex async_hcall_states_lock;
> + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> +
> /* device pointer, via link property */
> DeviceState *dev;
> bool unplug_requested;
> @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> /* Returns true if a hot plug/unplug request is pending */
> bool spapr_drc_transient(SpaprDrc *drc);
>
> +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> + SpaprDrcAsyncHcallWorkerFunc, void *data);
> +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> +
> static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> {
> return drc->unplug_requested;
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 2/2] spapr: nvdimm: Implement async flush hcalls
2020-11-30 15:17 ` Shivaprasad G Bhat
(?)
@ 2020-12-21 13:07 ` Greg Kurz
-1 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 13:07 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc,
linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
On Mon, 30 Nov 2020 09:17:24 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> When the persistent memory beacked by a file, a cpu cache flush instruction
> is not sufficient to ensure the stores are correctly flushed to the media.
>
> The patch implements the async hcalls for flush operation on demand from the
> guest kernel.
>
> The device option sync-dax is by default off and enables explicit asynchronous
> flush requests from guest. It can be disabled by setting syn-dax=on.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
> hw/mem/nvdimm.c | 1 +
> hw/ppc/spapr_nvdimm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/mem/nvdimm.h | 10 ++++++
> include/hw/ppc/spapr.h | 3 +-
> 4 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 03c2201b56..37a4db0135 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -220,6 +220,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
>
> static Property nvdimm_properties[] = {
> DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
> + DEFINE_PROP_BOOL(NVDIMM_SYNC_DAX_PROP, NVDIMMDevice, sync_dax, false),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5e..557e36aa98 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -22,6 +22,7 @@
> * THE SOFTWARE.
> */
> #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> #include "qapi/error.h"
> #include "hw/ppc/spapr_drc.h"
> #include "hw/ppc/spapr_nvdimm.h"
> @@ -155,6 +156,11 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
> "operating-system")));
> _FDT(fdt_setprop(fdt, child_offset, "ibm,cache-flush-required", NULL, 0));
>
> + if (!nvdimm->sync_dax) {
So this is done unconditionally for all machine types. This means that a
guest started on a newer QEMU cannot be migrated to an older QEMU. This
is annoying because people legitimately expect an existing machine type
to be migratable to any QEMU version that supports it.
This means that something like the following should be added in hw_compat_5_2[]
to fix the property for pre-6.0 machine types:
{ "nvdimm", "sync-dax", "on" },
> + _FDT(fdt_setprop(fdt, child_offset, "ibm,async-flush-required",
> + NULL, 0));
> + }
> +
> return child_offset;
> }
>
> @@ -370,6 +376,78 @@ static target_ulong h_scm_bind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
> return H_SUCCESS;
> }
>
> +typedef struct SCMAsyncFlushData {
> + int fd;
> + uint64_t token;
> +} SCMAsyncFlushData;
> +
> +static int flush_worker_cb(void *opaque)
> +{
> + int ret = H_SUCCESS;
> + SCMAsyncFlushData *req_data = opaque;
> +
> + /* flush raw backing image */
> + if (qemu_fdatasync(req_data->fd) < 0) {
> + error_report("papr_scm: Could not sync nvdimm to backend file: %s",
> + strerror(errno));
> + ret = H_HARDWARE;
> + }
> +
> + g_free(req_data);
> +
> + return ret;
> +}
> +
> +static target_ulong h_scm_async_flush(PowerPCCPU *cpu, SpaprMachineState *spapr,
> + target_ulong opcode, target_ulong *args)
> +{
> + int ret;
> + uint32_t drc_index = args[0];
> + uint64_t continue_token = args[1];
> + SpaprDrc *drc = spapr_drc_by_index(drc_index);
> + PCDIMMDevice *dimm;
> + HostMemoryBackend *backend = NULL;
> + SCMAsyncFlushData *req_data = NULL;
> +
> + if (!drc || !drc->dev ||
> + spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) {
> + return H_PARAMETER;
> + }
> +
> + if (continue_token != 0) {
> + ret = spapr_drc_get_async_hcall_status(drc, continue_token);
> + if (ret == H_BUSY) {
> + args[0] = continue_token;
> + return H_LONG_BUSY_ORDER_1_SEC;
> + }
> +
> + return ret;
> + }
> +
> + dimm = PC_DIMM(drc->dev);
> + backend = MEMORY_BACKEND(dimm->hostmem);
> +
> + req_data = g_malloc0(sizeof(SCMAsyncFlushData));
> + req_data->fd = memory_region_get_fd(&backend->mr);
> +
> + continue_token = spapr_drc_get_new_async_hcall_token(drc);
> + if (!continue_token) {
> + g_free(req_data);
> + return H_P2;
> + }
> + req_data->token = continue_token;
> +
> + spapr_drc_run_async_hcall(drc, continue_token, &flush_worker_cb, req_data);
> +
> + ret = spapr_drc_get_async_hcall_status(drc, continue_token);
> + if (ret == H_BUSY) {
> + args[0] = req_data->token;
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> static target_ulong h_scm_unbind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> @@ -486,6 +564,7 @@ static void spapr_scm_register_types(void)
> spapr_register_hypercall(H_SCM_BIND_MEM, h_scm_bind_mem);
> spapr_register_hypercall(H_SCM_UNBIND_MEM, h_scm_unbind_mem);
> spapr_register_hypercall(H_SCM_UNBIND_ALL, h_scm_unbind_all);
> + spapr_register_hypercall(H_SCM_ASYNC_FLUSH, h_scm_async_flush);
> }
>
> type_init(spapr_scm_register_types)
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index c699842dd0..9e8795766e 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -51,6 +51,7 @@ OBJECT_DECLARE_TYPE(NVDIMMDevice, NVDIMMClass, NVDIMM)
> #define NVDIMM_LABEL_SIZE_PROP "label-size"
> #define NVDIMM_UUID_PROP "uuid"
> #define NVDIMM_UNARMED_PROP "unarmed"
> +#define NVDIMM_SYNC_DAX_PROP "sync-dax"
>
> struct NVDIMMDevice {
> /* private */
> @@ -85,6 +86,15 @@ struct NVDIMMDevice {
> */
> bool unarmed;
>
> + /*
> + * On PPC64,
> + * The 'off' value results in the async-flush-required property set
> + * in the device tree for pseries machines. When 'off', the guest
> + * initiates explicity flush requests to the backend device ensuring
> + * write persistence.
> + */
> + bool sync_dax;
> +
> /*
> * The PPC64 - spapr requires each nvdimm device have a uuid.
> */
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2e89e36cfb..6d7110b7dc 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -535,8 +535,9 @@ struct SpaprMachineState {
> #define H_SCM_BIND_MEM 0x3EC
> #define H_SCM_UNBIND_MEM 0x3F0
> #define H_SCM_UNBIND_ALL 0x3FC
> +#define H_SCM_ASYNC_FLUSH 0x4A0
>
> -#define MAX_HCALL_OPCODE H_SCM_UNBIND_ALL
> +#define MAX_HCALL_OPCODE H_SCM_ASYNC_FLUSH
>
> /* The hcalls above are standardized in PAPR and implemented by pHyp
> * as well.
>
>
>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 2/2] spapr: nvdimm: Implement async flush hcalls
@ 2020-12-21 13:07 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 13:07 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev, david
On Mon, 30 Nov 2020 09:17:24 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> When the persistent memory beacked by a file, a cpu cache flush instruction
> is not sufficient to ensure the stores are correctly flushed to the media.
>
> The patch implements the async hcalls for flush operation on demand from the
> guest kernel.
>
> The device option sync-dax is by default off and enables explicit asynchronous
> flush requests from guest. It can be disabled by setting syn-dax=on.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
> hw/mem/nvdimm.c | 1 +
> hw/ppc/spapr_nvdimm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/mem/nvdimm.h | 10 ++++++
> include/hw/ppc/spapr.h | 3 +-
> 4 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 03c2201b56..37a4db0135 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -220,6 +220,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
>
> static Property nvdimm_properties[] = {
> DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
> + DEFINE_PROP_BOOL(NVDIMM_SYNC_DAX_PROP, NVDIMMDevice, sync_dax, false),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5e..557e36aa98 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -22,6 +22,7 @@
> * THE SOFTWARE.
> */
> #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> #include "qapi/error.h"
> #include "hw/ppc/spapr_drc.h"
> #include "hw/ppc/spapr_nvdimm.h"
> @@ -155,6 +156,11 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
> "operating-system")));
> _FDT(fdt_setprop(fdt, child_offset, "ibm,cache-flush-required", NULL, 0));
>
> + if (!nvdimm->sync_dax) {
So this is done unconditionally for all machine types. This means that a
guest started on a newer QEMU cannot be migrated to an older QEMU. This
is annoying because people legitimately expect an existing machine type
to be migratable to any QEMU version that supports it.
This means that something like the following should be added in hw_compat_5_2[]
to fix the property for pre-6.0 machine types:
{ "nvdimm", "sync-dax", "on" },
> + _FDT(fdt_setprop(fdt, child_offset, "ibm,async-flush-required",
> + NULL, 0));
> + }
> +
> return child_offset;
> }
>
> @@ -370,6 +376,78 @@ static target_ulong h_scm_bind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
> return H_SUCCESS;
> }
>
> +typedef struct SCMAsyncFlushData {
> + int fd;
> + uint64_t token;
> +} SCMAsyncFlushData;
> +
> +static int flush_worker_cb(void *opaque)
> +{
> + int ret = H_SUCCESS;
> + SCMAsyncFlushData *req_data = opaque;
> +
> + /* flush raw backing image */
> + if (qemu_fdatasync(req_data->fd) < 0) {
> + error_report("papr_scm: Could not sync nvdimm to backend file: %s",
> + strerror(errno));
> + ret = H_HARDWARE;
> + }
> +
> + g_free(req_data);
> +
> + return ret;
> +}
> +
> +static target_ulong h_scm_async_flush(PowerPCCPU *cpu, SpaprMachineState *spapr,
> + target_ulong opcode, target_ulong *args)
> +{
> + int ret;
> + uint32_t drc_index = args[0];
> + uint64_t continue_token = args[1];
> + SpaprDrc *drc = spapr_drc_by_index(drc_index);
> + PCDIMMDevice *dimm;
> + HostMemoryBackend *backend = NULL;
> + SCMAsyncFlushData *req_data = NULL;
> +
> + if (!drc || !drc->dev ||
> + spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) {
> + return H_PARAMETER;
> + }
> +
> + if (continue_token != 0) {
> + ret = spapr_drc_get_async_hcall_status(drc, continue_token);
> + if (ret == H_BUSY) {
> + args[0] = continue_token;
> + return H_LONG_BUSY_ORDER_1_SEC;
> + }
> +
> + return ret;
> + }
> +
> + dimm = PC_DIMM(drc->dev);
> + backend = MEMORY_BACKEND(dimm->hostmem);
> +
> + req_data = g_malloc0(sizeof(SCMAsyncFlushData));
> + req_data->fd = memory_region_get_fd(&backend->mr);
> +
> + continue_token = spapr_drc_get_new_async_hcall_token(drc);
> + if (!continue_token) {
> + g_free(req_data);
> + return H_P2;
> + }
> + req_data->token = continue_token;
> +
> + spapr_drc_run_async_hcall(drc, continue_token, &flush_worker_cb, req_data);
> +
> + ret = spapr_drc_get_async_hcall_status(drc, continue_token);
> + if (ret == H_BUSY) {
> + args[0] = req_data->token;
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> static target_ulong h_scm_unbind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> @@ -486,6 +564,7 @@ static void spapr_scm_register_types(void)
> spapr_register_hypercall(H_SCM_BIND_MEM, h_scm_bind_mem);
> spapr_register_hypercall(H_SCM_UNBIND_MEM, h_scm_unbind_mem);
> spapr_register_hypercall(H_SCM_UNBIND_ALL, h_scm_unbind_all);
> + spapr_register_hypercall(H_SCM_ASYNC_FLUSH, h_scm_async_flush);
> }
>
> type_init(spapr_scm_register_types)
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index c699842dd0..9e8795766e 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -51,6 +51,7 @@ OBJECT_DECLARE_TYPE(NVDIMMDevice, NVDIMMClass, NVDIMM)
> #define NVDIMM_LABEL_SIZE_PROP "label-size"
> #define NVDIMM_UUID_PROP "uuid"
> #define NVDIMM_UNARMED_PROP "unarmed"
> +#define NVDIMM_SYNC_DAX_PROP "sync-dax"
>
> struct NVDIMMDevice {
> /* private */
> @@ -85,6 +86,15 @@ struct NVDIMMDevice {
> */
> bool unarmed;
>
> + /*
> + * On PPC64,
> + * The 'off' value results in the async-flush-required property set
> + * in the device tree for pseries machines. When 'off', the guest
> + * initiates explicity flush requests to the backend device ensuring
> + * write persistence.
> + */
> + bool sync_dax;
> +
> /*
> * The PPC64 - spapr requires each nvdimm device have a uuid.
> */
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2e89e36cfb..6d7110b7dc 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -535,8 +535,9 @@ struct SpaprMachineState {
> #define H_SCM_BIND_MEM 0x3EC
> #define H_SCM_UNBIND_MEM 0x3F0
> #define H_SCM_UNBIND_ALL 0x3FC
> +#define H_SCM_ASYNC_FLUSH 0x4A0
>
> -#define MAX_HCALL_OPCODE H_SCM_UNBIND_ALL
> +#define MAX_HCALL_OPCODE H_SCM_ASYNC_FLUSH
>
> /* The hcalls above are standardized in PAPR and implemented by pHyp
> * as well.
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 2/2] spapr: nvdimm: Implement async flush hcalls
@ 2020-12-21 13:07 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 13:07 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc,
linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
On Mon, 30 Nov 2020 09:17:24 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> When the persistent memory beacked by a file, a cpu cache flush instruction
> is not sufficient to ensure the stores are correctly flushed to the media.
>
> The patch implements the async hcalls for flush operation on demand from the
> guest kernel.
>
> The device option sync-dax is by default off and enables explicit asynchronous
> flush requests from guest. It can be disabled by setting syn-dax=on.
>
> Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> ---
> hw/mem/nvdimm.c | 1 +
> hw/ppc/spapr_nvdimm.c | 79 +++++++++++++++++++++++++++++++++++++++++++++++
> include/hw/mem/nvdimm.h | 10 ++++++
> include/hw/ppc/spapr.h | 3 +-
> 4 files changed, 92 insertions(+), 1 deletion(-)
>
> diff --git a/hw/mem/nvdimm.c b/hw/mem/nvdimm.c
> index 03c2201b56..37a4db0135 100644
> --- a/hw/mem/nvdimm.c
> +++ b/hw/mem/nvdimm.c
> @@ -220,6 +220,7 @@ static void nvdimm_write_label_data(NVDIMMDevice *nvdimm, const void *buf,
>
> static Property nvdimm_properties[] = {
> DEFINE_PROP_BOOL(NVDIMM_UNARMED_PROP, NVDIMMDevice, unarmed, false),
> + DEFINE_PROP_BOOL(NVDIMM_SYNC_DAX_PROP, NVDIMMDevice, sync_dax, false),
> DEFINE_PROP_END_OF_LIST(),
> };
>
> diff --git a/hw/ppc/spapr_nvdimm.c b/hw/ppc/spapr_nvdimm.c
> index a833a63b5e..557e36aa98 100644
> --- a/hw/ppc/spapr_nvdimm.c
> +++ b/hw/ppc/spapr_nvdimm.c
> @@ -22,6 +22,7 @@
> * THE SOFTWARE.
> */
> #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
> #include "qapi/error.h"
> #include "hw/ppc/spapr_drc.h"
> #include "hw/ppc/spapr_nvdimm.h"
> @@ -155,6 +156,11 @@ static int spapr_dt_nvdimm(SpaprMachineState *spapr, void *fdt,
> "operating-system")));
> _FDT(fdt_setprop(fdt, child_offset, "ibm,cache-flush-required", NULL, 0));
>
> + if (!nvdimm->sync_dax) {
So this is done unconditionally for all machine types. This means that a
guest started on a newer QEMU cannot be migrated to an older QEMU. This
is annoying because people legitimately expect an existing machine type
to be migratable to any QEMU version that supports it.
This means that something like the following should be added in hw_compat_5_2[]
to fix the property for pre-6.0 machine types:
{ "nvdimm", "sync-dax", "on" },
> + _FDT(fdt_setprop(fdt, child_offset, "ibm,async-flush-required",
> + NULL, 0));
> + }
> +
> return child_offset;
> }
>
> @@ -370,6 +376,78 @@ static target_ulong h_scm_bind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
> return H_SUCCESS;
> }
>
> +typedef struct SCMAsyncFlushData {
> + int fd;
> + uint64_t token;
> +} SCMAsyncFlushData;
> +
> +static int flush_worker_cb(void *opaque)
> +{
> + int ret = H_SUCCESS;
> + SCMAsyncFlushData *req_data = opaque;
> +
> + /* flush raw backing image */
> + if (qemu_fdatasync(req_data->fd) < 0) {
> + error_report("papr_scm: Could not sync nvdimm to backend file: %s",
> + strerror(errno));
> + ret = H_HARDWARE;
> + }
> +
> + g_free(req_data);
> +
> + return ret;
> +}
> +
> +static target_ulong h_scm_async_flush(PowerPCCPU *cpu, SpaprMachineState *spapr,
> + target_ulong opcode, target_ulong *args)
> +{
> + int ret;
> + uint32_t drc_index = args[0];
> + uint64_t continue_token = args[1];
> + SpaprDrc *drc = spapr_drc_by_index(drc_index);
> + PCDIMMDevice *dimm;
> + HostMemoryBackend *backend = NULL;
> + SCMAsyncFlushData *req_data = NULL;
> +
> + if (!drc || !drc->dev ||
> + spapr_drc_type(drc) != SPAPR_DR_CONNECTOR_TYPE_PMEM) {
> + return H_PARAMETER;
> + }
> +
> + if (continue_token != 0) {
> + ret = spapr_drc_get_async_hcall_status(drc, continue_token);
> + if (ret = H_BUSY) {
> + args[0] = continue_token;
> + return H_LONG_BUSY_ORDER_1_SEC;
> + }
> +
> + return ret;
> + }
> +
> + dimm = PC_DIMM(drc->dev);
> + backend = MEMORY_BACKEND(dimm->hostmem);
> +
> + req_data = g_malloc0(sizeof(SCMAsyncFlushData));
> + req_data->fd = memory_region_get_fd(&backend->mr);
> +
> + continue_token = spapr_drc_get_new_async_hcall_token(drc);
> + if (!continue_token) {
> + g_free(req_data);
> + return H_P2;
> + }
> + req_data->token = continue_token;
> +
> + spapr_drc_run_async_hcall(drc, continue_token, &flush_worker_cb, req_data);
> +
> + ret = spapr_drc_get_async_hcall_status(drc, continue_token);
> + if (ret = H_BUSY) {
> + args[0] = req_data->token;
> + return ret;
> + }
> +
> + return ret;
> +}
> +
> static target_ulong h_scm_unbind_mem(PowerPCCPU *cpu, SpaprMachineState *spapr,
> target_ulong opcode, target_ulong *args)
> {
> @@ -486,6 +564,7 @@ static void spapr_scm_register_types(void)
> spapr_register_hypercall(H_SCM_BIND_MEM, h_scm_bind_mem);
> spapr_register_hypercall(H_SCM_UNBIND_MEM, h_scm_unbind_mem);
> spapr_register_hypercall(H_SCM_UNBIND_ALL, h_scm_unbind_all);
> + spapr_register_hypercall(H_SCM_ASYNC_FLUSH, h_scm_async_flush);
> }
>
> type_init(spapr_scm_register_types)
> diff --git a/include/hw/mem/nvdimm.h b/include/hw/mem/nvdimm.h
> index c699842dd0..9e8795766e 100644
> --- a/include/hw/mem/nvdimm.h
> +++ b/include/hw/mem/nvdimm.h
> @@ -51,6 +51,7 @@ OBJECT_DECLARE_TYPE(NVDIMMDevice, NVDIMMClass, NVDIMM)
> #define NVDIMM_LABEL_SIZE_PROP "label-size"
> #define NVDIMM_UUID_PROP "uuid"
> #define NVDIMM_UNARMED_PROP "unarmed"
> +#define NVDIMM_SYNC_DAX_PROP "sync-dax"
>
> struct NVDIMMDevice {
> /* private */
> @@ -85,6 +86,15 @@ struct NVDIMMDevice {
> */
> bool unarmed;
>
> + /*
> + * On PPC64,
> + * The 'off' value results in the async-flush-required property set
> + * in the device tree for pseries machines. When 'off', the guest
> + * initiates explicity flush requests to the backend device ensuring
> + * write persistence.
> + */
> + bool sync_dax;
> +
> /*
> * The PPC64 - spapr requires each nvdimm device have a uuid.
> */
> diff --git a/include/hw/ppc/spapr.h b/include/hw/ppc/spapr.h
> index 2e89e36cfb..6d7110b7dc 100644
> --- a/include/hw/ppc/spapr.h
> +++ b/include/hw/ppc/spapr.h
> @@ -535,8 +535,9 @@ struct SpaprMachineState {
> #define H_SCM_BIND_MEM 0x3EC
> #define H_SCM_UNBIND_MEM 0x3F0
> #define H_SCM_UNBIND_ALL 0x3FC
> +#define H_SCM_ASYNC_FLUSH 0x4A0
>
> -#define MAX_HCALL_OPCODE H_SCM_UNBIND_ALL
> +#define MAX_HCALL_OPCODE H_SCM_ASYNC_FLUSH
>
> /* The hcalls above are standardized in PAPR and implemented by pHyp
> * as well.
>
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 0/2] spapr: nvdimm: Asynchronus flush hcall support
2020-11-30 15:16 ` Shivaprasad G Bhat
(?)
@ 2020-12-21 13:07 ` Greg Kurz
-1 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 13:07 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc,
linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
On Mon, 30 Nov 2020 09:16:14 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> The nvdimm devices are expected to ensure write persistent during power
> failure kind of scenarios.
>
> The libpmem has architecture specific instructions like dcbf on power
> to flush the cache data to backend nvdimm device during normal writes.
>
> Qemu - virtual nvdimm devices are memory mapped. The dcbf in the guest
> doesn't traslate to actual flush to the backend file on the host in case
> of file backed vnvdimms. This is addressed by virtio-pmem in case of x86_64
> by making asynchronous flushes.
>
> On PAPR, issue is addressed by adding a new hcall to
> request for an explicit asynchronous flush requests from the guest ndctl
> driver when the backend nvdimm cannot ensure write persistence with dcbf
> alone. So, the approach here is to convey when the asynchronous flush is
> required in a device tree property. The guest makes the hcall when the
> property is found, instead of relying on dcbf.
>
> The first patch adds the necessary asynchronous hcall support infrastructure
> code at the DRC level. Second patch implements the hcall using the
> infrastructure.
>
> Hcall semantics are in review and not final.
>
> A new device property sync-dax is added to the nvdimm device. When the
> sync-dax is off(default), the asynchronous hcalls will be called.
>
> With respect to save from new qemu to restore on old qemu, having the
> sync-dax by default off(when not specified) causes IO errors in guests as
> the async-hcall would not be supported on old qemu. The new hcall
> implementation being supported only on the new pseries machine version,
> the current machine version checks may be sufficient to prevent
> such migration. Please suggest what should be done.
>
First, all requests that are still not completed from the guest POV,
ie. the hcall hasn't returned H_SUCCESS yet, are state that we should
migrate in theory. In this case, I guess we rather want to drain all
pending requests on the source in some pre-save handler.
Then, as explained in another mail, you should enforce stable behavior
for existing machine types with some hw_compat magic.
> The below demonstration shows the map_sync behavior with sync-dax on & off.
> (https://github.com/avocado-framework-tests/avocado-misc-tests/blob/master/memory/ndctl.py.data/map_sync.c)
>
> The pmem0 is from nvdimm with With sync-dax=on, and pmem1 is from nvdimm with syn-dax=off, mounted as
> /dev/pmem0 on /mnt1 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
> /dev/pmem1 on /mnt2 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
>
> [root@atest-guest ~]# ./mapsync /mnt1/newfile ----> When sync-dax=off
> [root@atest-guest ~]# ./mapsync /mnt2/newfile ----> when sync-dax=on
> Failed to mmap with Operation not supported
>
> ---
> v1 - https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06330.html
> Changes from v1
> - Fixed a missed-out unlock
> - using QLIST_FOREACH instead of QLIST_FOREACH_SAFE while generating token
>
> Shivaprasad G Bhat (2):
> spapr: drc: Add support for async hcalls at the drc level
> spapr: nvdimm: Implement async flush hcalls
>
>
> hw/mem/nvdimm.c | 1
> hw/ppc/spapr_drc.c | 146 ++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr_nvdimm.c | 79 ++++++++++++++++++++++++
> include/hw/mem/nvdimm.h | 10 +++
> include/hw/ppc/spapr.h | 3 +
> include/hw/ppc/spapr_drc.h | 25 ++++++++
> 6 files changed, 263 insertions(+), 1 deletion(-)
>
> --
> Signature
>
>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 0/2] spapr: nvdimm: Asynchronus flush hcall support
@ 2020-12-21 13:07 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 13:07 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev, david
On Mon, 30 Nov 2020 09:16:14 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> The nvdimm devices are expected to ensure write persistent during power
> failure kind of scenarios.
>
> The libpmem has architecture specific instructions like dcbf on power
> to flush the cache data to backend nvdimm device during normal writes.
>
> Qemu - virtual nvdimm devices are memory mapped. The dcbf in the guest
> doesn't traslate to actual flush to the backend file on the host in case
> of file backed vnvdimms. This is addressed by virtio-pmem in case of x86_64
> by making asynchronous flushes.
>
> On PAPR, issue is addressed by adding a new hcall to
> request for an explicit asynchronous flush requests from the guest ndctl
> driver when the backend nvdimm cannot ensure write persistence with dcbf
> alone. So, the approach here is to convey when the asynchronous flush is
> required in a device tree property. The guest makes the hcall when the
> property is found, instead of relying on dcbf.
>
> The first patch adds the necessary asynchronous hcall support infrastructure
> code at the DRC level. Second patch implements the hcall using the
> infrastructure.
>
> Hcall semantics are in review and not final.
>
> A new device property sync-dax is added to the nvdimm device. When the
> sync-dax is off(default), the asynchronous hcalls will be called.
>
> With respect to save from new qemu to restore on old qemu, having the
> sync-dax by default off(when not specified) causes IO errors in guests as
> the async-hcall would not be supported on old qemu. The new hcall
> implementation being supported only on the new pseries machine version,
> the current machine version checks may be sufficient to prevent
> such migration. Please suggest what should be done.
>
First, all requests that are still not completed from the guest POV,
ie. the hcall hasn't returned H_SUCCESS yet, are state that we should
migrate in theory. In this case, I guess we rather want to drain all
pending requests on the source in some pre-save handler.
Then, as explained in another mail, you should enforce stable behavior
for existing machine types with some hw_compat magic.
> The below demonstration shows the map_sync behavior with sync-dax on & off.
> (https://github.com/avocado-framework-tests/avocado-misc-tests/blob/master/memory/ndctl.py.data/map_sync.c)
>
> The pmem0 is from nvdimm with With sync-dax=on, and pmem1 is from nvdimm with syn-dax=off, mounted as
> /dev/pmem0 on /mnt1 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
> /dev/pmem1 on /mnt2 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize=32k,noquota)
>
> [root@atest-guest ~]# ./mapsync /mnt1/newfile ----> When sync-dax=off
> [root@atest-guest ~]# ./mapsync /mnt2/newfile ----> when sync-dax=on
> Failed to mmap with Operation not supported
>
> ---
> v1 - https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06330.html
> Changes from v1
> - Fixed a missed-out unlock
> - using QLIST_FOREACH instead of QLIST_FOREACH_SAFE while generating token
>
> Shivaprasad G Bhat (2):
> spapr: drc: Add support for async hcalls at the drc level
> spapr: nvdimm: Implement async flush hcalls
>
>
> hw/mem/nvdimm.c | 1
> hw/ppc/spapr_drc.c | 146 ++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr_nvdimm.c | 79 ++++++++++++++++++++++++
> include/hw/mem/nvdimm.h | 10 +++
> include/hw/ppc/spapr.h | 3 +
> include/hw/ppc/spapr_drc.h | 25 ++++++++
> 6 files changed, 263 insertions(+), 1 deletion(-)
>
> --
> Signature
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 0/2] spapr: nvdimm: Asynchronus flush hcall support
@ 2020-12-21 13:07 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 13:07 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, imammedo, david, qemu-devel, qemu-ppc,
linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
On Mon, 30 Nov 2020 09:16:14 -0600
Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
> The nvdimm devices are expected to ensure write persistent during power
> failure kind of scenarios.
>
> The libpmem has architecture specific instructions like dcbf on power
> to flush the cache data to backend nvdimm device during normal writes.
>
> Qemu - virtual nvdimm devices are memory mapped. The dcbf in the guest
> doesn't traslate to actual flush to the backend file on the host in case
> of file backed vnvdimms. This is addressed by virtio-pmem in case of x86_64
> by making asynchronous flushes.
>
> On PAPR, issue is addressed by adding a new hcall to
> request for an explicit asynchronous flush requests from the guest ndctl
> driver when the backend nvdimm cannot ensure write persistence with dcbf
> alone. So, the approach here is to convey when the asynchronous flush is
> required in a device tree property. The guest makes the hcall when the
> property is found, instead of relying on dcbf.
>
> The first patch adds the necessary asynchronous hcall support infrastructure
> code at the DRC level. Second patch implements the hcall using the
> infrastructure.
>
> Hcall semantics are in review and not final.
>
> A new device property sync-dax is added to the nvdimm device. When the
> sync-dax is off(default), the asynchronous hcalls will be called.
>
> With respect to save from new qemu to restore on old qemu, having the
> sync-dax by default off(when not specified) causes IO errors in guests as
> the async-hcall would not be supported on old qemu. The new hcall
> implementation being supported only on the new pseries machine version,
> the current machine version checks may be sufficient to prevent
> such migration. Please suggest what should be done.
>
First, all requests that are still not completed from the guest POV,
ie. the hcall hasn't returned H_SUCCESS yet, are state that we should
migrate in theory. In this case, I guess we rather want to drain all
pending requests on the source in some pre-save handler.
Then, as explained in another mail, you should enforce stable behavior
for existing machine types with some hw_compat magic.
> The below demonstration shows the map_sync behavior with sync-dax on & off.
> (https://github.com/avocado-framework-tests/avocado-misc-tests/blob/master/memory/ndctl.py.data/map_sync.c)
>
> The pmem0 is from nvdimm with With sync-dax=on, and pmem1 is from nvdimm with syn-dax=off, mounted as
> /dev/pmem0 on /mnt1 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize2k,noquota)
> /dev/pmem1 on /mnt2 type xfs (rw,relatime,attr2,dax=always,inode64,logbufs=8,logbsize2k,noquota)
>
> [root@atest-guest ~]# ./mapsync /mnt1/newfile ----> When sync-dax=off
> [root@atest-guest ~]# ./mapsync /mnt2/newfile ----> when sync-dax=on
> Failed to mmap with Operation not supported
>
> ---
> v1 - https://lists.gnu.org/archive/html/qemu-devel/2020-11/msg06330.html
> Changes from v1
> - Fixed a missed-out unlock
> - using QLIST_FOREACH instead of QLIST_FOREACH_SAFE while generating token
>
> Shivaprasad G Bhat (2):
> spapr: drc: Add support for async hcalls at the drc level
> spapr: nvdimm: Implement async flush hcalls
>
>
> hw/mem/nvdimm.c | 1
> hw/ppc/spapr_drc.c | 146 ++++++++++++++++++++++++++++++++++++++++++++
> hw/ppc/spapr_nvdimm.c | 79 ++++++++++++++++++++++++
> include/hw/mem/nvdimm.h | 10 +++
> include/hw/ppc/spapr.h | 3 +
> include/hw/ppc/spapr_drc.h | 25 ++++++++
> 6 files changed, 263 insertions(+), 1 deletion(-)
>
> --
> Signature
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
2020-12-21 12:08 ` Greg Kurz
(?)
@ 2020-12-21 14:37 ` Greg Kurz
-1 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 14:37 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev, david
On Mon, 21 Dec 2020 13:08:53 +0100
Greg Kurz <groug@kaod.org> wrote:
> Hi Shiva,
>
> On Mon, 30 Nov 2020 09:16:39 -0600
> Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
>
> > The patch adds support for async hcalls at the DRC level for the
> > spapr devices. To be used by spapr-scm devices in the patch/es to follow.
> >
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> > ---
>
> The overall idea looks good but I think you should consider using
> a thread pool to implement it. See below.
>
Some more comments.
> > hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_drc.h | 25 +++++++
> > 2 files changed, 174 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 77718cde1f..4ecd04f686 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -15,6 +15,7 @@
> > #include "qapi/qmp/qnull.h"
> > #include "cpu.h"
> > #include "qemu/cutils.h"
> > +#include "qemu/guest-random.h"
> > #include "hw/ppc/spapr_drc.h"
> > #include "qom/object.h"
> > #include "migration/vmstate.h"
> > @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> > spapr_drc_release(drc);
> > }
> >
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + *
> > + * All subsequent requests to run/query the status should use the
> > + * unique token returned here.
> > + */
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> > +{
> > + Error *err = NULL;
> > + uint64_t token;
> > + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> > +
> > + state = g_malloc0(sizeof(*state));
> > + state->pending = true;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +retry:
> > + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> > + error_report_err(err);
> > + g_free(state);
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > + return 0;
> > + }
> > +
> > + if (!token) /* Token should be non-zero */
> > + goto retry;
> > +
> > + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> > + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> > + if (tmp->continue_token == token) {
> > + /* If the token already in use, get a new one */
> > + goto retry;
> > + }
> > + }
> > + }
> > +
> > + state->continue_token = token;
> > + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> > +
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return state->continue_token;
> > +}
> > +
> > +static void *spapr_drc_async_hcall_runner(void *opaque)
> > +{
> > + int response = -1;
It feels wrong since the return value of func() is supposed to be
opaque to this function. And anyway it isn't needed since response
is always set a few lines below.
> > + SpaprDrcDeviceAsyncHCallState *state = opaque;
> > +
> > + /*
> > + * state is freed only after this thread finishes(after pthread_join()),
> > + * don't worry about it becoming NULL.
> > + */
> > +
> > + response = state->func(state->data);
> > +
> > + state->hcall_ret = response;
> > + state->pending = 0;
s/0/false/
> > +
> > + return NULL;
> > +}
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + * token : The continue token to be used for tracking as recived from
> > + * spapr_drc_get_new_async_hcall_token
> > + * @func() : the worker function which needs to be executed asynchronously
> > + * @data : data to be passed to the asynchronous function. Worker is supposed
> > + * to free/cleanup the data that is passed here
>
> It'd be cleaner to pass a completion callback and have free/cleanup handled there.
>
> > + */
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> > + if (state->continue_token == token) {
> > + state->func = func;
> > + state->data = data;
> > + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> > + spapr_drc_async_hcall_runner, state,
> > + QEMU_THREAD_JOINABLE);
>
> qemu_thread_create() exits on failure, it shouldn't be called on
> a guest triggerable path, eg. a buggy guest could call it up to
> the point that pthread_create() returns EAGAIN.
>
> Please use a thread pool (see thread_pool_submit_aio()). This takes care
> of all the thread housekeeping for you in a safe way, and it provides a
> completion callback API. The implementation could then be just about
> having two lists: one for pending requests (fed here) and one for
> completed requests (fed by the completion callback).
>
> > + break;
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_finish_async_hcalls
> > + * Waits for all pending async requests to complete
> > + * thier execution and free the states
> > + */
> > +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state, *next;
> > +
> > + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> > + return;
> > + }
> > +
This is called during machine reset and there won't be contention
in the large majority of cases. If the list is empty QLIST_FOREACH_SAFE()
won't iterate. So I don't think special casing the empty list brings much.
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> > + qemu_thread_join(&state->thread);
>
> With a thread-pool, you'd just need to aio_poll() until the pending list
> is empty and then clear the completed list.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_get_async_hcall_status
> > + * Fetches the status of the hcall worker and returns H_BUSY
> > + * if the worker is still running.
> > + */
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> > +{
> > + int ret = H_PARAMETER;
> > + SpaprDrcDeviceAsyncHCallState *state, *node;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> > + if (state->continue_token == token) {
> > + if (state->pending) {
> > + ret = H_BUSY;
> > + break;
> > + } else {
> > + ret = state->hcall_ret;
> > + qemu_thread_join(&state->thread);
>
> Like for qemu_thread_create(), the guest shouldn't be responsible for
> thread housekeeping. Getting the hcall status should just be about
> finding the token in the pending or completed lists.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + break;
> > + }
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return ret;
> > +}
> > +
> > void spapr_drc_reset(SpaprDrc *drc)
> > {
> > SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> > drc->ccs_offset = -1;
> > drc->ccs_depth = -1;
> > }
> > + spapr_drc_finish_async_hcalls(drc);
> > }
> >
> > static bool spapr_drc_unplug_requested_needed(void *opaque)
> > @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> > drc->owner = owner;
> > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> > spapr_drc_index(drc));
> > +
>
> Unrelated change.
>
> > object_property_add_child(owner, prop_name, OBJECT(drc));
> > object_unref(OBJECT(drc));
> > qdev_realize(DEVICE(drc), NULL, NULL);
> > @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> > object_property_add(obj, "fdt", "struct", prop_get_fdt,
> > NULL, NULL, NULL);
> > drc->state = drck->empty_state;
> > +
> > + qemu_mutex_init(&drc->async_hcall_states_lock);
> > + QLIST_INIT(&drc->async_hcall_states);
> > +
>
> Empty line not needed.
>
> > }
> >
> > static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 165b281496..77f6e4386c 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -18,6 +18,7 @@
> > #include "sysemu/runstate.h"
> > #include "hw/qdev-core.h"
> > #include "qapi/error.h"
> > +#include "block/thread-pool.h"
> >
> > #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> > #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> > @@ -168,6 +169,21 @@ typedef enum {
> > SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> > } SpaprDrcState;
> >
> > +typedef struct SpaprDrc SpaprDrc;
> > +
> > +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> > +typedef struct SpaprDrcDeviceAsyncHCallState {
> > + uint64_t continue_token;
> > + bool pending;
> > +
> > + int hcall_ret;
> > + SpaprDrcAsyncHcallWorkerFunc *func;
> > + void *data;
> > +
> > + QemuThread thread;
> > +
> > + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> > +} SpaprDrcDeviceAsyncHCallState;
> > typedef struct SpaprDrc {
> > /*< private >*/
> > DeviceState parent;
> > @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> > int ccs_offset;
> > int ccs_depth;
> >
> > + /* async hcall states */
> > + QemuMutex async_hcall_states_lock;
> > + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> > +
> > /* device pointer, via link property */
> > DeviceState *dev;
> > bool unplug_requested;
> > @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> > /* Returns true if a hot plug/unplug request is pending */
> > bool spapr_drc_transient(SpaprDrc *drc);
> >
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc, void *data);
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> > +
> > static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> > {
> > return drc->unplug_requested;
> >
> >
> >
>
>
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2020-12-21 14:37 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 14:37 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, linux-nvdimm, aneesh.kumar, mst, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev, david
On Mon, 21 Dec 2020 13:08:53 +0100
Greg Kurz <groug@kaod.org> wrote:
> Hi Shiva,
>
> On Mon, 30 Nov 2020 09:16:39 -0600
> Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
>
> > The patch adds support for async hcalls at the DRC level for the
> > spapr devices. To be used by spapr-scm devices in the patch/es to follow.
> >
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> > ---
>
> The overall idea looks good but I think you should consider using
> a thread pool to implement it. See below.
>
Some more comments.
> > hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_drc.h | 25 +++++++
> > 2 files changed, 174 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 77718cde1f..4ecd04f686 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -15,6 +15,7 @@
> > #include "qapi/qmp/qnull.h"
> > #include "cpu.h"
> > #include "qemu/cutils.h"
> > +#include "qemu/guest-random.h"
> > #include "hw/ppc/spapr_drc.h"
> > #include "qom/object.h"
> > #include "migration/vmstate.h"
> > @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> > spapr_drc_release(drc);
> > }
> >
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + *
> > + * All subsequent requests to run/query the status should use the
> > + * unique token returned here.
> > + */
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> > +{
> > + Error *err = NULL;
> > + uint64_t token;
> > + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> > +
> > + state = g_malloc0(sizeof(*state));
> > + state->pending = true;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +retry:
> > + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> > + error_report_err(err);
> > + g_free(state);
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > + return 0;
> > + }
> > +
> > + if (!token) /* Token should be non-zero */
> > + goto retry;
> > +
> > + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> > + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> > + if (tmp->continue_token == token) {
> > + /* If the token already in use, get a new one */
> > + goto retry;
> > + }
> > + }
> > + }
> > +
> > + state->continue_token = token;
> > + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> > +
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return state->continue_token;
> > +}
> > +
> > +static void *spapr_drc_async_hcall_runner(void *opaque)
> > +{
> > + int response = -1;
It feels wrong since the return value of func() is supposed to be
opaque to this function. And anyway it isn't needed since response
is always set a few lines below.
> > + SpaprDrcDeviceAsyncHCallState *state = opaque;
> > +
> > + /*
> > + * state is freed only after this thread finishes(after pthread_join()),
> > + * don't worry about it becoming NULL.
> > + */
> > +
> > + response = state->func(state->data);
> > +
> > + state->hcall_ret = response;
> > + state->pending = 0;
s/0/false/
> > +
> > + return NULL;
> > +}
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + * token : The continue token to be used for tracking as recived from
> > + * spapr_drc_get_new_async_hcall_token
> > + * @func() : the worker function which needs to be executed asynchronously
> > + * @data : data to be passed to the asynchronous function. Worker is supposed
> > + * to free/cleanup the data that is passed here
>
> It'd be cleaner to pass a completion callback and have free/cleanup handled there.
>
> > + */
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> > + if (state->continue_token == token) {
> > + state->func = func;
> > + state->data = data;
> > + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> > + spapr_drc_async_hcall_runner, state,
> > + QEMU_THREAD_JOINABLE);
>
> qemu_thread_create() exits on failure, it shouldn't be called on
> a guest triggerable path, eg. a buggy guest could call it up to
> the point that pthread_create() returns EAGAIN.
>
> Please use a thread pool (see thread_pool_submit_aio()). This takes care
> of all the thread housekeeping for you in a safe way, and it provides a
> completion callback API. The implementation could then be just about
> having two lists: one for pending requests (fed here) and one for
> completed requests (fed by the completion callback).
>
> > + break;
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_finish_async_hcalls
> > + * Waits for all pending async requests to complete
> > + * thier execution and free the states
> > + */
> > +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state, *next;
> > +
> > + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> > + return;
> > + }
> > +
This is called during machine reset and there won't be contention
in the large majority of cases. If the list is empty QLIST_FOREACH_SAFE()
won't iterate. So I don't think special casing the empty list brings much.
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> > + qemu_thread_join(&state->thread);
>
> With a thread-pool, you'd just need to aio_poll() until the pending list
> is empty and then clear the completed list.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_get_async_hcall_status
> > + * Fetches the status of the hcall worker and returns H_BUSY
> > + * if the worker is still running.
> > + */
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> > +{
> > + int ret = H_PARAMETER;
> > + SpaprDrcDeviceAsyncHCallState *state, *node;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> > + if (state->continue_token == token) {
> > + if (state->pending) {
> > + ret = H_BUSY;
> > + break;
> > + } else {
> > + ret = state->hcall_ret;
> > + qemu_thread_join(&state->thread);
>
> Like for qemu_thread_create(), the guest shouldn't be responsible for
> thread housekeeping. Getting the hcall status should just be about
> finding the token in the pending or completed lists.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + break;
> > + }
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return ret;
> > +}
> > +
> > void spapr_drc_reset(SpaprDrc *drc)
> > {
> > SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> > drc->ccs_offset = -1;
> > drc->ccs_depth = -1;
> > }
> > + spapr_drc_finish_async_hcalls(drc);
> > }
> >
> > static bool spapr_drc_unplug_requested_needed(void *opaque)
> > @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> > drc->owner = owner;
> > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> > spapr_drc_index(drc));
> > +
>
> Unrelated change.
>
> > object_property_add_child(owner, prop_name, OBJECT(drc));
> > object_unref(OBJECT(drc));
> > qdev_realize(DEVICE(drc), NULL, NULL);
> > @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> > object_property_add(obj, "fdt", "struct", prop_get_fdt,
> > NULL, NULL, NULL);
> > drc->state = drck->empty_state;
> > +
> > + qemu_mutex_init(&drc->async_hcall_states_lock);
> > + QLIST_INIT(&drc->async_hcall_states);
> > +
>
> Empty line not needed.
>
> > }
> >
> > static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 165b281496..77f6e4386c 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -18,6 +18,7 @@
> > #include "sysemu/runstate.h"
> > #include "hw/qdev-core.h"
> > #include "qapi/error.h"
> > +#include "block/thread-pool.h"
> >
> > #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> > #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> > @@ -168,6 +169,21 @@ typedef enum {
> > SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> > } SpaprDrcState;
> >
> > +typedef struct SpaprDrc SpaprDrc;
> > +
> > +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> > +typedef struct SpaprDrcDeviceAsyncHCallState {
> > + uint64_t continue_token;
> > + bool pending;
> > +
> > + int hcall_ret;
> > + SpaprDrcAsyncHcallWorkerFunc *func;
> > + void *data;
> > +
> > + QemuThread thread;
> > +
> > + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> > +} SpaprDrcDeviceAsyncHCallState;
> > typedef struct SpaprDrc {
> > /*< private >*/
> > DeviceState parent;
> > @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> > int ccs_offset;
> > int ccs_depth;
> >
> > + /* async hcall states */
> > + QemuMutex async_hcall_states_lock;
> > + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> > +
> > /* device pointer, via link property */
> > DeviceState *dev;
> > bool unplug_requested;
> > @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> > /* Returns true if a hot plug/unplug request is pending */
> > bool spapr_drc_transient(SpaprDrc *drc);
> >
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc, void *data);
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> > +
> > static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> > {
> > return drc->unplug_requested;
> >
> >
> >
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2020-12-21 14:37 ` Greg Kurz
0 siblings, 0 replies; 33+ messages in thread
From: Greg Kurz @ 2020-12-21 14:37 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev, david
On Mon, 21 Dec 2020 13:08:53 +0100
Greg Kurz <groug@kaod.org> wrote:
> Hi Shiva,
>
> On Mon, 30 Nov 2020 09:16:39 -0600
> Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
>
> > The patch adds support for async hcalls at the DRC level for the
> > spapr devices. To be used by spapr-scm devices in the patch/es to follow.
> >
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> > ---
>
> The overall idea looks good but I think you should consider using
> a thread pool to implement it. See below.
>
Some more comments.
> > hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_drc.h | 25 +++++++
> > 2 files changed, 174 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 77718cde1f..4ecd04f686 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -15,6 +15,7 @@
> > #include "qapi/qmp/qnull.h"
> > #include "cpu.h"
> > #include "qemu/cutils.h"
> > +#include "qemu/guest-random.h"
> > #include "hw/ppc/spapr_drc.h"
> > #include "qom/object.h"
> > #include "migration/vmstate.h"
> > @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> > spapr_drc_release(drc);
> > }
> >
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + *
> > + * All subsequent requests to run/query the status should use the
> > + * unique token returned here.
> > + */
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> > +{
> > + Error *err = NULL;
> > + uint64_t token;
> > + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> > +
> > + state = g_malloc0(sizeof(*state));
> > + state->pending = true;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +retry:
> > + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> > + error_report_err(err);
> > + g_free(state);
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > + return 0;
> > + }
> > +
> > + if (!token) /* Token should be non-zero */
> > + goto retry;
> > +
> > + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> > + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> > + if (tmp->continue_token = token) {
> > + /* If the token already in use, get a new one */
> > + goto retry;
> > + }
> > + }
> > + }
> > +
> > + state->continue_token = token;
> > + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> > +
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return state->continue_token;
> > +}
> > +
> > +static void *spapr_drc_async_hcall_runner(void *opaque)
> > +{
> > + int response = -1;
It feels wrong since the return value of func() is supposed to be
opaque to this function. And anyway it isn't needed since response
is always set a few lines below.
> > + SpaprDrcDeviceAsyncHCallState *state = opaque;
> > +
> > + /*
> > + * state is freed only after this thread finishes(after pthread_join()),
> > + * don't worry about it becoming NULL.
> > + */
> > +
> > + response = state->func(state->data);
> > +
> > + state->hcall_ret = response;
> > + state->pending = 0;
s/0/false/
> > +
> > + return NULL;
> > +}
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + * token : The continue token to be used for tracking as recived from
> > + * spapr_drc_get_new_async_hcall_token
> > + * @func() : the worker function which needs to be executed asynchronously
> > + * @data : data to be passed to the asynchronous function. Worker is supposed
> > + * to free/cleanup the data that is passed here
>
> It'd be cleaner to pass a completion callback and have free/cleanup handled there.
>
> > + */
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> > + if (state->continue_token = token) {
> > + state->func = func;
> > + state->data = data;
> > + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> > + spapr_drc_async_hcall_runner, state,
> > + QEMU_THREAD_JOINABLE);
>
> qemu_thread_create() exits on failure, it shouldn't be called on
> a guest triggerable path, eg. a buggy guest could call it up to
> the point that pthread_create() returns EAGAIN.
>
> Please use a thread pool (see thread_pool_submit_aio()). This takes care
> of all the thread housekeeping for you in a safe way, and it provides a
> completion callback API. The implementation could then be just about
> having two lists: one for pending requests (fed here) and one for
> completed requests (fed by the completion callback).
>
> > + break;
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_finish_async_hcalls
> > + * Waits for all pending async requests to complete
> > + * thier execution and free the states
> > + */
> > +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state, *next;
> > +
> > + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> > + return;
> > + }
> > +
This is called during machine reset and there won't be contention
in the large majority of cases. If the list is empty QLIST_FOREACH_SAFE()
won't iterate. So I don't think special casing the empty list brings much.
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> > + qemu_thread_join(&state->thread);
>
> With a thread-pool, you'd just need to aio_poll() until the pending list
> is empty and then clear the completed list.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_get_async_hcall_status
> > + * Fetches the status of the hcall worker and returns H_BUSY
> > + * if the worker is still running.
> > + */
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> > +{
> > + int ret = H_PARAMETER;
> > + SpaprDrcDeviceAsyncHCallState *state, *node;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> > + if (state->continue_token = token) {
> > + if (state->pending) {
> > + ret = H_BUSY;
> > + break;
> > + } else {
> > + ret = state->hcall_ret;
> > + qemu_thread_join(&state->thread);
>
> Like for qemu_thread_create(), the guest shouldn't be responsible for
> thread housekeeping. Getting the hcall status should just be about
> finding the token in the pending or completed lists.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + break;
> > + }
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return ret;
> > +}
> > +
> > void spapr_drc_reset(SpaprDrc *drc)
> > {
> > SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> > drc->ccs_offset = -1;
> > drc->ccs_depth = -1;
> > }
> > + spapr_drc_finish_async_hcalls(drc);
> > }
> >
> > static bool spapr_drc_unplug_requested_needed(void *opaque)
> > @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> > drc->owner = owner;
> > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> > spapr_drc_index(drc));
> > +
>
> Unrelated change.
>
> > object_property_add_child(owner, prop_name, OBJECT(drc));
> > object_unref(OBJECT(drc));
> > qdev_realize(DEVICE(drc), NULL, NULL);
> > @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> > object_property_add(obj, "fdt", "struct", prop_get_fdt,
> > NULL, NULL, NULL);
> > drc->state = drck->empty_state;
> > +
> > + qemu_mutex_init(&drc->async_hcall_states_lock);
> > + QLIST_INIT(&drc->async_hcall_states);
> > +
>
> Empty line not needed.
>
> > }
> >
> > static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 165b281496..77f6e4386c 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -18,6 +18,7 @@
> > #include "sysemu/runstate.h"
> > #include "hw/qdev-core.h"
> > #include "qapi/error.h"
> > +#include "block/thread-pool.h"
> >
> > #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> > #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> > @@ -168,6 +169,21 @@ typedef enum {
> > SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> > } SpaprDrcState;
> >
> > +typedef struct SpaprDrc SpaprDrc;
> > +
> > +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> > +typedef struct SpaprDrcDeviceAsyncHCallState {
> > + uint64_t continue_token;
> > + bool pending;
> > +
> > + int hcall_ret;
> > + SpaprDrcAsyncHcallWorkerFunc *func;
> > + void *data;
> > +
> > + QemuThread thread;
> > +
> > + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> > +} SpaprDrcDeviceAsyncHCallState;
> > typedef struct SpaprDrc {
> > /*< private >*/
> > DeviceState parent;
> > @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> > int ccs_offset;
> > int ccs_depth;
> >
> > + /* async hcall states */
> > + QemuMutex async_hcall_states_lock;
> > + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> > +
> > /* device pointer, via link property */
> > DeviceState *dev;
> > bool unplug_requested;
> > @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> > /* Returns true if a hot plug/unplug request is pending */
> > bool spapr_drc_transient(SpaprDrc *drc);
> >
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc, void *data);
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> > +
> > static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> > {
> > return drc->unplug_requested;
> >
> >
> >
>
>
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
2020-12-21 12:08 ` Greg Kurz
(?)
@ 2020-12-28 8:38 ` David Gibson
-1 siblings, 0 replies; 33+ messages in thread
From: David Gibson @ 2020-12-28 8:38 UTC (permalink / raw)
To: Greg Kurz
Cc: Shivaprasad G Bhat, xiaoguangrong.eric, mst, imammedo,
qemu-devel, qemu-ppc, linux-nvdimm, aneesh.kumar, kvm-ppc,
shivaprasadbhat, bharata, linuxppc-dev
[-- Attachment #1.1: Type: text/plain, Size: 11196 bytes --]
On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
> Hi Shiva,
>
> On Mon, 30 Nov 2020 09:16:39 -0600
> Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
>
> > The patch adds support for async hcalls at the DRC level for the
> > spapr devices. To be used by spapr-scm devices in the patch/es to follow.
> >
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> > ---
>
> The overall idea looks good but I think you should consider using
> a thread pool to implement it. See below.
I am not convinced, however. Specifically, attaching this to the DRC
doesn't make sense to me. We're adding exactly one DRC related async
hcall, and I can't really see much call for another one. We could
have other async hcalls - indeed we already have one for HPT resizing
- but attaching this to DRCs doesn't help for those.
>
> > hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_drc.h | 25 +++++++
> > 2 files changed, 174 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 77718cde1f..4ecd04f686 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -15,6 +15,7 @@
> > #include "qapi/qmp/qnull.h"
> > #include "cpu.h"
> > #include "qemu/cutils.h"
> > +#include "qemu/guest-random.h"
> > #include "hw/ppc/spapr_drc.h"
> > #include "qom/object.h"
> > #include "migration/vmstate.h"
> > @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> > spapr_drc_release(drc);
> > }
> >
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + *
> > + * All subsequent requests to run/query the status should use the
> > + * unique token returned here.
> > + */
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> > +{
> > + Error *err = NULL;
> > + uint64_t token;
> > + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> > +
> > + state = g_malloc0(sizeof(*state));
> > + state->pending = true;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +retry:
> > + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> > + error_report_err(err);
> > + g_free(state);
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > + return 0;
> > + }
> > +
> > + if (!token) /* Token should be non-zero */
> > + goto retry;
> > +
> > + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> > + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> > + if (tmp->continue_token == token) {
> > + /* If the token already in use, get a new one */
> > + goto retry;
> > + }
> > + }
> > + }
> > +
> > + state->continue_token = token;
> > + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> > +
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return state->continue_token;
> > +}
> > +
> > +static void *spapr_drc_async_hcall_runner(void *opaque)
> > +{
> > + int response = -1;
> > + SpaprDrcDeviceAsyncHCallState *state = opaque;
> > +
> > + /*
> > + * state is freed only after this thread finishes(after pthread_join()),
> > + * don't worry about it becoming NULL.
> > + */
> > +
> > + response = state->func(state->data);
> > +
> > + state->hcall_ret = response;
> > + state->pending = 0;
> > +
> > + return NULL;
> > +}
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + * token : The continue token to be used for tracking as recived from
> > + * spapr_drc_get_new_async_hcall_token
> > + * @func() : the worker function which needs to be executed asynchronously
> > + * @data : data to be passed to the asynchronous function. Worker is supposed
> > + * to free/cleanup the data that is passed here
>
> It'd be cleaner to pass a completion callback and have free/cleanup handled there.
>
> > + */
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> > + if (state->continue_token == token) {
> > + state->func = func;
> > + state->data = data;
> > + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> > + spapr_drc_async_hcall_runner, state,
> > + QEMU_THREAD_JOINABLE);
>
> qemu_thread_create() exits on failure, it shouldn't be called on
> a guest triggerable path, eg. a buggy guest could call it up to
> the point that pthread_create() returns EAGAIN.
>
> Please use a thread pool (see thread_pool_submit_aio()). This takes care
> of all the thread housekeeping for you in a safe way, and it provides a
> completion callback API. The implementation could then be just about
> having two lists: one for pending requests (fed here) and one for
> completed requests (fed by the completion callback).
>
> > + break;
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_finish_async_hcalls
> > + * Waits for all pending async requests to complete
> > + * thier execution and free the states
> > + */
> > +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state, *next;
> > +
> > + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> > + return;
> > + }
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> > + qemu_thread_join(&state->thread);
>
> With a thread-pool, you'd just need to aio_poll() until the pending list
> is empty and then clear the completed list.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_get_async_hcall_status
> > + * Fetches the status of the hcall worker and returns H_BUSY
> > + * if the worker is still running.
> > + */
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> > +{
> > + int ret = H_PARAMETER;
> > + SpaprDrcDeviceAsyncHCallState *state, *node;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> > + if (state->continue_token == token) {
> > + if (state->pending) {
> > + ret = H_BUSY;
> > + break;
> > + } else {
> > + ret = state->hcall_ret;
> > + qemu_thread_join(&state->thread);
>
> Like for qemu_thread_create(), the guest shouldn't be responsible for
> thread housekeeping. Getting the hcall status should just be about
> finding the token in the pending or completed lists.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + break;
> > + }
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return ret;
> > +}
> > +
> > void spapr_drc_reset(SpaprDrc *drc)
> > {
> > SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> > drc->ccs_offset = -1;
> > drc->ccs_depth = -1;
> > }
> > + spapr_drc_finish_async_hcalls(drc);
> > }
> >
> > static bool spapr_drc_unplug_requested_needed(void *opaque)
> > @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> > drc->owner = owner;
> > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> > spapr_drc_index(drc));
> > +
>
> Unrelated change.
>
> > object_property_add_child(owner, prop_name, OBJECT(drc));
> > object_unref(OBJECT(drc));
> > qdev_realize(DEVICE(drc), NULL, NULL);
> > @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> > object_property_add(obj, "fdt", "struct", prop_get_fdt,
> > NULL, NULL, NULL);
> > drc->state = drck->empty_state;
> > +
> > + qemu_mutex_init(&drc->async_hcall_states_lock);
> > + QLIST_INIT(&drc->async_hcall_states);
> > +
>
> Empty line not needed.
>
> > }
> >
> > static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 165b281496..77f6e4386c 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -18,6 +18,7 @@
> > #include "sysemu/runstate.h"
> > #include "hw/qdev-core.h"
> > #include "qapi/error.h"
> > +#include "block/thread-pool.h"
> >
> > #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> > #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> > @@ -168,6 +169,21 @@ typedef enum {
> > SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> > } SpaprDrcState;
> >
> > +typedef struct SpaprDrc SpaprDrc;
> > +
> > +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> > +typedef struct SpaprDrcDeviceAsyncHCallState {
> > + uint64_t continue_token;
> > + bool pending;
> > +
> > + int hcall_ret;
> > + SpaprDrcAsyncHcallWorkerFunc *func;
> > + void *data;
> > +
> > + QemuThread thread;
> > +
> > + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> > +} SpaprDrcDeviceAsyncHCallState;
> > typedef struct SpaprDrc {
> > /*< private >*/
> > DeviceState parent;
> > @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> > int ccs_offset;
> > int ccs_depth;
> >
> > + /* async hcall states */
> > + QemuMutex async_hcall_states_lock;
> > + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> > +
> > /* device pointer, via link property */
> > DeviceState *dev;
> > bool unplug_requested;
> > @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> > /* Returns true if a hot plug/unplug request is pending */
> > bool spapr_drc_transient(SpaprDrc *drc);
> >
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc, void *data);
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> > +
> > static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> > {
> > return drc->unplug_requested;
> >
> >
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 167 bytes --]
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2020-12-28 8:38 ` David Gibson
0 siblings, 0 replies; 33+ messages in thread
From: David Gibson @ 2020-12-28 8:38 UTC (permalink / raw)
To: Greg Kurz
Cc: xiaoguangrong.eric, Shivaprasad G Bhat, mst, aneesh.kumar,
linux-nvdimm, qemu-devel, kvm-ppc, shivaprasadbhat, qemu-ppc,
bharata, imammedo, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 11196 bytes --]
On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
> Hi Shiva,
>
> On Mon, 30 Nov 2020 09:16:39 -0600
> Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
>
> > The patch adds support for async hcalls at the DRC level for the
> > spapr devices. To be used by spapr-scm devices in the patch/es to follow.
> >
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> > ---
>
> The overall idea looks good but I think you should consider using
> a thread pool to implement it. See below.
I am not convinced, however. Specifically, attaching this to the DRC
doesn't make sense to me. We're adding exactly one DRC related async
hcall, and I can't really see much call for another one. We could
have other async hcalls - indeed we already have one for HPT resizing
- but attaching this to DRCs doesn't help for those.
>
> > hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_drc.h | 25 +++++++
> > 2 files changed, 174 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 77718cde1f..4ecd04f686 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -15,6 +15,7 @@
> > #include "qapi/qmp/qnull.h"
> > #include "cpu.h"
> > #include "qemu/cutils.h"
> > +#include "qemu/guest-random.h"
> > #include "hw/ppc/spapr_drc.h"
> > #include "qom/object.h"
> > #include "migration/vmstate.h"
> > @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> > spapr_drc_release(drc);
> > }
> >
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + *
> > + * All subsequent requests to run/query the status should use the
> > + * unique token returned here.
> > + */
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> > +{
> > + Error *err = NULL;
> > + uint64_t token;
> > + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> > +
> > + state = g_malloc0(sizeof(*state));
> > + state->pending = true;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +retry:
> > + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> > + error_report_err(err);
> > + g_free(state);
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > + return 0;
> > + }
> > +
> > + if (!token) /* Token should be non-zero */
> > + goto retry;
> > +
> > + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> > + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> > + if (tmp->continue_token == token) {
> > + /* If the token already in use, get a new one */
> > + goto retry;
> > + }
> > + }
> > + }
> > +
> > + state->continue_token = token;
> > + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> > +
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return state->continue_token;
> > +}
> > +
> > +static void *spapr_drc_async_hcall_runner(void *opaque)
> > +{
> > + int response = -1;
> > + SpaprDrcDeviceAsyncHCallState *state = opaque;
> > +
> > + /*
> > + * state is freed only after this thread finishes(after pthread_join()),
> > + * don't worry about it becoming NULL.
> > + */
> > +
> > + response = state->func(state->data);
> > +
> > + state->hcall_ret = response;
> > + state->pending = 0;
> > +
> > + return NULL;
> > +}
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + * token : The continue token to be used for tracking as recived from
> > + * spapr_drc_get_new_async_hcall_token
> > + * @func() : the worker function which needs to be executed asynchronously
> > + * @data : data to be passed to the asynchronous function. Worker is supposed
> > + * to free/cleanup the data that is passed here
>
> It'd be cleaner to pass a completion callback and have free/cleanup handled there.
>
> > + */
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> > + if (state->continue_token == token) {
> > + state->func = func;
> > + state->data = data;
> > + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> > + spapr_drc_async_hcall_runner, state,
> > + QEMU_THREAD_JOINABLE);
>
> qemu_thread_create() exits on failure, it shouldn't be called on
> a guest triggerable path, eg. a buggy guest could call it up to
> the point that pthread_create() returns EAGAIN.
>
> Please use a thread pool (see thread_pool_submit_aio()). This takes care
> of all the thread housekeeping for you in a safe way, and it provides a
> completion callback API. The implementation could then be just about
> having two lists: one for pending requests (fed here) and one for
> completed requests (fed by the completion callback).
>
> > + break;
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_finish_async_hcalls
> > + * Waits for all pending async requests to complete
> > + * thier execution and free the states
> > + */
> > +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state, *next;
> > +
> > + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> > + return;
> > + }
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> > + qemu_thread_join(&state->thread);
>
> With a thread-pool, you'd just need to aio_poll() until the pending list
> is empty and then clear the completed list.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_get_async_hcall_status
> > + * Fetches the status of the hcall worker and returns H_BUSY
> > + * if the worker is still running.
> > + */
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> > +{
> > + int ret = H_PARAMETER;
> > + SpaprDrcDeviceAsyncHCallState *state, *node;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> > + if (state->continue_token == token) {
> > + if (state->pending) {
> > + ret = H_BUSY;
> > + break;
> > + } else {
> > + ret = state->hcall_ret;
> > + qemu_thread_join(&state->thread);
>
> Like for qemu_thread_create(), the guest shouldn't be responsible for
> thread housekeeping. Getting the hcall status should just be about
> finding the token in the pending or completed lists.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + break;
> > + }
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return ret;
> > +}
> > +
> > void spapr_drc_reset(SpaprDrc *drc)
> > {
> > SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> > drc->ccs_offset = -1;
> > drc->ccs_depth = -1;
> > }
> > + spapr_drc_finish_async_hcalls(drc);
> > }
> >
> > static bool spapr_drc_unplug_requested_needed(void *opaque)
> > @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> > drc->owner = owner;
> > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> > spapr_drc_index(drc));
> > +
>
> Unrelated change.
>
> > object_property_add_child(owner, prop_name, OBJECT(drc));
> > object_unref(OBJECT(drc));
> > qdev_realize(DEVICE(drc), NULL, NULL);
> > @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> > object_property_add(obj, "fdt", "struct", prop_get_fdt,
> > NULL, NULL, NULL);
> > drc->state = drck->empty_state;
> > +
> > + qemu_mutex_init(&drc->async_hcall_states_lock);
> > + QLIST_INIT(&drc->async_hcall_states);
> > +
>
> Empty line not needed.
>
> > }
> >
> > static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 165b281496..77f6e4386c 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -18,6 +18,7 @@
> > #include "sysemu/runstate.h"
> > #include "hw/qdev-core.h"
> > #include "qapi/error.h"
> > +#include "block/thread-pool.h"
> >
> > #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> > #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> > @@ -168,6 +169,21 @@ typedef enum {
> > SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> > } SpaprDrcState;
> >
> > +typedef struct SpaprDrc SpaprDrc;
> > +
> > +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> > +typedef struct SpaprDrcDeviceAsyncHCallState {
> > + uint64_t continue_token;
> > + bool pending;
> > +
> > + int hcall_ret;
> > + SpaprDrcAsyncHcallWorkerFunc *func;
> > + void *data;
> > +
> > + QemuThread thread;
> > +
> > + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> > +} SpaprDrcDeviceAsyncHCallState;
> > typedef struct SpaprDrc {
> > /*< private >*/
> > DeviceState parent;
> > @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> > int ccs_offset;
> > int ccs_depth;
> >
> > + /* async hcall states */
> > + QemuMutex async_hcall_states_lock;
> > + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> > +
> > /* device pointer, via link property */
> > DeviceState *dev;
> > bool unplug_requested;
> > @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> > /* Returns true if a hot plug/unplug request is pending */
> > bool spapr_drc_transient(SpaprDrc *drc);
> >
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc, void *data);
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> > +
> > static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> > {
> > return drc->unplug_requested;
> >
> >
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2020-12-28 8:38 ` David Gibson
0 siblings, 0 replies; 33+ messages in thread
From: David Gibson @ 2020-12-28 8:38 UTC (permalink / raw)
To: Greg Kurz
Cc: Shivaprasad G Bhat, xiaoguangrong.eric, mst, imammedo,
qemu-devel, qemu-ppc, linux-nvdimm, aneesh.kumar, kvm-ppc,
shivaprasadbhat, bharata, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 11196 bytes --]
On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
> Hi Shiva,
>
> On Mon, 30 Nov 2020 09:16:39 -0600
> Shivaprasad G Bhat <sbhat@linux.ibm.com> wrote:
>
> > The patch adds support for async hcalls at the DRC level for the
> > spapr devices. To be used by spapr-scm devices in the patch/es to follow.
> >
> > Signed-off-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
> > ---
>
> The overall idea looks good but I think you should consider using
> a thread pool to implement it. See below.
I am not convinced, however. Specifically, attaching this to the DRC
doesn't make sense to me. We're adding exactly one DRC related async
hcall, and I can't really see much call for another one. We could
have other async hcalls - indeed we already have one for HPT resizing
- but attaching this to DRCs doesn't help for those.
>
> > hw/ppc/spapr_drc.c | 149 ++++++++++++++++++++++++++++++++++++++++++++
> > include/hw/ppc/spapr_drc.h | 25 +++++++
> > 2 files changed, 174 insertions(+)
> >
> > diff --git a/hw/ppc/spapr_drc.c b/hw/ppc/spapr_drc.c
> > index 77718cde1f..4ecd04f686 100644
> > --- a/hw/ppc/spapr_drc.c
> > +++ b/hw/ppc/spapr_drc.c
> > @@ -15,6 +15,7 @@
> > #include "qapi/qmp/qnull.h"
> > #include "cpu.h"
> > #include "qemu/cutils.h"
> > +#include "qemu/guest-random.h"
> > #include "hw/ppc/spapr_drc.h"
> > #include "qom/object.h"
> > #include "migration/vmstate.h"
> > @@ -421,6 +422,148 @@ void spapr_drc_detach(SpaprDrc *drc)
> > spapr_drc_release(drc);
> > }
> >
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + *
> > + * All subsequent requests to run/query the status should use the
> > + * unique token returned here.
> > + */
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc)
> > +{
> > + Error *err = NULL;
> > + uint64_t token;
> > + SpaprDrcDeviceAsyncHCallState *tmp, *next, *state;
> > +
> > + state = g_malloc0(sizeof(*state));
> > + state->pending = true;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > +retry:
> > + if (qemu_guest_getrandom(&token, sizeof(token), &err) < 0) {
> > + error_report_err(err);
> > + g_free(state);
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > + return 0;
> > + }
> > +
> > + if (!token) /* Token should be non-zero */
> > + goto retry;
> > +
> > + if (!QLIST_EMPTY(&drc->async_hcall_states)) {
> > + QLIST_FOREACH_SAFE(tmp, &drc->async_hcall_states, node, next) {
> > + if (tmp->continue_token == token) {
> > + /* If the token already in use, get a new one */
> > + goto retry;
> > + }
> > + }
> > + }
> > +
> > + state->continue_token = token;
> > + QLIST_INSERT_HEAD(&drc->async_hcall_states, state, node);
> > +
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return state->continue_token;
> > +}
> > +
> > +static void *spapr_drc_async_hcall_runner(void *opaque)
> > +{
> > + int response = -1;
> > + SpaprDrcDeviceAsyncHCallState *state = opaque;
> > +
> > + /*
> > + * state is freed only after this thread finishes(after pthread_join()),
> > + * don't worry about it becoming NULL.
> > + */
> > +
> > + response = state->func(state->data);
> > +
> > + state->hcall_ret = response;
> > + state->pending = 0;
> > +
> > + return NULL;
> > +}
> > +
> > +/*
> > + * @drc : device DRC targetting which the async hcalls to be made.
> > + * token : The continue token to be used for tracking as recived from
> > + * spapr_drc_get_new_async_hcall_token
> > + * @func() : the worker function which needs to be executed asynchronously
> > + * @data : data to be passed to the asynchronous function. Worker is supposed
> > + * to free/cleanup the data that is passed here
>
> It'd be cleaner to pass a completion callback and have free/cleanup handled there.
>
> > + */
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc *func, void *data)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH(state, &drc->async_hcall_states, node) {
> > + if (state->continue_token == token) {
> > + state->func = func;
> > + state->data = data;
> > + qemu_thread_create(&state->thread, "sPAPR Async HCALL",
> > + spapr_drc_async_hcall_runner, state,
> > + QEMU_THREAD_JOINABLE);
>
> qemu_thread_create() exits on failure, it shouldn't be called on
> a guest triggerable path, eg. a buggy guest could call it up to
> the point that pthread_create() returns EAGAIN.
>
> Please use a thread pool (see thread_pool_submit_aio()). This takes care
> of all the thread housekeeping for you in a safe way, and it provides a
> completion callback API. The implementation could then be just about
> having two lists: one for pending requests (fed here) and one for
> completed requests (fed by the completion callback).
>
> > + break;
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_finish_async_hcalls
> > + * Waits for all pending async requests to complete
> > + * thier execution and free the states
> > + */
> > +static void spapr_drc_finish_async_hcalls(SpaprDrc *drc)
> > +{
> > + SpaprDrcDeviceAsyncHCallState *state, *next;
> > +
> > + if (QLIST_EMPTY(&drc->async_hcall_states)) {
> > + return;
> > + }
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, next) {
> > + qemu_thread_join(&state->thread);
>
> With a thread-pool, you'd just need to aio_poll() until the pending list
> is empty and then clear the completed list.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +}
> > +
> > +/*
> > + * spapr_drc_get_async_hcall_status
> > + * Fetches the status of the hcall worker and returns H_BUSY
> > + * if the worker is still running.
> > + */
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token)
> > +{
> > + int ret = H_PARAMETER;
> > + SpaprDrcDeviceAsyncHCallState *state, *node;
> > +
> > + qemu_mutex_lock(&drc->async_hcall_states_lock);
> > + QLIST_FOREACH_SAFE(state, &drc->async_hcall_states, node, node) {
> > + if (state->continue_token == token) {
> > + if (state->pending) {
> > + ret = H_BUSY;
> > + break;
> > + } else {
> > + ret = state->hcall_ret;
> > + qemu_thread_join(&state->thread);
>
> Like for qemu_thread_create(), the guest shouldn't be responsible for
> thread housekeeping. Getting the hcall status should just be about
> finding the token in the pending or completed lists.
>
> > + QLIST_REMOVE(state, node);
> > + g_free(state);
> > + break;
> > + }
> > + }
> > + }
> > + qemu_mutex_unlock(&drc->async_hcall_states_lock);
> > +
> > + return ret;
> > +}
> > +
> > void spapr_drc_reset(SpaprDrc *drc)
> > {
> > SpaprDrcClass *drck = SPAPR_DR_CONNECTOR_GET_CLASS(drc);
> > @@ -448,6 +591,7 @@ void spapr_drc_reset(SpaprDrc *drc)
> > drc->ccs_offset = -1;
> > drc->ccs_depth = -1;
> > }
> > + spapr_drc_finish_async_hcalls(drc);
> > }
> >
> > static bool spapr_drc_unplug_requested_needed(void *opaque)
> > @@ -558,6 +702,7 @@ SpaprDrc *spapr_dr_connector_new(Object *owner, const char *type,
> > drc->owner = owner;
> > prop_name = g_strdup_printf("dr-connector[%"PRIu32"]",
> > spapr_drc_index(drc));
> > +
>
> Unrelated change.
>
> > object_property_add_child(owner, prop_name, OBJECT(drc));
> > object_unref(OBJECT(drc));
> > qdev_realize(DEVICE(drc), NULL, NULL);
> > @@ -577,6 +722,10 @@ static void spapr_dr_connector_instance_init(Object *obj)
> > object_property_add(obj, "fdt", "struct", prop_get_fdt,
> > NULL, NULL, NULL);
> > drc->state = drck->empty_state;
> > +
> > + qemu_mutex_init(&drc->async_hcall_states_lock);
> > + QLIST_INIT(&drc->async_hcall_states);
> > +
>
> Empty line not needed.
>
> > }
> >
> > static void spapr_dr_connector_class_init(ObjectClass *k, void *data)
> > diff --git a/include/hw/ppc/spapr_drc.h b/include/hw/ppc/spapr_drc.h
> > index 165b281496..77f6e4386c 100644
> > --- a/include/hw/ppc/spapr_drc.h
> > +++ b/include/hw/ppc/spapr_drc.h
> > @@ -18,6 +18,7 @@
> > #include "sysemu/runstate.h"
> > #include "hw/qdev-core.h"
> > #include "qapi/error.h"
> > +#include "block/thread-pool.h"
> >
> > #define TYPE_SPAPR_DR_CONNECTOR "spapr-dr-connector"
> > #define SPAPR_DR_CONNECTOR_GET_CLASS(obj) \
> > @@ -168,6 +169,21 @@ typedef enum {
> > SPAPR_DRC_STATE_PHYSICAL_CONFIGURED = 8,
> > } SpaprDrcState;
> >
> > +typedef struct SpaprDrc SpaprDrc;
> > +
> > +typedef int SpaprDrcAsyncHcallWorkerFunc(void *opaque);
> > +typedef struct SpaprDrcDeviceAsyncHCallState {
> > + uint64_t continue_token;
> > + bool pending;
> > +
> > + int hcall_ret;
> > + SpaprDrcAsyncHcallWorkerFunc *func;
> > + void *data;
> > +
> > + QemuThread thread;
> > +
> > + QLIST_ENTRY(SpaprDrcDeviceAsyncHCallState) node;
> > +} SpaprDrcDeviceAsyncHCallState;
> > typedef struct SpaprDrc {
> > /*< private >*/
> > DeviceState parent;
> > @@ -182,6 +198,10 @@ typedef struct SpaprDrc {
> > int ccs_offset;
> > int ccs_depth;
> >
> > + /* async hcall states */
> > + QemuMutex async_hcall_states_lock;
> > + QLIST_HEAD(, SpaprDrcDeviceAsyncHCallState) async_hcall_states;
> > +
> > /* device pointer, via link property */
> > DeviceState *dev;
> > bool unplug_requested;
> > @@ -241,6 +261,11 @@ void spapr_drc_detach(SpaprDrc *drc);
> > /* Returns true if a hot plug/unplug request is pending */
> > bool spapr_drc_transient(SpaprDrc *drc);
> >
> > +uint64_t spapr_drc_get_new_async_hcall_token(SpaprDrc *drc);
> > +void spapr_drc_run_async_hcall(SpaprDrc *drc, uint64_t token,
> > + SpaprDrcAsyncHcallWorkerFunc, void *data);
> > +int spapr_drc_get_async_hcall_status(SpaprDrc *drc, uint64_t token);
> > +
> > static inline bool spapr_drc_unplug_requested(SpaprDrc *drc)
> > {
> > return drc->unplug_requested;
> >
> >
> >
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
2020-12-28 8:38 ` David Gibson
(?)
@ 2021-01-19 7:10 ` Shivaprasad G Bhat
-1 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2021-01-19 7:10 UTC (permalink / raw)
To: David Gibson, Greg Kurz
Cc: xiaoguangrong.eric, mst, imammedo, qemu-devel, qemu-ppc,
linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
Thanks for the comments!
On 12/28/20 2:08 PM, David Gibson wrote:
> On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
...
>> The overall idea looks good but I think you should consider using
>> a thread pool to implement it. See below.
> I am not convinced, however. Specifically, attaching this to the DRC
> doesn't make sense to me. We're adding exactly one DRC related async
> hcall, and I can't really see much call for another one. We could
> have other async hcalls - indeed we already have one for HPT resizing
> - but attaching this to DRCs doesn't help for those.
The semantics of the hcall made me think, if this is going to be
re-usable for future if implemented at DRC level. Other option
is to move the async-hcall-state/list into the NVDIMMState structure
in include/hw/mem/nvdimm.h and handle it with machine->nvdimms_state
at a global level.
Hope you are okay with using the pool based approach that Greg
suggested.
Please let me know.
Thanks,
Shivaprasad
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2021-01-19 7:10 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2021-01-19 7:10 UTC (permalink / raw)
To: David Gibson, Greg Kurz
Cc: xiaoguangrong.eric, linux-nvdimm, aneesh.kumar, mst, qemu-devel,
kvm-ppc, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev
Thanks for the comments!
On 12/28/20 2:08 PM, David Gibson wrote:
> On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
...
>> The overall idea looks good but I think you should consider using
>> a thread pool to implement it. See below.
> I am not convinced, however. Specifically, attaching this to the DRC
> doesn't make sense to me. We're adding exactly one DRC related async
> hcall, and I can't really see much call for another one. We could
> have other async hcalls - indeed we already have one for HPT resizing
> - but attaching this to DRCs doesn't help for those.
The semantics of the hcall made me think, if this is going to be
re-usable for future if implemented at DRC level. Other option
is to move the async-hcall-state/list into the NVDIMMState structure
in include/hw/mem/nvdimm.h and handle it with machine->nvdimms_state
at a global level.
Hope you are okay with using the pool based approach that Greg
suggested.
Please let me know.
Thanks,
Shivaprasad
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2021-01-19 7:10 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2021-01-19 7:22 UTC (permalink / raw)
To: David Gibson, Greg Kurz
Cc: xiaoguangrong.eric, mst, imammedo, qemu-devel, qemu-ppc,
linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat, bharata,
linuxppc-dev
Thanks for the comments!
On 12/28/20 2:08 PM, David Gibson wrote:
> On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
...
>> The overall idea looks good but I think you should consider using
>> a thread pool to implement it. See below.
> I am not convinced, however. Specifically, attaching this to the DRC
> doesn't make sense to me. We're adding exactly one DRC related async
> hcall, and I can't really see much call for another one. We could
> have other async hcalls - indeed we already have one for HPT resizing
> - but attaching this to DRCs doesn't help for those.
The semantics of the hcall made me think, if this is going to be
re-usable for future if implemented at DRC level. Other option
is to move the async-hcall-state/list into the NVDIMMState structure
in include/hw/mem/nvdimm.h and handle it with machine->nvdimms_state
at a global level.
Hope you are okay with using the pool based approach that Greg
suggested.
Please let me know.
Thanks,
Shivaprasad
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
2021-01-19 7:10 ` Shivaprasad G Bhat
(?)
@ 2021-02-08 6:21 ` David Gibson
-1 siblings, 0 replies; 33+ messages in thread
From: David Gibson @ 2021-02-08 6:21 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: Greg Kurz, xiaoguangrong.eric, mst, imammedo, qemu-devel,
qemu-ppc, linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat,
bharata, linuxppc-dev
[-- Attachment #1.1: Type: text/plain, Size: 1890 bytes --]
On Tue, Jan 19, 2021 at 12:40:31PM +0530, Shivaprasad G Bhat wrote:
> Thanks for the comments!
>
>
> On 12/28/20 2:08 PM, David Gibson wrote:
>
> > On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
> ...
> > > The overall idea looks good but I think you should consider using
> > > a thread pool to implement it. See below.
> > I am not convinced, however. Specifically, attaching this to the DRC
> > doesn't make sense to me. We're adding exactly one DRC related async
> > hcall, and I can't really see much call for another one. We could
> > have other async hcalls - indeed we already have one for HPT resizing
> > - but attaching this to DRCs doesn't help for those.
>
> The semantics of the hcall made me think, if this is going to be
> re-usable for future if implemented at DRC level.
It would only be re-usable for operations that are actually connected
to DRCs. It doesn't seem to me particularly likely that we'll ever
have more asynchronous hcalls that are also associated with DRCs.
> Other option
> is to move the async-hcall-state/list into the NVDIMMState structure
> in include/hw/mem/nvdimm.h and handle it with machine->nvdimms_state
> at a global level.
I'm ok with either of two options:
A) Implement this ad-hoc for this specific case, making whatever
simplifications you can based on this specific case.
B) Implement a general mechanism for async hcalls that is *not* tied
to DRCs. Then use that for the existing H_RESIZE_HPT_PREPARE call as
well as this new one.
> Hope you are okay with using the pool based approach that Greg
Honestly a thread pool seems like it might be overkill for this
application.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 167 bytes --]
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2021-02-08 6:21 ` David Gibson
0 siblings, 0 replies; 33+ messages in thread
From: David Gibson @ 2021-02-08 6:21 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, Greg Kurz, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]
On Tue, Jan 19, 2021 at 12:40:31PM +0530, Shivaprasad G Bhat wrote:
> Thanks for the comments!
>
>
> On 12/28/20 2:08 PM, David Gibson wrote:
>
> > On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
> ...
> > > The overall idea looks good but I think you should consider using
> > > a thread pool to implement it. See below.
> > I am not convinced, however. Specifically, attaching this to the DRC
> > doesn't make sense to me. We're adding exactly one DRC related async
> > hcall, and I can't really see much call for another one. We could
> > have other async hcalls - indeed we already have one for HPT resizing
> > - but attaching this to DRCs doesn't help for those.
>
> The semantics of the hcall made me think, if this is going to be
> re-usable for future if implemented at DRC level.
It would only be re-usable for operations that are actually connected
to DRCs. It doesn't seem to me particularly likely that we'll ever
have more asynchronous hcalls that are also associated with DRCs.
> Other option
> is to move the async-hcall-state/list into the NVDIMMState structure
> in include/hw/mem/nvdimm.h and handle it with machine->nvdimms_state
> at a global level.
I'm ok with either of two options:
A) Implement this ad-hoc for this specific case, making whatever
simplifications you can based on this specific case.
B) Implement a general mechanism for async hcalls that is *not* tied
to DRCs. Then use that for the existing H_RESIZE_HPT_PREPARE call as
well as this new one.
> Hope you are okay with using the pool based approach that Greg
Honestly a thread pool seems like it might be overkill for this
application.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2021-02-08 6:21 ` David Gibson
0 siblings, 0 replies; 33+ messages in thread
From: David Gibson @ 2021-02-08 6:21 UTC (permalink / raw)
To: Shivaprasad G Bhat
Cc: Greg Kurz, xiaoguangrong.eric, mst, imammedo, qemu-devel,
qemu-ppc, linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat,
bharata, linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]
On Tue, Jan 19, 2021 at 12:40:31PM +0530, Shivaprasad G Bhat wrote:
> Thanks for the comments!
>
>
> On 12/28/20 2:08 PM, David Gibson wrote:
>
> > On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
> ...
> > > The overall idea looks good but I think you should consider using
> > > a thread pool to implement it. See below.
> > I am not convinced, however. Specifically, attaching this to the DRC
> > doesn't make sense to me. We're adding exactly one DRC related async
> > hcall, and I can't really see much call for another one. We could
> > have other async hcalls - indeed we already have one for HPT resizing
> > - but attaching this to DRCs doesn't help for those.
>
> The semantics of the hcall made me think, if this is going to be
> re-usable for future if implemented at DRC level.
It would only be re-usable for operations that are actually connected
to DRCs. It doesn't seem to me particularly likely that we'll ever
have more asynchronous hcalls that are also associated with DRCs.
> Other option
> is to move the async-hcall-state/list into the NVDIMMState structure
> in include/hw/mem/nvdimm.h and handle it with machine->nvdimms_state
> at a global level.
I'm ok with either of two options:
A) Implement this ad-hoc for this specific case, making whatever
simplifications you can based on this specific case.
B) Implement a general mechanism for async hcalls that is *not* tied
to DRCs. Then use that for the existing H_RESIZE_HPT_PREPARE call as
well as this new one.
> Hope you are okay with using the pool based approach that Greg
Honestly a thread pool seems like it might be overkill for this
application.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
2021-02-08 6:21 ` David Gibson
(?)
@ 2021-03-23 7:53 ` Shivaprasad G Bhat
-1 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2021-03-23 7:53 UTC (permalink / raw)
To: David Gibson
Cc: Greg Kurz, xiaoguangrong.eric, mst, imammedo, qemu-devel,
qemu-ppc, linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat,
bharata, linuxppc-dev
Hi David,
Sorry about the delay.
On 2/8/21 11:51 AM, David Gibson wrote:
> On Tue, Jan 19, 2021 at 12:40:31PM +0530, Shivaprasad G Bhat wrote:
>> Thanks for the comments!
>>
>>
>> On 12/28/20 2:08 PM, David Gibson wrote:
>>
>>> On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
>> ...
>>>> The overall idea looks good but I think you should consider using
>>>> a thread pool to implement it. See below.
>>> I am not convinced, however. Specifically, attaching this to the DRC
>>> doesn't make sense to me. We're adding exactly one DRC related async
>>> hcall, and I can't really see much call for another one. We could
>>> have other async hcalls - indeed we already have one for HPT resizing
>>> - but attaching this to DRCs doesn't help for those.
>> The semantics of the hcall made me think, if this is going to be
>> re-usable for future if implemented at DRC level.
> It would only be re-usable for operations that are actually connected
> to DRCs. It doesn't seem to me particularly likely that we'll ever
> have more asynchronous hcalls that are also associated with DRCs.
Okay
>> Other option
>> is to move the async-hcall-state/list into the NVDIMMState structure
>> in include/hw/mem/nvdimm.h and handle it with machine->nvdimms_state
>> at a global level.
> I'm ok with either of two options:
>
> A) Implement this ad-hoc for this specific case, making whatever
> simplifications you can based on this specific case.
I am simplifying it to nvdimm use-case alone and limiting the scope.
> B) Implement a general mechanism for async hcalls that is *not* tied
> to DRCs. Then use that for the existing H_RESIZE_HPT_PREPARE call as
> well as this new one.
>
>> Hope you are okay with using the pool based approach that Greg
> Honestly a thread pool seems like it might be overkill for this
> application.
I think its appropriate here as that is what is being done by virtio-pmem
too for flush requests. The aio infrastructure simplifies lot of the
thread handling usage. Please suggest if you think there are better ways.
I am sending the next version addressing all the comments from you and Greg.
Thanks,
Shivaprasad
_______________________________________________
Linux-nvdimm mailing list -- linux-nvdimm@lists.01.org
To unsubscribe send an email to linux-nvdimm-leave@lists.01.org
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2021-03-23 7:53 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2021-03-23 7:53 UTC (permalink / raw)
To: David Gibson
Cc: xiaoguangrong.eric, mst, aneesh.kumar, linux-nvdimm, qemu-devel,
kvm-ppc, Greg Kurz, shivaprasadbhat, qemu-ppc, bharata, imammedo,
linuxppc-dev
Hi David,
Sorry about the delay.
On 2/8/21 11:51 AM, David Gibson wrote:
> On Tue, Jan 19, 2021 at 12:40:31PM +0530, Shivaprasad G Bhat wrote:
>> Thanks for the comments!
>>
>>
>> On 12/28/20 2:08 PM, David Gibson wrote:
>>
>>> On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
>> ...
>>>> The overall idea looks good but I think you should consider using
>>>> a thread pool to implement it. See below.
>>> I am not convinced, however. Specifically, attaching this to the DRC
>>> doesn't make sense to me. We're adding exactly one DRC related async
>>> hcall, and I can't really see much call for another one. We could
>>> have other async hcalls - indeed we already have one for HPT resizing
>>> - but attaching this to DRCs doesn't help for those.
>> The semantics of the hcall made me think, if this is going to be
>> re-usable for future if implemented at DRC level.
> It would only be re-usable for operations that are actually connected
> to DRCs. It doesn't seem to me particularly likely that we'll ever
> have more asynchronous hcalls that are also associated with DRCs.
Okay
>> Other option
>> is to move the async-hcall-state/list into the NVDIMMState structure
>> in include/hw/mem/nvdimm.h and handle it with machine->nvdimms_state
>> at a global level.
> I'm ok with either of two options:
>
> A) Implement this ad-hoc for this specific case, making whatever
> simplifications you can based on this specific case.
I am simplifying it to nvdimm use-case alone and limiting the scope.
> B) Implement a general mechanism for async hcalls that is *not* tied
> to DRCs. Then use that for the existing H_RESIZE_HPT_PREPARE call as
> well as this new one.
>
>> Hope you are okay with using the pool based approach that Greg
> Honestly a thread pool seems like it might be overkill for this
> application.
I think its appropriate here as that is what is being done by virtio-pmem
too for flush requests. The aio infrastructure simplifies lot of the
thread handling usage. Please suggest if you think there are better ways.
I am sending the next version addressing all the comments from you and Greg.
Thanks,
Shivaprasad
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level
@ 2021-03-23 7:53 ` Shivaprasad G Bhat
0 siblings, 0 replies; 33+ messages in thread
From: Shivaprasad G Bhat @ 2021-03-23 7:54 UTC (permalink / raw)
To: David Gibson
Cc: Greg Kurz, xiaoguangrong.eric, mst, imammedo, qemu-devel,
qemu-ppc, linux-nvdimm, aneesh.kumar, kvm-ppc, shivaprasadbhat,
bharata, linuxppc-dev
Hi David,
Sorry about the delay.
On 2/8/21 11:51 AM, David Gibson wrote:
> On Tue, Jan 19, 2021 at 12:40:31PM +0530, Shivaprasad G Bhat wrote:
>> Thanks for the comments!
>>
>>
>> On 12/28/20 2:08 PM, David Gibson wrote:
>>
>>> On Mon, Dec 21, 2020 at 01:08:53PM +0100, Greg Kurz wrote:
>> ...
>>>> The overall idea looks good but I think you should consider using
>>>> a thread pool to implement it. See below.
>>> I am not convinced, however. Specifically, attaching this to the DRC
>>> doesn't make sense to me. We're adding exactly one DRC related async
>>> hcall, and I can't really see much call for another one. We could
>>> have other async hcalls - indeed we already have one for HPT resizing
>>> - but attaching this to DRCs doesn't help for those.
>> The semantics of the hcall made me think, if this is going to be
>> re-usable for future if implemented at DRC level.
> It would only be re-usable for operations that are actually connected
> to DRCs. It doesn't seem to me particularly likely that we'll ever
> have more asynchronous hcalls that are also associated with DRCs.
Okay
>> Other option
>> is to move the async-hcall-state/list into the NVDIMMState structure
>> in include/hw/mem/nvdimm.h and handle it with machine->nvdimms_state
>> at a global level.
> I'm ok with either of two options:
>
> A) Implement this ad-hoc for this specific case, making whatever
> simplifications you can based on this specific case.
I am simplifying it to nvdimm use-case alone and limiting the scope.
> B) Implement a general mechanism for async hcalls that is *not* tied
> to DRCs. Then use that for the existing H_RESIZE_HPT_PREPARE call as
> well as this new one.
>
>> Hope you are okay with using the pool based approach that Greg
> Honestly a thread pool seems like it might be overkill for this
> application.
I think its appropriate here as that is what is being done by virtio-pmem
too for flush requests. The aio infrastructure simplifies lot of the
thread handling usage. Please suggest if you think there are better ways.
I am sending the next version addressing all the comments from you and Greg.
Thanks,
Shivaprasad
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2021-03-23 7:54 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-30 15:16 [RFC Qemu PATCH v2 0/2] spapr: nvdimm: Asynchronus flush hcall support Shivaprasad G Bhat
2020-11-30 15:16 ` Shivaprasad G Bhat
2020-11-30 15:16 ` Shivaprasad G Bhat
2020-11-30 15:16 ` [RFC Qemu PATCH v2 1/2] spapr: drc: Add support for async hcalls at the drc level Shivaprasad G Bhat
2020-11-30 15:16 ` Shivaprasad G Bhat
2020-11-30 15:16 ` Shivaprasad G Bhat
2020-12-21 12:08 ` Greg Kurz
2020-12-21 12:08 ` Greg Kurz
2020-12-21 12:08 ` Greg Kurz
2020-12-21 14:37 ` Greg Kurz
2020-12-21 14:37 ` Greg Kurz
2020-12-21 14:37 ` Greg Kurz
2020-12-28 8:38 ` David Gibson
2020-12-28 8:38 ` David Gibson
2020-12-28 8:38 ` David Gibson
2021-01-19 7:10 ` Shivaprasad G Bhat
2021-01-19 7:22 ` Shivaprasad G Bhat
2021-01-19 7:10 ` Shivaprasad G Bhat
2021-02-08 6:21 ` David Gibson
2021-02-08 6:21 ` David Gibson
2021-02-08 6:21 ` David Gibson
2021-03-23 7:53 ` Shivaprasad G Bhat
2021-03-23 7:54 ` Shivaprasad G Bhat
2021-03-23 7:53 ` Shivaprasad G Bhat
2020-11-30 15:17 ` [RFC Qemu PATCH v2 2/2] spapr: nvdimm: Implement async flush hcalls Shivaprasad G Bhat
2020-11-30 15:17 ` Shivaprasad G Bhat
2020-11-30 15:17 ` Shivaprasad G Bhat
2020-12-21 13:07 ` Greg Kurz
2020-12-21 13:07 ` Greg Kurz
2020-12-21 13:07 ` Greg Kurz
2020-12-21 13:07 ` [RFC Qemu PATCH v2 0/2] spapr: nvdimm: Asynchronus flush hcall support Greg Kurz
2020-12-21 13:07 ` Greg Kurz
2020-12-21 13:07 ` Greg Kurz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.