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);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
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
>>> 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
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 >
>>> 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
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 >