Hi all,
It appears netfront is leaking 2 pages (info->tx.sring and rx.sring) on
netfront unload, because netback is not unmapping them when entering the
Closed state.
I came across this while working on the Windows Xennet driver unload
routines. I am trying to end access to those two pages after both
netfront and netback are in the "Closed" state (having passed through
"Closing") but they are still in use.
See below from netback/xenbus.c. Backend is online:
case XenbusStateClosed:
xenbus_switch_state(dev, XenbusStateClosed);
if (xenbus_dev_is_online(dev))
break;
/* fall through if not online */
Should it instead be:
case XenbusStateClosed:
if (xenbus_dev_is_online(dev)) {
struct backend_info *be = dev->dev.driver_data;
netif_disconnect(be->netif);
break;
}
xenbus_switch_state(dev, XenbusStateClosed);
/* fall through if not online */
?
Otherwise the pages cannot be freed properly in the guest, right?
Thanks -- Regards -- Andy
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On 16/1/08 21:16, "Andy Grover" <andy.grover@oracle.com> wrote:> Should it instead be: > > case XenbusStateClosed: > if (xenbus_dev_is_online(dev)) { > struct backend_info *be = dev->dev.driver_data; > netif_disconnect(be->netif); > break; > } > xenbus_switch_state(dev, XenbusStateClosed); > /* fall through if not online */ > > ? > > Otherwise the pages cannot be freed properly in the guest, right?Well, I think you''ve chosen slightly the wrong place, so I''ve just committed a change that makes the close-down logic more like in blkback. Possibly the reason we didn''t follow that same logic before is that netif_disconnect() also makes the network device in dom0 also go away, which might cause unwanted chnages to routing and firewall rules, execution of hotplug scripts, etc. But we can separate the concepts of relinquishing frontend resources and destroying the backend netdev later if it turns out to be necessary. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On Thu, 2008-01-17 at 15:48 +0000, Keir Fraser wrote:> On 16/1/08 21:16, "Andy Grover" <andy.grover@oracle.com> wrote:> > Otherwise the pages cannot be freed properly in the guest, right? > > Well, I think you''ve chosen slightly the wrong place, so I''ve just committed > a change that makes the close-down logic more like in blkback. Possibly the > reason we didn''t follow that same logic before is that netif_disconnect() > also makes the network device in dom0 also go away, which might cause > unwanted chnages to routing and firewall rules, execution of hotplug > scripts, etc. But we can separate the concepts of relinquishing frontend > resources and destroying the backend netdev later if it turns out to be > necessary.Thanks for taking a look. I was going to put together a patch to properly free the 2 pages in netfront, but it looks like this is already handled inside gnttab_end_foreign_access(). It seems a little non-intuitive that gnttab_end_foreign_access doesn''t just end foreign access, it frees the page too. This leads to what confused me -- a driver allocating a page but (seemingly) never freeing it. I think it might be clearer to make callers explicitly free the page after checking gnttab_end_foreign_access''s return val for success. Would you accept a patch doing this, or are there other reasons for the current code? Thanks -- Regards -- Andy _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 18/1/08 20:35, "Andy Grover" <andy.grover@oracle.com> wrote:> Thanks for taking a look. > > I was going to put together a patch to properly free the 2 pages in > netfront, but it looks like this is already handled inside > gnttab_end_foreign_access(). > > It seems a little non-intuitive that gnttab_end_foreign_access doesn''t > just end foreign access, it frees the page too. This leads to what > confused me -- a driver allocating a page but (seemingly) never freeing > it. > > I think it might be clearer to make callers explicitly free the page > after checking gnttab_end_foreign_access''s return val for success. Would > you accept a patch doing this, or are there other reasons for the > current code?If the grant is not yet unmapped by the backend then the gnttab subsystem in the frontend OS should take responsibility for freeing the page at the appropriate time in the future (i.e., when the grant does become unmapped). We don''t want to leave that to the frontend driver itself because it would result in code duplication in every driver, and also the driver may be unloaded before the page can be freed. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel