[PATCH] fix page_list_splice()

[PATCH] fix page_list_splice()

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 770 bytes --]

Other than in __list_splice(), the first element's prev pointer doesn't
need adjustment here - it already is PAGE_LIST_NULL. Rather than fixing
the assignment (to formally match __list_splice()), simply assert that
this assignment is really unnecessary.

Reported-by: Jisoo Yang <jisooy@gmail.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -270,7 +270,8 @@ page_list_splice(struct page_list_head *
     last = list->tail;
     at = head->next;
 
-    first->list.prev = page_to_pdx(head->next);
+    /* Both first->list.prev and at->list.prev are PAGE_LIST_NULL. */
+    ASSERT(first->list.prev == at->list.prev);
     head->next = first;
 
     last->list.next = page_to_pdx(at);




[-- Attachment #2: page-list-splice.patch --]
[-- Type: text/plain, Size: 790 bytes --]

fix page_list_splice()

Other than in __list_splice(), the first element's prev pointer doesn't
need adjustment here - it already is PAGE_LIST_NULL. Rather than fixing
the assignment (to formally match __list_splice()), simply assert that
this assignment is really unnecessary.

Reported-by: Jisoo Yang <jisooy@gmail.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>

--- a/xen/include/xen/mm.h
+++ b/xen/include/xen/mm.h
@@ -270,7 +270,8 @@ page_list_splice(struct page_list_head *
     last = list->tail;
     at = head->next;
 
-    first->list.prev = page_to_pdx(head->next);
+    /* Both first->list.prev and at->list.prev are PAGE_LIST_NULL. */
+    ASSERT(first->list.prev == at->list.prev);
     head->next = first;
 
     last->list.next = page_to_pdx(at);

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] fix page_list_splice()

[Keir Fraser]

On 06/06/2012 09:23, "Jan Beulich" <JBeulich@suse.com> wrote:

> Other than in __list_splice(), the first element's prev pointer doesn't
> need adjustment here - it already is PAGE_LIST_NULL. Rather than fixing
> the assignment (to formally match __list_splice()), simply assert that
> this assignment is really unnecessary.
>
> Reported-by: Jisoo Yang <jisooy@gmail.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> --- a/xen/include/xen/mm.h
> +++ b/xen/include/xen/mm.h
> @@ -270,7 +270,8 @@ page_list_splice(struct page_list_head *
>      last = list->tail;
>      at = head->next;
>  
> -    first->list.prev = page_to_pdx(head->next);
> +    /* Both first->list.prev and at->list.prev are PAGE_LIST_NULL. */

ASSERT(first->list.prev == PAGE_LIST_NULL); ?

It seems odd to have one assumption encoded as an ASSERTion, and one as a
code comment... A second assertion makes the assumption explicit, and
run-time checked in debug builds.

 -- Keir

> +    ASSERT(first->list.prev == at->list.prev);
>      head->next = first;
>  
>      last->list.next = page_to_pdx(at);
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH] fix page_list_splice()

[Jan Beulich]

>>> On 06.06.12 at 11:02, Keir Fraser <keir@xen.org> wrote:
> On 06/06/2012 09:23, "Jan Beulich" <JBeulich@suse.com> wrote:
> 
>> Other than in __list_splice(), the first element's prev pointer doesn't
>> need adjustment here - it already is PAGE_LIST_NULL. Rather than fixing
>> the assignment (to formally match __list_splice()), simply assert that
>> this assignment is really unnecessary.
>>
>> Reported-by: Jisoo Yang <jisooy@gmail.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> 
>> --- a/xen/include/xen/mm.h
>> +++ b/xen/include/xen/mm.h
>> @@ -270,7 +270,8 @@ page_list_splice(struct page_list_head *
>>      last = list->tail;
>>      at = head->next;
>>  
>> -    first->list.prev = page_to_pdx(head->next);
>> +    /* Both first->list.prev and at->list.prev are PAGE_LIST_NULL. */
> 
> ASSERT(first->list.prev == PAGE_LIST_NULL); ?
> 
> It seems odd to have one assumption encoded as an ASSERTion, and one as a
> code comment... A second assertion makes the assumption explicit, and
> run-time checked in debug builds.

But the __list_splice() equivalent assignment would be

    first->list.prev = at->list.prev;

which is why I chose the assert expression that way, yet made
the comment clarify what the actual state is. If the comment
just repeated what the ASSERT() already says, I'd rather drop
the comment altogether.

Jan

Re: [PATCH] fix page_list_splice()

[Keir Fraser]

On 06/06/2012 10:26, "Jan Beulich" <JBeulich@suse.com> wrote:

>>>> On 06.06.12 at 11:02, Keir Fraser <keir@xen.org> wrote:
>> On 06/06/2012 09:23, "Jan Beulich" <JBeulich@suse.com> wrote:
>> 
>>> Other than in __list_splice(), the first element's prev pointer doesn't
>>> need adjustment here - it already is PAGE_LIST_NULL. Rather than fixing
>>> the assignment (to formally match __list_splice()), simply assert that
>>> this assignment is really unnecessary.
>>> 
>>> Reported-by: Jisoo Yang <jisooy@gmail.com>
>>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>>> 
>>> --- a/xen/include/xen/mm.h
>>> +++ b/xen/include/xen/mm.h
>>> @@ -270,7 +270,8 @@ page_list_splice(struct page_list_head *
>>>      last = list->tail;
>>>      at = head->next;
>>>  
>>> -    first->list.prev = page_to_pdx(head->next);
>>> +    /* Both first->list.prev and at->list.prev are PAGE_LIST_NULL. */
>> 
>> ASSERT(first->list.prev == PAGE_LIST_NULL); ?
>> 
>> It seems odd to have one assumption encoded as an ASSERTion, and one as a
>> code comment... A second assertion makes the assumption explicit, and
>> run-time checked in debug builds.
> 
> But the __list_splice() equivalent assignment would be
> 
>     first->list.prev = at->list.prev;
> 
> which is why I chose the assert expression that way, yet made
> the comment clarify what the actual state is. If the comment
> just repeated what the ASSERT() already says, I'd rather drop
> the comment altogether.

I mean to replace the comment with a second assertion, not to replace the
assertion your patch already adds. Does that make sense?

 -- Keir

> Jan
> 

Re: [PATCH] fix page_list_splice()

[Jan Beulich]

>>> Keir Fraser <keir.xen@gmail.com> 06/06/12 1:28 PM >>>
>On 06/06/2012 10:26, "Jan Beulich" <JBeulich@suse.com> wrote:
>>>>> On 06.06.12 at 11:02, Keir Fraser <keir@xen.org> wrote:
>>> On 06/06/2012 09:23, "Jan Beulich" <JBeulich@suse.com> wrote:
>>> +    /* Both first->list.prev and at->list.prev are PAGE_LIST_NULL. */
>>> 
>>> ASSERT(first->list.prev == PAGE_LIST_NULL); ?
>>> 
>>> It seems odd to have one assumption encoded as an ASSERTion, and one as a
>>> code comment... A second assertion makes the assumption explicit, and
>>> run-time checked in debug builds.
>> 
>> But the __list_splice() equivalent assignment would be
>> 
>>     first->list.prev = at->list.prev;
>> 
>> which is why I chose the assert expression that way, yet made
>> the comment clarify what the actual state is. If the comment
>> just repeated what the ASSERT() already says, I'd rather drop
>> the comment altogether.
>
>I mean to replace the comment with a second assertion, not to replace the
>assertion your patch already adds. Does that make sense?

>From the perspective of the operation here, we really just want to make
sure that the new list head's prev points to where the old one's pointed;
the fact that they're both supposed to be PAGE_LIST_NULL doesn't really
matter. Hence simply asserting that the assignment is superfluous seems
the best choice to me, with the comment explaining why. If you have
suggestions for better wording of the comment, I'll gladly take those.

The second best choice imo would be to just have the superfluous
assignment there (making it likely that later someone will come and
ask why it's there when not really needed).

Jan

Re: [PATCH] fix page_list_splice()

[Keir Fraser]

On 07/06/2012 20:10, "Jan Beulich" <jbeulich@suse.com> wrote:

>> I mean to replace the comment with a second assertion, not to replace the
>> assertion your patch already adds. Does that make sense?
> 
> From the perspective of the operation here, we really just want to make
> sure that the new list head's prev points to where the old one's pointed;
> the fact that they're both supposed to be PAGE_LIST_NULL doesn't really
> matter. Hence simply asserting that the assignment is superfluous seems
> the best choice to me, with the comment explaining why. If you have
> suggestions for better wording of the comment, I'll gladly take those.
> 
> The second best choice imo would be to just have the superfluous
> assignment there (making it likely that later someone will come and
> ask why it's there when not really needed).

Well, I already did check in a version with two assertions, yesterday. :) I
like assertions, and asserting things about the good form of the data
structure being manipulated, even if that good form is not essential to this
specific operation, is not necessarily bad. Especially while we're already
asserting other things about those same list pointers.

It's not a major point here of course, it just seemed silly to to me to take
up a source line with a comment which could be stated just as clearly with
an assertion, and with added benefit (imo).

 -- Keir

> Jan
>