Xen devel - Nov 2012 - [PATCH] x86/hap: Fix memory leak of domain->arch.hvm_domain.dirty_vram

This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.

Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

>>> On 28.11.12 at 07:51, Kouya Shimura <kouya@jp.fujitsu.com>
wrote:
> This patch fixes the memory leak of domain->arch.hvm_domain.dirty_vram.
> 
> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
Wouldn''t it be more consistent (and less redundant) to do this
through calling hap_track_dirty_vram(d, 0, 0, ...)? And even if
not, the conditional around the freeing/clearing is pointless.

Jan

On 11/28/2012 05:39 PM, Jan Beulich wrote:
>>>> On 28.11.12 at 07:51, Kouya Shimura
<kouya@jp.fujitsu.com> wrote:
>> This patch fixes the memory leak of
domain->arch.hvm_domain.dirty_vram.
>>
>> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
>
> Wouldn''t it be more consistent (and less redundant) to do this
> through calling hap_track_dirty_vram(d, 0, 0, ...)? And even if
> not, the conditional around the freeing/clearing is pointless.
>
> Jan
>
 From another point of view, it''s consistent since it almost
copied from shadow_teardown()@xen/arch/x86/mm/shadow/common.c.
That is a sibling function of hap_teardown().
If it''s not preferable, another cleanup patch should be made.

IMHO, I feel no good to remain a pointer to the freed memory
even though it never be dereferenced any more.
So it makes sense to check the pointer and set it NULL.

Thanks,
Kouya

>>> On 29.11.12 at 02:00, Kouya Shimura <kouya@jp.fujitsu.com>
wrote:
> On 11/28/2012 05:39 PM, Jan Beulich wrote:
>>>>> On 28.11.12 at 07:51, Kouya Shimura
<kouya@jp.fujitsu.com> wrote:
>>> This patch fixes the memory leak of
domain->arch.hvm_domain.dirty_vram.
>>>
>>> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
>>
>> Wouldn''t it be more consistent (and less redundant) to do this
>> through calling hap_track_dirty_vram(d, 0, 0, ...)? And even if
>> not, the conditional around the freeing/clearing is pointless.
> 
>  From another point of view, it''s consistent since it almost
> copied from shadow_teardown()@xen/arch/x86/mm/shadow/common.c.
> That is a sibling function of hap_teardown().
> If it''s not preferable, another cleanup patch should be made.
Tim will have the final say here anyway. But my general opinion
is that copying existing code is not an excuse to also copy its
eventual deficiencies.
> IMHO, I feel no good to remain a pointer to the freed memory
> even though it never be dereferenced any more.
> So it makes sense to check the pointer and set it NULL.
I didn''t say you shouldn''t clear the pointer field. All I said
is
that the conditional around the freeing and clearing is pointless.

Jan

At 07:26 +0000 on 29 Nov (1354174019), Jan Beulich
wrote:
> >>> On 29.11.12 at 02:00, Kouya Shimura
<kouya@jp.fujitsu.com> wrote:
> > On 11/28/2012 05:39 PM, Jan Beulich wrote:
> >>>>> On 28.11.12 at 07:51, Kouya Shimura
<kouya@jp.fujitsu.com> wrote:
> >>> This patch fixes the memory leak of
domain->arch.hvm_domain.dirty_vram.
> >>>
> >>> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
> >>
> >> Wouldn''t it be more consistent (and less redundant) to do
this
> >> through calling hap_track_dirty_vram(d, 0, 0, ...)? And even if
> >> not, the conditional around the freeing/clearing is pointless.
> > 
> >  From another point of view, it''s consistent since it almost
> > copied from shadow_teardown()@xen/arch/x86/mm/shadow/common.c.
> > That is a sibling function of hap_teardown().
> > If it''s not preferable, another cleanup patch should be made.
> 
> Tim will have the final say here anyway.
I have taken the conditional out but left it as an explicit free(). 
It might not always be safe to call hap_track_dirty_vram(), even for
disabling, this late in the teardown.

Thanks for the fix!

Tim.

>>> On 29.11.12 at 12:05, Tim Deegan <tim@xen.org> wrote:
> At 07:26 +0000 on 29 Nov (1354174019), Jan Beulich wrote:
>> >>> On 29.11.12 at 02:00, Kouya Shimura
<kouya@jp.fujitsu.com> wrote:
>> > On 11/28/2012 05:39 PM, Jan Beulich wrote:
>> >>>>> On 28.11.12 at 07:51, Kouya Shimura
<kouya@jp.fujitsu.com> wrote:
>> >>> This patch fixes the memory leak of
domain->arch.hvm_domain.dirty_vram.
>> >>>
>> >>> Signed-off-by: Kouya Shimura <kouya@jp.fujitsu.com>
>> >>
>> >> Wouldn''t it be more consistent (and less redundant)
to do this
>> >> through calling hap_track_dirty_vram(d, 0, 0, ...)? And even
if
>> >> not, the conditional around the freeing/clearing is pointless.
>> > 
>> >  From another point of view, it''s consistent since it
almost
>> > copied from shadow_teardown()@xen/arch/x86/mm/shadow/common.c.
>> > That is a sibling function of hap_teardown().
>> > If it''s not preferable, another cleanup patch should be
made.
>> 
>> Tim will have the final say here anyway.
> 
> I have taken the conditional out but left it as an explicit free(). 
> It might not always be safe to call hap_track_dirty_vram(), even for
> disabling, this late in the teardown.
So is this something we also want to have on 4.2 and 4.1?

Jan

At 13:16 +0000 on 29 Nov (1354194991), Jan Beulich
wrote:
> >>> On 29.11.12 at 12:05, Tim Deegan <tim@xen.org> wrote:
> > I have taken the conditional out but left it as an explicit free(). 
> > It might not always be safe to call hap_track_dirty_vram(), even for
> > disabling, this late in the teardown.
> 
> So is this something we also want to have on 4.2 and 4.1?
Yes, I think so (after the customary delay).  It applies OK to 4.2; 
a backport to 4.1 is attached.

Tim.


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel