From mboxrd@z Thu Jan  1 00:00:00 1970
From: Keir Fraser <keir.xen@gmail.com>
Subject: Re: [PATCH] fix page_list_splice()
Date: Wed, 06 Jun 2012 12:22:07 +0100
Message-ID: <CBF4FBEF.3558E%keir.xen@gmail.com>
References: <4FCF3E5002000078000886D7@nat28.tlf.novell.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <4FCF3E5002000078000886D7@nat28.tlf.novell.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: Jan Beulich <JBeulich@suse.com>
Cc: Jisoo Yang <jisooy@gmail.com>, xen-devel <xen-devel@lists.xen.org>
List-Id: xen-devel@lists.xenproject.org

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
>