Re: [PATCH] xen-netback: fix memory leaks on XenBus disconnect

[Igor Druzhinin <igor.druzhinin@citrix.com>]

On 13/01/17 10:38, Wei Liu wrote:
> On Thu, Jan 12, 2017 at 05:51:56PM +0000, Igor Druzhinin wrote:
>> Eliminate memory leaks introduced several years ago by cleaning the queue
>> resources which are allocated on XenBus connection event. Namely, queue
>> structure array and pages used for IO rings.
>> vif->lock is used to protect statistics gathering agents from using the
>> queue structure during cleaning.
>>
> 
> There is code in netback_remove which eventually calls xenvif_free to
> free up the resources, maybe you should modify xenvif_free instead? That
> seems more symmetric to me. What do you think?
> 
>> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
>> ---
>>  drivers/net/xen-netback/interface.c |  6 ++++--
>>  drivers/net/xen-netback/xenbus.c    | 13 +++++++++++++
>>  2 files changed, 17 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
>> index e30ffd2..5795213 100644
>> --- a/drivers/net/xen-netback/interface.c
>> +++ b/drivers/net/xen-netback/interface.c
>> @@ -221,18 +221,18 @@ static struct net_device_stats *xenvif_get_stats(struct net_device *dev)
>>  {
>>  	struct xenvif *vif = netdev_priv(dev);
>>  	struct xenvif_queue *queue = NULL;
>> -	unsigned int num_queues = vif->num_queues;
>>  	unsigned long rx_bytes = 0;
>>  	unsigned long rx_packets = 0;
>>  	unsigned long tx_bytes = 0;
>>  	unsigned long tx_packets = 0;
>>  	unsigned int index;
>>  
>> +	spin_lock(&vif->lock);
>>  	if (vif->queues == NULL)
>>  		goto out;
>>  
>>  	/* Aggregate tx and rx stats from each queue */
>> -	for (index = 0; index < num_queues; ++index) {
>> +	for (index = 0; index < vif->num_queues; ++index) {
>>  		queue = &vif->queues[index];
>>  		rx_bytes += queue->stats.rx_bytes;
>>  		rx_packets += queue->stats.rx_packets;
>> @@ -241,6 +241,8 @@ static struct net_device_stats *xenvif_get_stats(struct net_device *dev)
>>  	}
>>  
>>  out:
>> +	spin_unlock(&vif->lock);
>> +
> 
> Good catch, this is definitely needed. And it would probably be in a
> separate patch.

I suggest we also need to have this spinlock acquired in another part
of cleaning code (xenvif_free). The reason to introduce it is a locking
practice in openvswitch when they don't grab any network subsystem locks
(rtnl_lock) in order to gather the statistics. I'm not sure that it's
correct behavior. Anyone can advise?

> Wei.
>