In 'virResctrlAllocUpdateMask', mask is updated only if 'previous mask' is NULL.

By default, the bitmask for a cache resource for a VM is initialized with
'default-resctrl-group' bitmask. So the 'previous mask' would not be NULL and
mask won't get updated if cachetune is configured for a VM. This causes libvirt
to use same bitmask as 'default-resctrl-group' bitmask for a cache resource for
a VM. This patch fixes the issue.

Fixes: d8a354954aba9cd45ab0317915a0a2be27c04767

Signed-off-by: Vinayak Kale <vkale@nvidia.com>
---
 src/util/virresctrl.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
index 35e92022db..a7d36f492c 100644
--- a/src/util/virresctrl.c
+++ b/src/util/virresctrl.c
@@ -1104,8 +1104,10 @@ virResctrlAllocUpdateMask(virResctrlAlloc *alloc,
         VIR_EXPAND_N(a_type->masks, a_type->nmasks,
                      cache - a_type->nmasks + 1);
 
-    if (!a_type->masks[cache])
-        a_type->masks[cache] = virBitmapNewCopy(mask);
+    if (a_type->masks[cache])
+        virBitmapFree(a_type->masks[cache]);
+
+    a_type->masks[cache] = virBitmapNewCopy(mask);
 
     return 0;
 }
-- 
2.17.1

On 7/2/21 4:23 AM, Vinayak Kale wrote:
> In 'virResctrlAllocUpdateMask', mask is updated only if 'previous mask' is NULL.
> 
> By default, the bitmask for a cache resource for a VM is initialized with
> 'default-resctrl-group' bitmask. So the 'previous mask' would not be NULL and
> mask won't get updated if cachetune is configured for a VM. This causes libvirt
> to use same bitmask as 'default-resctrl-group' bitmask for a cache resource for
> a VM. This patch fixes the issue.
> 
> Fixes: d8a354954aba9cd45ab0317915a0a2be27c04767
> 
> Signed-off-by: Vinayak Kale <vkale@nvidia.com>
> ---

I was going to suggest whether cachetune couldn't just overwrite the default
mask if it's configured, but that would just shift the problem somewhere else.

This fix seems adequate.

Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>



>   src/util/virresctrl.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c
> index 35e92022db..a7d36f492c 100644
> --- a/src/util/virresctrl.c
> +++ b/src/util/virresctrl.c
> @@ -1104,8 +1104,10 @@ virResctrlAllocUpdateMask(virResctrlAlloc *alloc,
>           VIR_EXPAND_N(a_type->masks, a_type->nmasks,
>                        cache - a_type->nmasks + 1);
>   
> -    if (!a_type->masks[cache])
> -        a_type->masks[cache] = virBitmapNewCopy(mask);
> +    if (a_type->masks[cache])
> +        virBitmapFree(a_type->masks[cache]);
> +
> +    a_type->masks[cache] = virBitmapNewCopy(mask);
>   
>       return 0;
>   }
> 

On a %A in %Y, Vinayak Kale wrote:
> In 'virResctrlAllocUpdateMask', mask is updated only if 'previous mask' is NULL.
> 
> By default, the bitmask for a cache resource for a VM is initialized with
> 'default-resctrl-group' bitmask. So the 'previous mask' would not be NULL and
> mask won't get updated if cachetune is configured for a VM. This causes libvirt
> to use same bitmask as 'default-resctrl-group' bitmask for a cache resource for
> a VM. This patch fixes the issue.
>> Fixes: d8a354954aba9cd45ab0317915a0a2be27c04767

Thanks for catching this, it seems to be the only such case in that commit.

> 
> Signed-off-by: Vinayak Kale <vkale@nvidia.com>
> ---
>  src/util/virresctrl.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

Reviewed-by: Ján Tomko <jtomko@redhat.com>

I will push it after the pipeline passes:
https://gitlab.com/janotomko/libvirt/-/pipelines/333213773

Jano

On 07-Jul-21 6:46 PM, Jano Tomko wrote:
>> Signed-off-by: Vinayak Kale <vkale@nvidia.com>
>> ---
>>   src/util/virresctrl.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
> Reviewed-by: Ján Tomko <jtomko@redhat.com>
>
> I will push it after the pipeline passes:
> https://gitlab.com/janotomko/libvirt/-/pipelines/333213773
>
> Jano

Thanks Jano and Daniel.