On Sun, 23 Mar 2008, Manish Katiyar wrote:
> On Sun, Mar 23, 2008 at 6:34 PM, Robert P. J. Day <rpjday@xxxxxxxxxxxxxx> wrote:
> > On Sun, 23 Mar 2008, Robert P. J. Day wrote:
> >
> > ... snip ...
> >
> >
> > > an obvious fix is, once you do the splice, initialize that list
> > > header to designate an empty list, and in fact, there's a list
> > > function that does just that:
> > >
> > > =====
> > > static inline void list_splice_init(struct list_head *list,
> > > struct list_head *head)
> > > {
> > > if (!list_empty(list)) {
> > > __list_splice(list, head);
> > > INIT_LIST_HEAD(list); <-- that solves the problem
> > > }
> > > }
> > > =====
> > >
> > > which makes me wonder ... why aren't *all* list splices treated that
> > > way? what is the value of using just the first form and leaving that
> > > potentially dangerous head element lying around? in short, as long as
> > > i've understood this correctly, it would seem to make more sense for
> > > all list_splice() calls to be list_splice_init() instead, for safety.
> > > or did i screw something up?
> >
> > just to follow up on this post, i was wondering why anyone would call
> > list_splice() *without* doing a list head init on that detached list
> > head element just to play it safe, so i did a grep for calls to
> > list_splice() under drivers and, lo and behold, here's the very first
> > example that showed up (drivers/ieee1394/ohci1394.c):
> >
> > ...
> > list_splice(&d->fifo_list, &packet_list);
> > list_splice(&d->pending_list, &packet_list);
> > INIT_LIST_HEAD(&d->fifo_list);
> > INIT_LIST_HEAD(&d->pending_list);
> > ...
>
> Interesting point Robert, I agree. However not every call to
> list_splice is accompanied by a call to INIT_LIST_HEAD in the current
> code (26.24.2) and I can see this *seem to be buggy* call in some of
> the important files like VM and cache. One such example is in
>
> kmem_cache_shrink
> shrink_page_list
>
> etc. and lot of others. Wonder how they work unless we are missing
> something :-)
ah, i see what's happening. if that list is statically allocated
within a function so that it is unallocated at function return time,
then you can afford to be "sloppy", as long as you're feeling
confident that you won't do something silly with that dangling head in
the meantime. and there's a *lot* of those but, if it's not going to
last long, i guess it would be wasteful to clear those pointers for no
good reason. i'm ok with that.
what *would* be worrisome are examples of the above where that
dangling head hangs around for a long time. and, again, just leaving
those dangling heads with valid element pointers in them doesn't
necessarily break anything, it's just, well, potentially unsafe.
rday
--
========================================================================
Robert P. J. Day
Linux Consulting, Training and Annoying Kernel Pedantry:
Have classroom, will lecture.
http://crashcourse.ca Waterloo, Ontario, CANADA
========================================================================
--
To unsubscribe from this list: send an email with
"unsubscribe kernelnewbies" to ecartis@xxxxxxxxxxxx
Please read the FAQ at http://kernelnewbies.org/FAQ
