Keir, Jan (or anybody familiar with the new page_list_* functions) --
I''ve implemented page_list_splice as below but I don''t
think the second if clause should be necessary... I''m
using it with this clause because it works for this case
which is nearly always true (splicing into the scrub_list
which is almost always empty).
Without this clause, I get a null-pointer (actually null+4)
dereference... I think I must be missing some basic
difference between normal list code and page_list code.
Anyway, could you take a look at the routine to see if
I''ve missed something obvious in the case where neither
page_list parameter is empty? It''s tough to reproduce
this condition and you might be able to see a bug
on inspection. (Or maybe the differences between normal
list code and page_list code require the extra
page_list_empty check and this code will work properly
regardless of whether one or both page_list is empty.)
Thanks,
Dan
P.S. In case its not obvious, I started from list_splice
in list.h
=============
diff -r 4ac8bc60c000 xen/include/xen/mm.h
--- a/xen/include/xen/mm.h Tue Feb 10 05:51:00 2009 +0000
+++ b/xen/include/xen/mm.h Thu Mar 05 13:06:42 2009 -0700
@@ -219,6 +221,32 @@ page_list_remove_head(struct page_list_h
return page;
}
+static inline void
+page_list_splice(struct page_list_head *list, struct page_list_head *head)
+{
+ struct page_info *first, *last, *at;
+
+ if ( page_list_empty(list) )
+ return;
+
+ if ( page_list_empty(head) )
+ {
+ head->next = list->next;
+ head->tail = list->tail;
+ return;
+ }
+
+ first = list->next;
+ last = list->tail;
+ at = head->next;
+
+ first->list.prev = page_to_mfn(head->next);
+ head->next = first;
+
+ last->list.next = page_to_mfn(at);
+ at->list.prev = page_to_mfn(last);
+}
+
#define page_list_for_each(pos, head) \
for ( pos = (head)->next; pos; pos = page_list_next(pos, head) )
#define page_list_for_each_safe(pos, tmp, head) \
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
On 05/03/2009 20:23, "Dan Magenheimer" <dan.magenheimer@oracle.com> wrote:> Without this clause, I get a null-pointer (actually null+4) > dereference... I think I must be missing some basic > difference between normal list code and page_list code.An empty list has NULL pointers in its list_head. You therefore set at=NULL, and then dereference at->list.prev later. So I guess you do need that second if stmt. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
>>> Dan Magenheimer <dan.magenheimer@oracle.com> 05.03.09 21:23 >>> >Without this clause, I get a null-pointer (actually null+4) >dereference... I think I must be missing some basic >difference between normal list code and page_list code.The basic difference is that page_list_head has NULL pointers when empty, whereas ''normal'' lists heads have pointers to itself. So as Keir already stated, yes, the second conditional is needed.>Anyway, could you take a look at the routine to see if >I''ve missed something obvious in the case where neither >page_list parameter is empty? It''s tough to reproduce >this condition and you might be able to see a bug >on inspection. (Or maybe the differences between normal >list code and page_list code require the extra >page_list_empty check and this code will work properly >regardless of whether one or both page_list is empty.)The one question I would have is whether you indeed intend this to be an insertion at the head of the list - inserting at the tail would seem more natural. The ''normal'' list accessors allow doing this by simply passing &head->prev as the second argument, but since the page lists aren''t symmetric this cannot be done that way by the invoking code (and hence a suffix-less version of page_list_splice() would seem to more naturally splice to the end of the existing list, while if splicing to the beginning is indeed intended I''d favor calling it page_list_splice_head() or some such). Jan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
> >Without this clause, I get a null-pointer (actually null+4) > >dereference... I think I must be missing some basic > >difference between normal list code and page_list code. > > The basic difference is that page_list_head has NULL pointers > when empty, > whereas ''normal'' lists heads have pointers to itself. So as > Keir already > stated, yes, the second conditional is needed.Thanks (Keir too) for the confirmation.> >Anyway, could you take a look at the routine to see if > > The one question I would have is whether you indeed intend this to be > an insertion at the head of the list - inserting at the tail > would seem > more natural. The ''normal'' list accessors allow doing this by simply > passing &head->prev as the second argument, but since the page lists > aren''t symmetric this cannot be done that way by the invoking > code (and > hence a suffix-less version of page_list_splice() would seem to more > naturally splice to the end of the existing list, while if > splicing to the > beginning is indeed intended I''d favor calling it > page_list_splice_head() > or some such).Good feedback. But I was just adapting the "official" (in Xen and Linux) list.h list_splice() routine routine which apparently inserts at the head. For my usage, head vs tail doesn''t matter. Thanks, Dan _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel