[PATCH] tools: fix build after recent xenpaging changes

[PATCH] tools: fix build after recent xenpaging changes

[Tim Deegan]

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

Re: [PATCH] tools: fix build after recent xenpaging changes

[Olaf Hering]

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>

Re: [PATCH] tools: fix build after recent xenpaging changes

[Ian Campbell]

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

Re: [PATCH] tools: fix build after recent xenpaging changes

[Olaf Hering]

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

Re: [PATCH] tools: fix build after recent xenpaging changes

[Ian Campbell]

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 pages
	page_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 loop
	page_in_trigger(2)
		lock
		page_in_gfn = 2
		unlock
>>> page-in thread (signalled above) finally gets to run and preempts us
page_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

Re: [PATCH] tools: fix build after recent xenpaging changes

[Olaf Hering]

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

Re: [PATCH] tools: fix build after recent xenpaging changes

[Ian Campbell]

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.

Re: [PATCH] tools: fix build after recent xenpaging changes

[Olaf Hering]

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

Re: [PATCH] tools: fix build after recent xenpaging changes

[Ian Campbell]

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.

Re: [PATCH] tools: fix build after recent xenpaging changes

[Olaf Hering]

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

Re: [PATCH] tools: fix build after recent xenpaging changes

[Ian Jackson]

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.