Update the optional mdev attributes by running an mdevctl update on a
new created nodedev object representing an mdev.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158
Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
---
src/node_device/node_device_udev.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 4c37ec3189..fce4212728 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1757,12 +1757,20 @@ nodeStateCleanup(void)
static int
udevHandleOneDevice(struct udev_device *device)
{
+ virNodeDevCapType dev_cap_type;
const char *action = udev_device_get_action(device);
VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device));
- if (STREQ(action, "add") || STREQ(action, "change"))
- return udevAddOneDevice(device);
+ if (STREQ(action, "add") || STREQ(action, "change")) {
+ int ret = udevAddOneDevice(device);
+ if (ret == 0 &&
+ udevGetDeviceType(device, &dev_cap_type) == 0 &&
+ dev_cap_type == VIR_NODE_DEV_CAP_MDEV)
+ if (nodeDeviceUpdateMediatedDevices() < 0)
+ VIR_WARN("mdevctl failed to update mediated devices");
+ return ret;
+ }
if (STREQ(action, "remove"))
return udevRemoveOneDevice(device);
--
2.41.0On 6/30/23 6:34 AM, Boris Fiuczynski wrote:
> Update the optional mdev attributes by running an mdevctl update on a
> new created nodedev object representing an mdev.
>
> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158
> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
> ---
> src/node_device/node_device_udev.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index 4c37ec3189..fce4212728 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -1757,12 +1757,20 @@ nodeStateCleanup(void)
> static int
> udevHandleOneDevice(struct udev_device *device)
> {
> + virNodeDevCapType dev_cap_type;
> const char *action = udev_device_get_action(device);
>
> VIR_DEBUG("udev action: '%s': %s", action, udev_device_get_syspath(device));
>
> - if (STREQ(action, "add") || STREQ(action, "change"))
> - return udevAddOneDevice(device);
> + if (STREQ(action, "add") || STREQ(action, "change")) {
> + int ret = udevAddOneDevice(device);
> + if (ret == 0 &&
> + udevGetDeviceType(device, &dev_cap_type) == 0 &&
> + dev_cap_type == VIR_NODE_DEV_CAP_MDEV)
> + if (nodeDeviceUpdateMediatedDevices() < 0)
> + VIR_WARN("mdevctl failed to update mediated devices");
> + return ret;
> + }
>
> if (STREQ(action, "remove"))
> return udevRemoveOneDevice(device);
So, this should work and roughly matches what we've done for other
similar cases. But this is running from within the udev event handler
thread, and theoretically there could already be an mdevctl thread
running concurrently and updating the internal device list. The device
list update functions are thread-safe so I don't think it'll corrupt
anything, but it seems better to do all mdevctl updates from mdevctl
update thread rather than calling it directly here. The same actually
applies to a couple other existing calls to
nodeDeviceUpdateMediatedDevices(). I'll send a couple patches that apply
on top of yours and refactor things a bit. Let me know what you think.
Jonathon
move mdevctl updates to the mdevctl thread. These two patches apply on top of Boris's patch: "[PATCH v2] nodedev: transient mdev update on nodeDeviceCreateXML" Jonathon Jongsma (2): nodedev: refactor mdevctl thread functions nodedev: update mdevs from the mdevctl thread src/node_device/node_device_udev.c | 50 +++++++++++++++++++----------- 1 file changed, 32 insertions(+), 18 deletions(-) -- 2.41.0
Factor out a new scheduleMdevctlUpdate() function so that we can re-use
it from other places. Now that other events can make it necessary to
re-query mdevctl for mdev updates, this function will be useful for
coalescing multiple updates in quick succession into a single mdevctl
query.
Also rename a couple functions. The names weren't very descriptive of
their behavior. For example, the old scheduleMdevctlHandler() function
didn't actually schedule anything, it just started a thread. So rename
it to free up the 'schedule' name for the above refactored function.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
src/node_device/node_device_udev.c | 37 ++++++++++++++++++++----------
1 file changed, 25 insertions(+), 12 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index fce4212728..9ba550dbc1 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -2075,7 +2075,7 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
static void
-mdevctlHandlerThread(void *opaque G_GNUC_UNUSED)
+mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED)
{
udevEventData *priv = driver->privateData;
VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock);
@@ -2085,8 +2085,9 @@ mdevctlHandlerThread(void *opaque G_GNUC_UNUSED)
}
+
static void
-scheduleMdevctlHandler(int timer G_GNUC_UNUSED, void *opaque)
+launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque)
{
udevEventData *priv = opaque;
virThread thread;
@@ -2096,7 +2097,7 @@ scheduleMdevctlHandler(int timer G_GNUC_UNUSED, void *opaque)
priv->mdevctlTimeout = -1;
}
- if (virThreadCreateFull(&thread, false, mdevctlHandlerThread,
+ if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc,
"mdevctl-thread", false, NULL) < 0) {
virReportSystemError(errno, "%s",
_("failed to create mdevctl thread"));
@@ -2192,6 +2193,26 @@ mdevctlEnableMonitor(udevEventData *priv)
}
+/* Schedules an mdevctl update for 100ms in the future, canceling any existing
+ * timeout that may have been set. In this way, multiple update requests in
+ * quick succession can be collapsed into a single update. if @force is true,
+ * an update thread will be spawned immediately. */
+static void
+scheduleMdevctlUpdate(udevEventData *data,
+ bool force)
+{
+ if (!force) {
+ if (data->mdevctlTimeout > 0)
+ virEventRemoveTimeout(data->mdevctlTimeout);
+ data->mdevctlTimeout = virEventAddTimeout(100, launchMdevctlUpdateThread,
+ data, NULL);
+ return;
+ }
+
+ launchMdevctlUpdateThread(-1, data);
+}
+
+
static void
mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED,
GFile *file,
@@ -2222,15 +2243,7 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED,
* configuration change, try to coalesce these changes by waiting for the
* CHANGES_DONE_HINT event. As a fallback, add a timeout to trigger the
* signal if that event never comes */
- if (event_type != G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT) {
- if (priv->mdevctlTimeout > 0)
- virEventRemoveTimeout(priv->mdevctlTimeout);
- priv->mdevctlTimeout = virEventAddTimeout(100, scheduleMdevctlHandler,
- priv, NULL);
- return;
- }
-
- scheduleMdevctlHandler(-1, priv);
+ scheduleMdevctlUpdate(priv, (event_type == G_FILE_MONITOR_EVENT_CHANGES_DONE_HINT));
}
--
2.41.0On 7/6/23 9:44 PM, Jonathon Jongsma wrote:
> Factor out a new scheduleMdevctlUpdate() function so that we can re-use
> it from other places. Now that other events can make it necessary to
> re-query mdevctl for mdev updates, this function will be useful for
> coalescing multiple updates in quick succession into a single mdevctl
> query.
>
> Also rename a couple functions. The names weren't very descriptive of
> their behavior. For example, the old scheduleMdevctlHandler() function
> didn't actually schedule anything, it just started a thread. So rename
> it to free up the 'schedule' name for the above refactored function.
>
> Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
> ---
> src/node_device/node_device_udev.c | 37 ++++++++++++++++++++----------
> 1 file changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
> index fce4212728..9ba550dbc1 100644
> --- a/src/node_device/node_device_udev.c
> +++ b/src/node_device/node_device_udev.c
> @@ -2075,7 +2075,7 @@ udevPCITranslateInit(bool privileged G_GNUC_UNUSED)
>
>
> static void
> -mdevctlHandlerThread(void *opaque G_GNUC_UNUSED)
> +mdevctlUpdateThreadFunc(void *opaque G_GNUC_UNUSED)
> {
> udevEventData *priv = driver->privateData;
> VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock);
> @@ -2085,8 +2085,9 @@ mdevctlHandlerThread(void *opaque G_GNUC_UNUSED)
> }
>
>
> +
The above additional line should be removed.
> static void
> -scheduleMdevctlHandler(int timer G_GNUC_UNUSED, void *opaque)
> +launchMdevctlUpdateThread(int timer G_GNUC_UNUSED, void *opaque)
> {
> udevEventData *priv = opaque;
> virThread thread;
> @@ -2096,7 +2097,7 @@ scheduleMdevctlHandler(int timer G_GNUC_UNUSED, void *opaque)
> priv->mdevctlTimeout = -1;
> }
>
> - if (virThreadCreateFull(&thread, false, mdevctlHandlerThread,
> + if (virThreadCreateFull(&thread, false, mdevctlUpdateThreadFunc,
> "mdevctl-thread", false, NULL) < 0) {
> virReportSystemError(errno, "%s",
> _("failed to create mdevctl thread"));
> @@ -2192,6 +2193,26 @@ mdevctlEnableMonitor(udevEventData *priv)
> }
<snip>
Beside the nit above
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Rather than directly executing mdevctl from the udev event thread when
we determine that we need to re-query, schedule the mdevctl thread to
run. This also helps to coalesce multiple back-to-back updates into a
single one when there are multiple updates in a row or at startup when a
host has a very large number of mdevs.
Signed-off-by: Jonathon Jongsma <jjongsma@redhat.com>
---
src/node_device/node_device_udev.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c
index 9ba550dbc1..da63d326a1 100644
--- a/src/node_device/node_device_udev.c
+++ b/src/node_device/node_device_udev.c
@@ -1443,6 +1443,9 @@ udevGetDeviceDetails(struct udev_device *device,
}
+static void scheduleMdevctlUpdate(udevEventData *data, bool force);
+
+
static int
udevRemoveOneDeviceSysPath(const char *path)
{
@@ -1475,8 +1478,7 @@ udevRemoveOneDeviceSysPath(const char *path)
virNodeDeviceObjEndAPI(&obj);
/* cannot check for mdev_types since they have already been removed */
- if (nodeDeviceUpdateMediatedDevices() < 0)
- VIR_WARN("mdevctl failed to update mediated devices");
+ scheduleMdevctlUpdate(driver->privateData, false);
virObjectEventStateQueue(driver->nodeDeviceEventState, event);
return 0;
@@ -1604,8 +1606,8 @@ udevAddOneDevice(struct udev_device *device)
has_mdev_types = virNodeDeviceObjHasCap(obj, VIR_NODE_DEV_CAP_MDEV_TYPES);
virNodeDeviceObjEndAPI(&obj);
- if (has_mdev_types && nodeDeviceUpdateMediatedDevices() < 0)
- VIR_WARN("mdevctl failed to update mediated devices");
+ if (has_mdev_types)
+ scheduleMdevctlUpdate(driver->privateData, false);
ret = 0;
@@ -1767,8 +1769,7 @@ udevHandleOneDevice(struct udev_device *device)
if (ret == 0 &&
udevGetDeviceType(device, &dev_cap_type) == 0 &&
dev_cap_type == VIR_NODE_DEV_CAP_MDEV)
- if (nodeDeviceUpdateMediatedDevices() < 0)
- VIR_WARN("mdevctl failed to update mediated devices");
+ scheduleMdevctlUpdate(driver->privateData, false);
return ret;
}
--
2.41.0On 7/6/23 9:44 PM, Jonathon Jongsma wrote:
> Rather than directly executing mdevctl from the udev event thread when
> we determine that we need to re-query, schedule the mdevctl thread to
> run. This also helps to coalesce multiple back-to-back updates into a
> single one when there are multiple updates in a row or at startup when a
> host has a very large number of mdevs.
>
> Signed-off-by: Jonathon Jongsma<jjongsma@redhat.com>
> ---
> src/node_device/node_device_udev.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
LGTM and works for vfio-ap mdevs (therefore for all other types as well).
Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
On 7/6/23 9:40 PM, Jonathon Jongsma wrote:
> On 6/30/23 6:34 AM, Boris Fiuczynski wrote:
>> Update the optional mdev attributes by running an mdevctl update on a
>> new created nodedev object representing an mdev.
>>
>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158
>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>> ---
>> src/node_device/node_device_udev.c | 12 ++++++++++--
>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/node_device/node_device_udev.c
>> b/src/node_device/node_device_udev.c
>> index 4c37ec3189..fce4212728 100644
>> --- a/src/node_device/node_device_udev.c
>> +++ b/src/node_device/node_device_udev.c
>> @@ -1757,12 +1757,20 @@ nodeStateCleanup(void)
>> static int
>> udevHandleOneDevice(struct udev_device *device)
>> {
>> + virNodeDevCapType dev_cap_type;
>> const char *action = udev_device_get_action(device);
>> VIR_DEBUG("udev action: '%s': %s", action,
>> udev_device_get_syspath(device));
>> - if (STREQ(action, "add") || STREQ(action, "change"))
>> - return udevAddOneDevice(device);
>> + if (STREQ(action, "add") || STREQ(action, "change")) {
>> + int ret = udevAddOneDevice(device);
>> + if (ret == 0 &&
>> + udevGetDeviceType(device, &dev_cap_type) == 0 &&
>> + dev_cap_type == VIR_NODE_DEV_CAP_MDEV)
>> + if (nodeDeviceUpdateMediatedDevices() < 0)
>> + VIR_WARN("mdevctl failed to update mediated devices");
>> + return ret;
>> + }
>> if (STREQ(action, "remove"))
>> return udevRemoveOneDevice(device);
>
>
> So, this should work and roughly matches what we've done for other
> similar cases. But this is running from within the udev event handler
> thread, and theoretically there could already be an mdevctl thread
> running concurrently and updating the internal device list. The device
> list update functions are thread-safe so I don't think it'll corrupt
> anything, but it seems better to do all mdevctl updates from mdevctl
> update thread rather than calling it directly here. The same actually
> applies to a couple other existing calls to
> nodeDeviceUpdateMediatedDevices(). I'll send a couple patches that apply
> on top of yours and refactor things a bit. Let me know what you think.
>
> Jonathon
>
LGTM and testing was successful.
See my direct replies on the patches.
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
Polite ping
On 7/7/23 11:17 AM, Boris Fiuczynski wrote:
> On 7/6/23 9:40 PM, Jonathon Jongsma wrote:
>> On 6/30/23 6:34 AM, Boris Fiuczynski wrote:
>>> Update the optional mdev attributes by running an mdevctl update on a
>>> new created nodedev object representing an mdev.
>>>
>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158
>>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>>> ---
>>> src/node_device/node_device_udev.c | 12 ++++++++++--
>>> 1 file changed, 10 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/node_device/node_device_udev.c
>>> b/src/node_device/node_device_udev.c
>>> index 4c37ec3189..fce4212728 100644
>>> --- a/src/node_device/node_device_udev.c
>>> +++ b/src/node_device/node_device_udev.c
>>> @@ -1757,12 +1757,20 @@ nodeStateCleanup(void)
>>> static int
>>> udevHandleOneDevice(struct udev_device *device)
>>> {
>>> + virNodeDevCapType dev_cap_type;
>>> const char *action = udev_device_get_action(device);
>>> VIR_DEBUG("udev action: '%s': %s", action,
>>> udev_device_get_syspath(device));
>>> - if (STREQ(action, "add") || STREQ(action, "change"))
>>> - return udevAddOneDevice(device);
>>> + if (STREQ(action, "add") || STREQ(action, "change")) {
>>> + int ret = udevAddOneDevice(device);
>>> + if (ret == 0 &&
>>> + udevGetDeviceType(device, &dev_cap_type) == 0 &&
>>> + dev_cap_type == VIR_NODE_DEV_CAP_MDEV)
>>> + if (nodeDeviceUpdateMediatedDevices() < 0)
>>> + VIR_WARN("mdevctl failed to update mediated devices");
>>> + return ret;
>>> + }
>>> if (STREQ(action, "remove"))
>>> return udevRemoveOneDevice(device);
>>
>>
>> So, this should work and roughly matches what we've done for other
>> similar cases. But this is running from within the udev event handler
>> thread, and theoretically there could already be an mdevctl thread
>> running concurrently and updating the internal device list. The device
>> list update functions are thread-safe so I don't think it'll corrupt
>> anything, but it seems better to do all mdevctl updates from mdevctl
>> update thread rather than calling it directly here. The same actually
>> applies to a couple other existing calls to
>> nodeDeviceUpdateMediatedDevices(). I'll send a couple patches that
>> apply on top of yours and refactor things a bit. Let me know what you
>> think.
>>
>> Jonathon
>>
>
> LGTM and testing was successful.
> See my direct replies on the patches.
>
>
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
On 7/17/23 10:12 AM, Boris Fiuczynski wrote:
> Polite ping
>
> On 7/7/23 11:17 AM, Boris Fiuczynski wrote:
>> On 7/6/23 9:40 PM, Jonathon Jongsma wrote:
>>> On 6/30/23 6:34 AM, Boris Fiuczynski wrote:
>>>> Update the optional mdev attributes by running an mdevctl update on a
>>>> new created nodedev object representing an mdev.
>>>>
>>>> Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2143158
>>>> Signed-off-by: Boris Fiuczynski <fiuczy@linux.ibm.com>
>>>> ---
Just found that it was push to master last Thursday. Thanks.
--
Mit freundlichen Grüßen/Kind regards
Boris Fiuczynski
IBM Deutschland Research & Development GmbH
Vorsitzender des Aufsichtsrats: Gregor Pillen
Geschäftsführung: David Faller
Sitz der Gesellschaft: Böblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294
© 2016 - 2024 Red Hat, Inc.