Tim Deegan
2011-Jun-24 12:16 UTC
[Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
tools: fix build after recent xenpaging changes xenpaging now uses pthreads, so must link appropriately. Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com> diff -r 2633588c2427 tools/xenpaging/Makefile --- a/tools/xenpaging/Makefile Fri Jun 24 13:03:38 2011 +0100 +++ b/tools/xenpaging/Makefile Fri Jun 24 13:10:34 2011 +0100 @@ -2,7 +2,7 @@ XEN_ROOT=$(CURDIR)/../.. include $(XEN_ROOT)/tools/Rules.mk CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenstore) -LDLIBS += $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore) +LDLIBS += $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore) -pthread POLICY = default _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Olaf Hering
2011-Jun-24 12:24 UTC
Re: [Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
On Fri, Jun 24, Tim Deegan wrote:> tools: fix build after recent xenpaging changes > xenpaging now uses pthreads, so must link appropriately. > > Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com>Acked-by: Olaf Hering <olaf@aepfle.de> _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-24 12:33 UTC
Re: [Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
On Fri, 2011-06-24 at 13:16 +0100, Tim Deegan wrote:> tools: fix build after recent xenpaging changes > xenpaging now uses pthreads, so must link appropriately.Why does 23625:c49e22648d0e need a new thread to do the page in on exit? Can''t it just signal the main loop to do it? Also page_in_trigger doesn''t seem safe to me: +void page_in_trigger(unsigned long gfn) +{ + if (!page_in_possible) + return; + + pthread_mutex_lock(&page_in_mutex); + page_in_gfn = gfn; + pthread_mutex_unlock(&page_in_mutex); + pthread_cond_signal(&page_in_cond); +} Two back to back calls to this function (which is what the caller will do) will both update page_in_gfn without the page in thread necessarily running in the interim. i.e. the first gfn may be missed. I don''t think pthread_cond_signal makes any guarantees about whether this thread or the signalled thread will run afterwards. For this approach to woek page_in_gfn really needs to remain locked until the page in thread has finished with that particular entry, or you need s return signal, or a queue, or whatever. I suppose you could also push the "/* Write all pages back into the guest */" loop down into the thread rather than feeding the thread mfns one-by-one. Ian.> > Signed-off-by: Tim Deegan <Tim.Deegan@citrix.com> > > diff -r 2633588c2427 tools/xenpaging/Makefile > --- a/tools/xenpaging/Makefile Fri Jun 24 13:03:38 2011 +0100 > +++ b/tools/xenpaging/Makefile Fri Jun 24 13:10:34 2011 +0100 > @@ -2,7 +2,7 @@ XEN_ROOT=$(CURDIR)/../.. > include $(XEN_ROOT)/tools/Rules.mk > > CFLAGS += $(CFLAGS_libxenctrl) $(CFLAGS_libxenstore) > -LDLIBS += $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore) > +LDLIBS += $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore) -pthread > > POLICY = default > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Olaf Hering
2011-Jun-24 13:32 UTC
Re: [Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
On Fri, Jun 24, Ian Campbell wrote:> On Fri, 2011-06-24 at 13:16 +0100, Tim Deegan wrote: > > tools: fix build after recent xenpaging changes > > xenpaging now uses pthreads, so must link appropriately. > > Why does 23625:c49e22648d0e need a new thread to do the page in on exit? > Can''t it just signal the main loop to do it?If the page is mappend and the gfn is not there, the attempt to map it may block. I havent tried it, and I think the current code will not block (linux_privcmd_map_foreign_bulk will just loop). If it does block, the mainloop can not proceed and process the page-in request.> Also page_in_trigger doesn''t seem safe to me: > +void page_in_trigger(unsigned long gfn) > +{ > + if (!page_in_possible) > + return; > + > + pthread_mutex_lock(&page_in_mutex); > + page_in_gfn = gfn; > + pthread_mutex_unlock(&page_in_mutex); > + pthread_cond_signal(&page_in_cond); > +} > > Two back to back calls to this function (which is what the caller will > do) will both update page_in_gfn without the page in thread necessarily > running in the interim. i.e. the first gfn may be missed. I don''t think > pthread_cond_signal makes any guarantees about whether this thread or > the signalled thread will run afterwards. For this approach to woek > page_in_gfn really needs to remain locked until the page in thread has > finished with that particular entry, or you need s return signal, or a > queue, or whatever.Its coded after an example in the APUE book. The page-in thread grabs a copy of page_in_gfn. Are you saying page_in_trigger() can be called more than once while xc_map_foreign_pages()/munmap() is being called? If the caller of page_in_trigger will find the gfn is still in paging state, it will just try again. Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-24 13:48 UTC
Re: [Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
On Fri, 2011-06-24 at 14:32 +0100, Olaf Hering wrote:> On Fri, Jun 24, Ian Campbell wrote: > > > On Fri, 2011-06-24 at 13:16 +0100, Tim Deegan wrote: > > > tools: fix build after recent xenpaging changes > > > xenpaging now uses pthreads, so must link appropriately. > > > > Why does 23625:c49e22648d0e need a new thread to do the page in on exit? > > Can''t it just signal the main loop to do it? > > If the page is mappend and the gfn is not there, the attempt to map it > may block. I havent tried it, and I think the current code will not > block (linux_privcmd_map_foreign_bulk will just loop). > If it does block, the mainloop can not proceed and process the page-in > request.It doesn''t return EINTR due to the signal? I wonder if it would be worth investigating setjmp here?> > > Also page_in_trigger doesn''t seem safe to me: > > +void page_in_trigger(unsigned long gfn) > > +{ > > + if (!page_in_possible) > > + return; > > + > > + pthread_mutex_lock(&page_in_mutex); > > + page_in_gfn = gfn; > > + pthread_mutex_unlock(&page_in_mutex); > > + pthread_cond_signal(&page_in_cond); > > +} > > > > Two back to back calls to this function (which is what the caller will > > do) will both update page_in_gfn without the page in thread necessarily > > running in the interim. i.e. the first gfn may be missed. I don''t think > > pthread_cond_signal makes any guarantees about whether this thread or > > the signalled thread will run afterwards. For this approach to woek > > page_in_gfn really needs to remain locked until the page in thread has > > finished with that particular entry, or you need s return signal, or a > > queue, or whatever. > > Its coded after an example in the APUE book. The page-in thread grabs a > copy of page_in_gfn.But there is no interlock between the page-in thread and page-in trigger. IOW it is possible to do>loop over pagespage_in_trigger(1) lock page_in_gfn = 1 unlock signal page-in thread (but it doesn''t get scheduled yet, for whatever reason)>next iteration of looppage_in_trigger(2) lock page_in_gfn = 2 unlock>>> page-in thread (signalled above) finally gets to run and preempts uspage_in_thread lock read page_in_gfn ==> 2 *** we''ve missed page 1 *** unlock> Are you saying page_in_trigger() can be called > more than once while xc_map_foreign_pages()/munmap() is being called?The loop is: for ( i = 0; i < paging->domain_info->max_pages; i++ ) { if ( test_bit(i, paging->bitmap) ) { page_in_trigger(i); break; } } There is nothing to stop it going round again as fast as it wants. The xc_map_foreign_pages()/munmap() are in another thread and so can be running in parallel and/or you don''t control the preemption between the two threads. In fact since the page-in thread is doing relatively expensive work I''d expect that the trigger loop would get to run several iterations for each time the page-in loop ran..> > If the caller of page_in_trigger will find the gfn is still in paging > state, it will just try again.I don''t see where it would go back and try page 1 again if it gets missed (as in the above example) Ian.> > Olaf_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Olaf Hering
2011-Jun-24 13:57 UTC
Re: [Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
On Fri, Jun 24, Ian Campbell wrote:> In fact since the page-in thread is doing relatively expensive work I''d > expect that the trigger loop would get to run several iterations for > each time the page-in loop ran..That did not happen for me, I will think about it.> > If the caller of page_in_trigger will find the gfn is still in paging > > state, it will just try again. > > I don''t see where it would go back and try page 1 again if it gets > missed (as in the above example)The break exits the for() loop, not the while(1). In the next iteration page 1 may still be in paging->bitmap and tried again. Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-24 14:30 UTC
Re: [Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
On Fri, 2011-06-24 at 14:57 +0100, Olaf Hering wrote:> On Fri, Jun 24, Ian Campbell wrote: > > > In fact since the page-in thread is doing relatively expensive work I''d > > expect that the trigger loop would get to run several iterations for > > each time the page-in loop ran.. > > That did not happen for me, I will think about it.It possibly doesn''t matter, based on hat you said below, since you will come back round and try again. It makes the existing locking a bit pointless though I think, since you are in "fast-and-lose-mode" already.> > > If the caller of page_in_trigger will find the gfn is still in paging > > > state, it will just try again. > > > > I don''t see where it would go back and try page 1 again if it gets > > missed (as in the above example) > > The break exits the for() loop, not the while(1). In the next iteration > page 1 may still be in paging->bitmap and tried again.I''d missed the interrupted -> 1 in the while loop. I presume there is some other exit condition which triggers once everything has been paged back in and actually causes the daemon to exit? Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Olaf Hering
2011-Jun-24 14:35 UTC
Re: [Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
On Fri, Jun 24, Ian Campbell wrote:> > The break exits the for() loop, not the while(1). In the next iteration > > page 1 may still be in paging->bitmap and tried again. > > I''d missed the interrupted -> 1 in the while loop. I presume there is > some other exit condition which triggers once everything has been paged > back in and actually causes the daemon to exit?If the for() loop found nothing, then this triggers: 775 /* If no more pages to process, exit loop */ 776 if ( i == paging->domain_info->max_pages ) 777 break; Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Campbell
2011-Jun-24 14:41 UTC
Re: [Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
On Fri, 2011-06-24 at 15:35 +0100, Olaf Hering wrote:> On Fri, Jun 24, Ian Campbell wrote: > > > > The break exits the for() loop, not the while(1). In the next iteration > > > page 1 may still be in paging->bitmap and tried again. > > > > I''d missed the interrupted -> 1 in the while loop. I presume there is > > some other exit condition which triggers once everything has been paged > > back in and actually causes the daemon to exit? > > If the for() loop found nothing, then this triggers: > > 775 /* If no more pages to process, exit loop */ > 776 if ( i == paging->domain_info->max_pages ) > 777 break;Oh yes. It''s a bit counter intuitive to have a for loop which only processes the first thing it finds and then relying on going round another outer loop to pickup the second etc. It''s at least O(N^2), isn''t it? Why not count = 0; for ( i = 0; i < paging->domain_info->max_pages; i++ ) { if ( test_bit(i, paging->bitmap) ) { page_in_trigger(i); count++ } } /* If no more pages to process, exit loop */ if ( !count ) break; That will at least process as many pages as it can on each iteration through the outer loop. Although it will most likely exacerbate the locking issue I pointed to earlier. Ian. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Olaf Hering
2011-Jun-24 15:24 UTC
Re: [Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
On Fri, Jun 24, Ian Campbell wrote:> On Fri, 2011-06-24 at 15:35 +0100, Olaf Hering wrote: > > On Fri, Jun 24, Ian Campbell wrote: > > > > > > The break exits the for() loop, not the while(1). In the next iteration > > > > page 1 may still be in paging->bitmap and tried again. > > > > > > I''d missed the interrupted -> 1 in the while loop. I presume there is > > > some other exit condition which triggers once everything has been paged > > > back in and actually causes the daemon to exit? > > > > If the for() loop found nothing, then this triggers: > > > > 775 /* If no more pages to process, exit loop */ > > 776 if ( i == paging->domain_info->max_pages ) > > 777 break; > > Oh yes. > > It''s a bit counter intuitive to have a for loop which only processes the > first thing it finds and then relying on going round another outer loop > to pickup the second etc. It''s at least O(N^2), isn''t it? > > Why not > count = 0; > for ( i = 0; i < paging->domain_info->max_pages; i++ ) > { > if ( test_bit(i, paging->bitmap) ) > { > page_in_trigger(i); > count++ > } > } > /* If no more pages to process, exit loop */ > if ( !count ) > break; > > That will at least process as many pages as it can on each iteration > through the outer loop. Although it will most likely exacerbate the > locking issue I pointed to earlier.I think an early version of my change had something like that, but it did not fillup the ringbuffer for some reason. I will look at this change again and see what can be improved. Olaf _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Ian Jackson
2011-Jun-27 13:50 UTC
Re: [Xen-devel] [PATCH] tools: fix build after recent xenpaging changes
Tim Deegan writes ("[Xen-devel] [PATCH] tools: fix build after recent
xenpaging changes"):> tools: fix build after recent xenpaging changes
> xenpaging now uses pthreads, so must link appropriately.
Thanks, I''ve applied this. Sorry, I got distracted by the huge
thread. We can fix the thread-safety problems in xenpaging at our
leisure.
> -LDLIBS += $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore)
> +LDLIBS += $(LDLIBS_libxenctrl) $(LDLIBS_libxenstore) -pthread
Arguably config/StdGNU.mk should say
PTHREAD_LIBS = -pthread
and we should use that here.
Ian.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel