Re: [PATCH v1 04/24] vfio-user: add region cache
> On Dec 12, 2022, at 3:42 AM, Philippe Mathieu-Daudé wrote:
>
> On 9/11/22 00:13, John Johnson wrote:
>> cache VFIO_DEVICE_GET_REGION_INFO results to reduce
>> memory alloc/free cycles and as prep work for vfio-user
>> Signed-off-by: John G Johnson
>> Signed-off-by: Elena Ufimtseva
>> Signed-off-by: Jagannathan Raman
>> ---
>> hw/vfio/ccw.c | 5 -
>> hw/vfio/common.c | 41 +++--
>> hw/vfio/igd.c | 23 +--
>> hw/vfio/migration.c | 2 --
>> hw/vfio/pci-quirks.c | 19 +--
>> hw/vfio/pci.c | 8
>> include/hw/vfio/vfio-common.h | 2 ++
>> 7 files changed, 51 insertions(+), 49 deletions(-)
>
>
>> void vfio_put_base_device(VFIODevice *vbasedev)
>> {
>> +if (vbasedev->regions != NULL) {
>> +int i;
>> +
>> +for (i = 0; i < vbasedev->num_regions; i++) {
>> +g_free(vbasedev->regions[i]);
>> +}
>> +g_free(vbasedev->regions);
>> +vbasedev->regions = NULL;
>> +}
>> +
>> if (!vbasedev->group) {
>> return;
>> }
>> @@ -2432,6 +2451,17 @@ int vfio_get_region_info(VFIODevice *vbasedev, int
>> index,
>> {
>> size_t argsz = sizeof(struct vfio_region_info);
>> +/* create region cache */
>> +if (vbasedev->regions == NULL) {
>> +vbasedev->regions = g_new0(struct vfio_region_info *,
>> + vbasedev->num_regions);
>> +}
>> +/* check cache */
>> +if (vbasedev->regions[index] != NULL) {
>> +*info = vbasedev->regions[index];
>> +return 0;
>> +}
>
> What about simply allocating/releasing once regions[] in
> vfio_instance_init / vfio_instance_finalize?
I think this is done already, except with _realize and
_instance_finalize.
e.g,, vfio_realize() -> vfio_get_device() -> vfio_get_all_regions allocates
and
vfio_instance_finalize -> vfio_put_device -> vfio_put_base_device deallocates
JJ
Re: [PATCH v1 04/24] vfio-user: add region cache
On 9/11/22 00:13, John Johnson wrote:
cache VFIO_DEVICE_GET_REGION_INFO results to reduce
memory alloc/free cycles and as prep work for vfio-user
Signed-off-by: John G Johnson
Signed-off-by: Elena Ufimtseva
Signed-off-by: Jagannathan Raman
---
hw/vfio/ccw.c | 5 -
hw/vfio/common.c | 41 +++--
hw/vfio/igd.c | 23 +--
hw/vfio/migration.c | 2 --
hw/vfio/pci-quirks.c | 19 +--
hw/vfio/pci.c | 8
include/hw/vfio/vfio-common.h | 2 ++
7 files changed, 51 insertions(+), 49 deletions(-)
void vfio_put_base_device(VFIODevice *vbasedev)
{
+if (vbasedev->regions != NULL) {
+int i;
+
+for (i = 0; i < vbasedev->num_regions; i++) {
+g_free(vbasedev->regions[i]);
+}
+g_free(vbasedev->regions);
+vbasedev->regions = NULL;
+}
+
if (!vbasedev->group) {
return;
}
@@ -2432,6 +2451,17 @@ int vfio_get_region_info(VFIODevice *vbasedev, int index,
{
size_t argsz = sizeof(struct vfio_region_info);
+/* create region cache */
+if (vbasedev->regions == NULL) {
+vbasedev->regions = g_new0(struct vfio_region_info *,
+ vbasedev->num_regions);
+}
+/* check cache */
+if (vbasedev->regions[index] != NULL) {
+*info = vbasedev->regions[index];
+return 0;
+}
What about simply allocating/releasing once regions[] in
vfio_instance_init / vfio_instance_finalize?
Re: [PATCH v1 04/24] vfio-user: add region cache
On 11/9/22 00:13, John Johnson wrote:
cache VFIO_DEVICE_GET_REGION_INFO results to reduce
memory alloc/free cycles and as prep work for vfio-user
Signed-off-by: John G Johnson
Signed-off-by: Elena Ufimtseva
Signed-off-by: Jagannathan Raman
LGTM,
Reviewed-by: Cédric Le Goater
Thanks,
C.
---
hw/vfio/ccw.c | 5 -
hw/vfio/common.c | 41 +++--
hw/vfio/igd.c | 23 +--
hw/vfio/migration.c | 2 --
hw/vfio/pci-quirks.c | 19 +--
hw/vfio/pci.c | 8
include/hw/vfio/vfio-common.h | 2 ++
7 files changed, 51 insertions(+), 49 deletions(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 0354737..06b588c 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -517,7 +517,6 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
vcdev->io_region_offset = info->offset;
vcdev->io_region = g_malloc0(info->size);
-g_free(info);
/* check for the optional async command region */
ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
@@ -530,7 +529,6 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
}
vcdev->async_cmd_region_offset = info->offset;
vcdev->async_cmd_region = g_malloc0(info->size);
-g_free(info);
}
ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
@@ -543,7 +541,6 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
}
vcdev->schib_region_offset = info->offset;
vcdev->schib_region = g_malloc(info->size);
-g_free(info);
}
ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
@@ -557,7 +554,6 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
}
vcdev->crw_region_offset = info->offset;
vcdev->crw_region = g_malloc(info->size);
-g_free(info);
}
return;
@@ -567,7 +563,6 @@ out_err:
g_free(vcdev->schib_region);
g_free(vcdev->async_cmd_region);
g_free(vcdev->io_region);
-g_free(info);
return;
}
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 83d69b9..dd9104f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1600,8 +1600,6 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev,
VFIORegion *region,
}
}
-g_free(info);
-
trace_vfio_region_setup(vbasedev->name, index, name,
region->flags, region->fd_offset, region->size);
return 0;
@@ -2357,6 +2355,16 @@ void vfio_put_group(VFIOGroup *group)
}
}
+void vfio_get_all_regions(VFIODevice *vbasedev)
+{
+struct vfio_region_info *info;
+int i;
+
+for (i = 0; i < vbasedev->num_regions; i++) {
+vfio_get_region_info(vbasedev, i, );
+}
+}
+
int vfio_get_device(VFIOGroup *group, const char *name,
VFIODevice *vbasedev, Error **errp)
{
@@ -2412,12 +2420,23 @@ int vfio_get_device(VFIOGroup *group, const char *name,
trace_vfio_get_device(name, dev_info.flags, dev_info.num_regions,
dev_info.num_irqs);
+vfio_get_all_regions(vbasedev);
vbasedev->reset_works = !!(dev_info.flags & VFIO_DEVICE_FLAGS_RESET);
return 0;
}
void vfio_put_base_device(VFIODevice *vbasedev)
{
+if (vbasedev->regions != NULL) {
+int i;
+
+for (i = 0; i < vbasedev->num_regions; i++) {
+g_free(vbasedev->regions[i]);
+}
+g_free(vbasedev->regions);
+vbasedev->regions = NULL;
+}
+
if (!vbasedev->group) {
return;
}
@@ -2432,6 +2451,17 @@ int vfio_get_region_info(VFIODevice *vbasedev, int index,
{
size_t argsz = sizeof(struct vfio_region_info);
+/* create region cache */
+if (vbasedev->regions == NULL) {
+vbasedev->regions = g_new0(struct vfio_region_info *,
+ vbasedev->num_regions);
+}
+/* check cache */
+if (vbasedev->regions[index] != NULL) {
+*info = vbasedev->regions[index];
+return 0;
+}
+
*info = g_malloc0(argsz);
(*info)->index = index;
@@ -2451,6 +2481,9 @@ retry:
goto retry;
}
+/* fill cache */
+vbasedev->regions[index] = *info;
+
return 0;
}
@@ -2469,7 +2502,6 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
hdr = vfio_get_region_info_cap(*info, VFIO_REGION_INFO_CAP_TYPE);
if (!hdr) {
-g_free(*info);
continue;
}
@@ -2481,8 +2513,6 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev, uint32_t type,
if (cap_type->type == type && cap_type->subtype == subtype) {
return 0;
}
-
-g_free(*info);
}
*info = NULL;
@@ -2498,7 +2528,6 @@ bool
Re: [PATCH v1 04/24] vfio-user: add region cache
On Tue, Nov 08, 2022 at 03:13:26PM -0800, John Johnson wrote: > cache VFIO_DEVICE_GET_REGION_INFO results to reduce > memory alloc/free cycles and as prep work for vfio-user LGTM Reviewed-by: John Levon regards john
[PATCH v1 04/24] vfio-user: add region cache
cache VFIO_DEVICE_GET_REGION_INFO results to reduce
memory alloc/free cycles and as prep work for vfio-user
Signed-off-by: John G Johnson
Signed-off-by: Elena Ufimtseva
Signed-off-by: Jagannathan Raman
---
hw/vfio/ccw.c | 5 -
hw/vfio/common.c | 41 +++--
hw/vfio/igd.c | 23 +--
hw/vfio/migration.c | 2 --
hw/vfio/pci-quirks.c | 19 +--
hw/vfio/pci.c | 8
include/hw/vfio/vfio-common.h | 2 ++
7 files changed, 51 insertions(+), 49 deletions(-)
diff --git a/hw/vfio/ccw.c b/hw/vfio/ccw.c
index 0354737..06b588c 100644
--- a/hw/vfio/ccw.c
+++ b/hw/vfio/ccw.c
@@ -517,7 +517,6 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
vcdev->io_region_offset = info->offset;
vcdev->io_region = g_malloc0(info->size);
-g_free(info);
/* check for the optional async command region */
ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
@@ -530,7 +529,6 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
}
vcdev->async_cmd_region_offset = info->offset;
vcdev->async_cmd_region = g_malloc0(info->size);
-g_free(info);
}
ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
@@ -543,7 +541,6 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
}
vcdev->schib_region_offset = info->offset;
vcdev->schib_region = g_malloc(info->size);
-g_free(info);
}
ret = vfio_get_dev_region_info(vdev, VFIO_REGION_TYPE_CCW,
@@ -557,7 +554,6 @@ static void vfio_ccw_get_region(VFIOCCWDevice *vcdev, Error
**errp)
}
vcdev->crw_region_offset = info->offset;
vcdev->crw_region = g_malloc(info->size);
-g_free(info);
}
return;
@@ -567,7 +563,6 @@ out_err:
g_free(vcdev->schib_region);
g_free(vcdev->async_cmd_region);
g_free(vcdev->io_region);
-g_free(info);
return;
}
diff --git a/hw/vfio/common.c b/hw/vfio/common.c
index 83d69b9..dd9104f 100644
--- a/hw/vfio/common.c
+++ b/hw/vfio/common.c
@@ -1600,8 +1600,6 @@ int vfio_region_setup(Object *obj, VFIODevice *vbasedev,
VFIORegion *region,
}
}
-g_free(info);
-
trace_vfio_region_setup(vbasedev->name, index, name,
region->flags, region->fd_offset, region->size);
return 0;
@@ -2357,6 +2355,16 @@ void vfio_put_group(VFIOGroup *group)
}
}
+void vfio_get_all_regions(VFIODevice *vbasedev)
+{
+struct vfio_region_info *info;
+int i;
+
+for (i = 0; i < vbasedev->num_regions; i++) {
+vfio_get_region_info(vbasedev, i, );
+}
+}
+
int vfio_get_device(VFIOGroup *group, const char *name,
VFIODevice *vbasedev, Error **errp)
{
@@ -2412,12 +2420,23 @@ int vfio_get_device(VFIOGroup *group, const char *name,
trace_vfio_get_device(name, dev_info.flags, dev_info.num_regions,
dev_info.num_irqs);
+vfio_get_all_regions(vbasedev);
vbasedev->reset_works = !!(dev_info.flags & VFIO_DEVICE_FLAGS_RESET);
return 0;
}
void vfio_put_base_device(VFIODevice *vbasedev)
{
+if (vbasedev->regions != NULL) {
+int i;
+
+for (i = 0; i < vbasedev->num_regions; i++) {
+g_free(vbasedev->regions[i]);
+}
+g_free(vbasedev->regions);
+vbasedev->regions = NULL;
+}
+
if (!vbasedev->group) {
return;
}
@@ -2432,6 +2451,17 @@ int vfio_get_region_info(VFIODevice *vbasedev, int index,
{
size_t argsz = sizeof(struct vfio_region_info);
+/* create region cache */
+if (vbasedev->regions == NULL) {
+vbasedev->regions = g_new0(struct vfio_region_info *,
+ vbasedev->num_regions);
+}
+/* check cache */
+if (vbasedev->regions[index] != NULL) {
+*info = vbasedev->regions[index];
+return 0;
+}
+
*info = g_malloc0(argsz);
(*info)->index = index;
@@ -2451,6 +2481,9 @@ retry:
goto retry;
}
+/* fill cache */
+vbasedev->regions[index] = *info;
+
return 0;
}
@@ -2469,7 +2502,6 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev,
uint32_t type,
hdr = vfio_get_region_info_cap(*info, VFIO_REGION_INFO_CAP_TYPE);
if (!hdr) {
-g_free(*info);
continue;
}
@@ -2481,8 +2513,6 @@ int vfio_get_dev_region_info(VFIODevice *vbasedev,
uint32_t type,
if (cap_type->type == type && cap_type->subtype == subtype) {
return 0;
}
-
-g_free(*info);
}
*info = NULL;
@@ -2498,7 +2528,6 @@ bool vfio_has_region_cap(VFIODevice *vbasedev, int
region, uint16_t cap_type)
if (vfio_get_region_info_cap(info, cap_type)) {
ret = true;
}
-g_free(info);
}
