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