[PATCH] python/xc: add missing Py_DECREF() to fix a memory leak

[PATCH] python/xc: add missing Py_DECREF() to fix a memory leak

[Zhigang Wang]

Python PyList_Append() will increase reference count of the item. We have to
decrease its reference count to let it garbage collected.

We missed the Py_DECREF() for 'cpuinfo_obj' here, thus we have a memory leak.

The memory leak could be easily confirmed by:

  # python
  >>> import xen.lowlevel.xc
  >>> xc = xen.lowlevel.xc.xc()
  >>> for i in range(1000): xc.getcpuinfo(1)

And check the python process memory usage before and after:

  # ps f -o vsize,rss,%mem,size,cmd -p <pid>

Signed-off-by: Zhigang Wang <zhigang.x.wang@oracle.com>
---
 tools/python/xen/lowlevel/xc/xc.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c
index 2c36eb2..9a161d3 100644
--- a/tools/python/xen/lowlevel/xc/xc.c
+++ b/tools/python/xen/lowlevel/xc/xc.c
@@ -1210,6 +1210,7 @@ static PyObject *pyxc_getcpuinfo(XcObject *self, PyObject *args, PyObject *kwds)
     for (i = 0; i < nr_cpus; i++) {
         cpuinfo_obj = Py_BuildValue("{s:k}", "idletime", cpuinfo_ptr->idletime);
         PyList_Append(cpuinfo_list_obj, cpuinfo_obj);
+        Py_DECREF(cpuinfo_obj);
         cpuinfo_ptr++;
     }
 
-- 
2.1.0

Re: [PATCH] python/xc: add missing Py_DECREF() to fix a memory leak

[Wei Liu]

On Fri, Aug 28, 2015 at 05:35:18PM -0400, Zhigang Wang wrote:
> Python PyList_Append() will increase reference count of the item. We have to
> decrease its reference count to let it garbage collected.
> 
> We missed the Py_DECREF() for 'cpuinfo_obj' here, thus we have a memory leak.
> 
> The memory leak could be easily confirmed by:
> 
>   # python
>   >>> import xen.lowlevel.xc
>   >>> xc = xen.lowlevel.xc.xc()
>   >>> for i in range(1000): xc.getcpuinfo(1)
> 
> And check the python process memory usage before and after:
> 
>   # ps f -o vsize,rss,%mem,size,cmd -p <pid>
> 
> Signed-off-by: Zhigang Wang <zhigang.x.wang@oracle.com>

Acked-by: Wei Liu <wei.liu2@citrix.com>

There is no in-tree user of python binding, I was planning to remove it
in 4.7 window.

But it seems like Oracle is still actively using it? Would you be happy to step
up to be the maintainer of this binding? I wouldn't expect too much traffic
regarding this piece of code (there were only 8 commits so far in this cycle
touching this binding, most of which cosmetic), so the workload wouldn't be a
big problem.

With a maintainer and active user in place we can probably avoid some of
the back and forth when touching this piece of code. For example:

commit ec55a3ae01a5be34d9a753781fba41d5930569f0
Author: Wei Liu <wei.liu2@citrix.com>
Date:   Mon Jul 27 18:45:08 2015 +0100

    python/xc: reinstate original implementation of next_bdf
    
    I missed the fact that next_bdf is used to parsed user supplied
    strings when reviewing. The user supplied string is a NULL-terminated
    string separated by comma. User can supply several PCI devices in that
    string. There is, however, no delimiter for different devices, hence
    we can't change the syntax of that string.
    
    This patch reinstate the original implementation of next_bdf to
    preserve the original syntax. The last argument for xc_assign_device
    is always 0.
    
    Signed-off-by: Wei Liu <wei.liu2@citrix.com>
    Acked-by: Ian Campbell <ian.campbell@citrix.com>

commit 9b34056cb4cab826771dba031735f77a02df015c
Author: Tiejun Chen <tiejun.chen@intel.com>
Date:   Wed Jul 22 01:40:08 2015 +0000

    tools: extend xc_assign_device() to support rdm reservation policy
    
    This patch passes rdm reservation policy to xc_assign_device() so the policy
    is checked when assigning devices to a VM.
    
    Note this also bring some fallout to python usage of xc_assign_device().
    
    CC: Ian Jackson <ian.jackson@eu.citrix.com>
    CC: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
    CC: Ian Campbell <ian.campbell@citrix.com>
    CC: Wei Liu <wei.liu2@citrix.com>
    CC: David Scott <dave.scott@eu.citrix.com>
    Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
    Acked-by: Wei Liu <wei.liu2@citrix.com>

You will also get least surprise when using the binding in new version of Xen.

Wei.

Re: [PATCH] python/xc: add missing Py_DECREF() to fix a memory leak

[Andrew Cooper]

On 31/08/2015 13:04, Wei Liu wrote:
> On Fri, Aug 28, 2015 at 05:35:18PM -0400, Zhigang Wang wrote:
>> Python PyList_Append() will increase reference count of the item. We have to
>> decrease its reference count to let it garbage collected.
>>
>> We missed the Py_DECREF() for 'cpuinfo_obj' here, thus we have a memory leak.
>>
>> The memory leak could be easily confirmed by:
>>
>>   # python
>>   >>> import xen.lowlevel.xc
>>   >>> xc = xen.lowlevel.xc.xc()
>>   >>> for i in range(1000): xc.getcpuinfo(1)
>>
>> And check the python process memory usage before and after:
>>
>>   # ps f -o vsize,rss,%mem,size,cmd -p <pid>
>>
>> Signed-off-by: Zhigang Wang <zhigang.x.wang@oracle.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>
>
> There is no in-tree user of python binding, I was planning to remove it
> in 4.7 window.

I am sure I have mentioned this in the past, but XenServer still uses
them in places, and they are very useful to use from interactive sessions.

~Andrew

(Oracle are still using them as they are still using xend)

Re: [PATCH] python/xc: add missing Py_DECREF() to fix a memory leak

[Wei Liu]

On Mon, Aug 31, 2015 at 01:27:35PM +0100, Andrew Cooper wrote:
> On 31/08/2015 13:04, Wei Liu wrote:
> > On Fri, Aug 28, 2015 at 05:35:18PM -0400, Zhigang Wang wrote:
> >> Python PyList_Append() will increase reference count of the item. We have to
> >> decrease its reference count to let it garbage collected.
> >>
> >> We missed the Py_DECREF() for 'cpuinfo_obj' here, thus we have a memory leak.
> >>
> >> The memory leak could be easily confirmed by:
> >>
> >>   # python
> >>   >>> import xen.lowlevel.xc
> >>   >>> xc = xen.lowlevel.xc.xc()
> >>   >>> for i in range(1000): xc.getcpuinfo(1)
> >>
> >> And check the python process memory usage before and after:
> >>
> >>   # ps f -o vsize,rss,%mem,size,cmd -p <pid>
> >>
> >> Signed-off-by: Zhigang Wang <zhigang.x.wang@oracle.com>
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> >
> > There is no in-tree user of python binding, I was planning to remove it
> > in 4.7 window.
> 
> I am sure I have mentioned this in the past, but XenServer still uses
> them in places, and they are very useful to use from interactive sessions.
> 

That's why I said "was". :-)

I don't mind leaving it in-tree given there is out-of-tree users.  The
problem is I don't think I can do a good job maintaining this piece of
code that I don't use and can't test, hence I encourage users to step
forward to maintain this piece of code.

Wei.

> ~Andrew
> 
> (Oracle are still using them as they are still using xend)

Re: [PATCH] python/xc: add missing Py_DECREF() to fix a memory leak

[Ian Campbell]

On Mon, 2015-08-31 at 13:32 +0100, Wei Liu wrote:
> On Mon, Aug 31, 2015 at 01:27:35PM +0100, Andrew Cooper wrote:
> > On 31/08/2015 13:04, Wei Liu wrote:
> > > On Fri, Aug 28, 2015 at 05:35:18PM -0400, Zhigang Wang wrote:
> > > > Python PyList_Append() will increase reference count of the item. 
> > > > We have to
> > > > decrease its reference count to let it garbage collected.
> > > > 
> > > > We missed the Py_DECREF() for 'cpuinfo_obj' here, thus we have a 
> > > > memory leak.
> > > > 
> > > > The memory leak could be easily confirmed by:
> > > > 
> > > >   # python
> > > >   >>> import xen.lowlevel.xc
> > > >   >>> xc = xen.lowlevel.xc.xc()
> > > >   >>> for i in range(1000): xc.getcpuinfo(1)
> > > > 
> > > > And check the python process memory usage before and after:
> > > > 
> > > >   # ps f -o vsize,rss,%mem,size,cmd -p <pid>
> > > > 
> > > > Signed-off-by: Zhigang Wang <zhigang.x.wang@oracle.com>
> > > Acked-by: Wei Liu <wei.liu2@citrix.com>
> > > 
> > > There is no in-tree user of python binding, I was planning to remove 
> > > it
> > > in 4.7 window.
> > 
> > I am sure I have mentioned this in the past, but XenServer still uses
> > them in places, and they are very useful to use from interactive 
> > sessions.
> > 
> 
> That's why I said "was". :-)
> 
> I don't mind leaving it in-tree given there is out-of-tree users.  The
> problem is I don't think I can do a good job maintaining this piece of
> code that I don't use and can't test, hence I encourage users to step
> forward to maintain this piece of code.

I too would be happy to see a patch to MAINTAINERS adding a section
covering at least tools/python/xen/lowlevel/xc (maybe xs too) and some
names of some actual users/maintainers of that code.

I think the xl bindings are properly dead and should be removed.

Ian.

Re: [PATCH] python/xc: add missing Py_DECREF() to fix a memory leak

[Ian Campbell]

On Mon, 2015-08-31 at 13:04 +0100, Wei Liu wrote:
> On Fri, Aug 28, 2015 at 05:35:18PM -0400, Zhigang Wang wrote:
> > Python PyList_Append() will increase reference count of the item. We 
> > have to
> > decrease its reference count to let it garbage collected.
> > 
> > We missed the Py_DECREF() for 'cpuinfo_obj' here, thus we have a memory 
> > leak.
> > 
> > The memory leak could be easily confirmed by:
> > 
> >   # python
> >   >>> import xen.lowlevel.xc
> >   >>> xc = xen.lowlevel.xc.xc()
> >   >>> for i in range(1000): xc.getcpuinfo(1)
> > 
> > And check the python process memory usage before and after:
> > 
> >   # ps f -o vsize,rss,%mem,size,cmd -p <pid>
> > 
> > Signed-off-by: Zhigang Wang <zhigang.x.wang@oracle.com>
> 
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Applied.