* [PATCH] xen-netback: fix memory leaks on XenBus disconnect
@ 2017-01-12 17:51 Igor Druzhinin
2017-01-12 18:05 ` Igor Druzhinin
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Igor Druzhinin @ 2017-01-12 17:51 UTC (permalink / raw)
To: wei.liu2, xen-devel, paul.durrant; +Cc: netdev, linux-kernel, igor.druzhinin
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.
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);
+
vif->dev->stats.rx_bytes = rx_bytes;
vif->dev->stats.rx_packets = rx_packets;
vif->dev->stats.tx_bytes = tx_bytes;
diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
index 3124eae..85b742e 100644
--- a/drivers/net/xen-netback/xenbus.c
+++ b/drivers/net/xen-netback/xenbus.c
@@ -493,11 +493,22 @@ static int backend_create_xenvif(struct backend_info *be)
static void backend_disconnect(struct backend_info *be)
{
if (be->vif) {
+ unsigned int queue_index;
+
xen_unregister_watchers(be->vif);
#ifdef CONFIG_DEBUG_FS
xenvif_debugfs_delif(be->vif);
#endif /* CONFIG_DEBUG_FS */
xenvif_disconnect_data(be->vif);
+ for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index)
+ xenvif_deinit_queue(&be->vif->queues[queue_index]);
+
+ spin_lock(&be->vif->lock);
+ vfree(be->vif->queues);
+ be->vif->num_queues = 0;
+ be->vif->queues = NULL;
+ spin_unlock(&be->vif->lock);
+
xenvif_disconnect_ctrl(be->vif);
}
}
@@ -1034,6 +1045,8 @@ static void connect(struct backend_info *be)
err:
if (be->vif->num_queues > 0)
xenvif_disconnect_data(be->vif); /* Clean up existing queues */
+ for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index)
+ xenvif_deinit_queue(&be->vif->queues[queue_index]);
vfree(be->vif->queues);
be->vif->queues = NULL;
be->vif->num_queues = 0;
--
1.8.3.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen-netback: fix memory leaks on XenBus disconnect
2017-01-12 17:51 [PATCH] xen-netback: fix memory leaks on XenBus disconnect Igor Druzhinin
@ 2017-01-12 18:05 ` Igor Druzhinin
2017-01-13 9:01 ` Paul Durrant
2017-01-13 10:38 ` Wei Liu
2 siblings, 0 replies; 6+ messages in thread
From: Igor Druzhinin @ 2017-01-12 18:05 UTC (permalink / raw)
To: wei.liu2, xen-devel, paul.durrant; +Cc: netdev, linux-kernel, jgross
On 12/01/17 17:51, 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.
>
> 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);
> +
> vif->dev->stats.rx_bytes = rx_bytes;
> vif->dev->stats.rx_packets = rx_packets;
> vif->dev->stats.tx_bytes = tx_bytes;
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-netback/xenbus.c
> index 3124eae..85b742e 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -493,11 +493,22 @@ static int backend_create_xenvif(struct backend_info *be)
> static void backend_disconnect(struct backend_info *be)
> {
> if (be->vif) {
> + unsigned int queue_index;
> +
> xen_unregister_watchers(be->vif);
> #ifdef CONFIG_DEBUG_FS
> xenvif_debugfs_delif(be->vif);
> #endif /* CONFIG_DEBUG_FS */
> xenvif_disconnect_data(be->vif);
> + for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index)
> + xenvif_deinit_queue(&be->vif->queues[queue_index]);
> +
> + spin_lock(&be->vif->lock);
> + vfree(be->vif->queues);
> + be->vif->num_queues = 0;
> + be->vif->queues = NULL;
> + spin_unlock(&be->vif->lock);
> +
> xenvif_disconnect_ctrl(be->vif);
> }
> }
> @@ -1034,6 +1045,8 @@ static void connect(struct backend_info *be)
> err:
> if (be->vif->num_queues > 0)
> xenvif_disconnect_data(be->vif); /* Clean up existing queues */
> + for (queue_index = 0; queue_index < be->vif->num_queues; ++queue_index)
> + xenvif_deinit_queue(&be->vif->queues[queue_index]);
> vfree(be->vif->queues);
> be->vif->queues = NULL;
> be->vif->num_queues = 0;
>
Add Juergen Gross to CC.
Igor
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] xen-netback: fix memory leaks on XenBus disconnect
2017-01-12 17:51 [PATCH] xen-netback: fix memory leaks on XenBus disconnect Igor Druzhinin
2017-01-12 18:05 ` Igor Druzhinin
@ 2017-01-13 9:01 ` Paul Durrant
2017-01-13 10:38 ` Wei Liu
2 siblings, 0 replies; 6+ messages in thread
From: Paul Durrant @ 2017-01-13 9:01 UTC (permalink / raw)
To: Igor Druzhinin, Wei Liu, xen-devel; +Cc: netdev, linux-kernel, Igor Druzhinin
> -----Original Message-----
> From: Igor Druzhinin [mailto:igor.druzhinin@citrix.com]
> Sent: 12 January 2017 17:52
> To: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org; Paul
> Durrant <Paul.Durrant@citrix.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Igor Druzhinin
> <igor.druzhinin@citrix.com>
> Subject: [PATCH] xen-netback: fix memory leaks on XenBus disconnect
>
> 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.
>
> Signed-off-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
...although I was involved with discussions concerning this, so Wei should probably look it over too.
> ---
> 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);
> +
> vif->dev->stats.rx_bytes = rx_bytes;
> vif->dev->stats.rx_packets = rx_packets;
> vif->dev->stats.tx_bytes = tx_bytes;
> diff --git a/drivers/net/xen-netback/xenbus.c b/drivers/net/xen-
> netback/xenbus.c
> index 3124eae..85b742e 100644
> --- a/drivers/net/xen-netback/xenbus.c
> +++ b/drivers/net/xen-netback/xenbus.c
> @@ -493,11 +493,22 @@ static int backend_create_xenvif(struct
> backend_info *be)
> static void backend_disconnect(struct backend_info *be)
> {
> if (be->vif) {
> + unsigned int queue_index;
> +
> xen_unregister_watchers(be->vif);
> #ifdef CONFIG_DEBUG_FS
> xenvif_debugfs_delif(be->vif);
> #endif /* CONFIG_DEBUG_FS */
> xenvif_disconnect_data(be->vif);
> + for (queue_index = 0; queue_index < be->vif-
> >num_queues; ++queue_index)
> + xenvif_deinit_queue(&be->vif-
> >queues[queue_index]);
> +
> + spin_lock(&be->vif->lock);
> + vfree(be->vif->queues);
> + be->vif->num_queues = 0;
> + be->vif->queues = NULL;
> + spin_unlock(&be->vif->lock);
> +
> xenvif_disconnect_ctrl(be->vif);
> }
> }
> @@ -1034,6 +1045,8 @@ static void connect(struct backend_info *be)
> err:
> if (be->vif->num_queues > 0)
> xenvif_disconnect_data(be->vif); /* Clean up existing
> queues */
> + for (queue_index = 0; queue_index < be->vif->num_queues;
> ++queue_index)
> + xenvif_deinit_queue(&be->vif->queues[queue_index]);
> vfree(be->vif->queues);
> be->vif->queues = NULL;
> be->vif->num_queues = 0;
> --
> 1.8.3.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen-netback: fix memory leaks on XenBus disconnect
2017-01-12 17:51 [PATCH] xen-netback: fix memory leaks on XenBus disconnect Igor Druzhinin
2017-01-12 18:05 ` Igor Druzhinin
2017-01-13 9:01 ` Paul Durrant
@ 2017-01-13 10:38 ` Wei Liu
2017-01-13 10:49 ` Paul Durrant
2017-01-13 11:06 ` Igor Druzhinin
2 siblings, 2 replies; 6+ messages in thread
From: Wei Liu @ 2017-01-13 10:38 UTC (permalink / raw)
To: Igor Druzhinin; +Cc: wei.liu2, xen-devel, paul.durrant, netdev, linux-kernel
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.
Wei.
^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH] xen-netback: fix memory leaks on XenBus disconnect
2017-01-13 10:38 ` Wei Liu
@ 2017-01-13 10:49 ` Paul Durrant
2017-01-13 11:06 ` Igor Druzhinin
1 sibling, 0 replies; 6+ messages in thread
From: Paul Durrant @ 2017-01-13 10:49 UTC (permalink / raw)
To: Wei Liu, Igor Druzhinin; +Cc: Wei Liu, xen-devel, netdev, linux-kernel
> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 13 January 2017 10:38
> To: Igor Druzhinin <igor.druzhinin@citrix.com>
> Cc: Wei Liu <wei.liu2@citrix.com>; xen-devel@lists.xenproject.org; Paul
> Durrant <Paul.Durrant@citrix.com>; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH] xen-netback: fix memory leaks on XenBus disconnect
>
> 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?
The connect code vallocs the queue array because the size is not known until then so it makes sense that disconnect vfrees it.
Paul
>
> > 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.
>
> Wei.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen-netback: fix memory leaks on XenBus disconnect
2017-01-13 10:38 ` Wei Liu
2017-01-13 10:49 ` Paul Durrant
@ 2017-01-13 11:06 ` Igor Druzhinin
1 sibling, 0 replies; 6+ messages in thread
From: Igor Druzhinin @ 2017-01-13 11:06 UTC (permalink / raw)
To: Wei Liu; +Cc: xen-devel, paul.durrant, netdev, linux-kernel
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.
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-01-13 11:07 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-12 17:51 [PATCH] xen-netback: fix memory leaks on XenBus disconnect Igor Druzhinin
2017-01-12 18:05 ` Igor Druzhinin
2017-01-13 9:01 ` Paul Durrant
2017-01-13 10:38 ` Wei Liu
2017-01-13 10:49 ` Paul Durrant
2017-01-13 11:06 ` Igor Druzhinin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).