Messages in this thread | | | Date | Fri, 8 Mar 2002 01:11:45 +0100 | From | Andrea Arcangeli <> | Subject | Re: 2.4.19pre2aa1 |
| |
On Thu, Mar 07, 2002 at 12:18:19PM -0800, William Lee Irwin III wrote: > On Thu, Mar 07, 2002 at 06:03:00PM +0100, Andrea Arcangeli wrote: > > For the other points I think you shouldn't really complain (both at > > runtime and in code style as well, please see how clean it is with the > > wait_table_t thing), I made a definitive improvement to your code, the > > only not obvious part is the hashfn but I really cannot see yours > > beating mine because of the total random input, infact it could be the > > other way around due the fact if something there's the probability the > > pages are physically consecutive and I take care of that fine. > > > I don't know whose definition of clean code this is: > > +static inline wait_queue_head_t * wait_table_hashfn(struct page * page, wait_table_t * wait_table) > +{ > +#define i (((unsigned long) page)/(sizeof(struct page) & ~ (sizeof(struct page) - 1))) > +#define s(x) ((x)+((x)>>wait_table->shift)) > + return wait_table->head + (s(i) & (wait_table->size-1)); > +#undef i > +#undef s > +} > > > I'm not sure I want to find out.
The above is again the hashfunction, the hashfn code doesn't need to be nice, the API around wait_table_hashfn has to instead. See the above wait_table_t typedef.
During some further auditing I also noticed now that you introduced a certain usused wake_up_page. That's buggy, if you use it you'll deadlock. Also it would be cleaner if __lock_page wasn't using the exclusive waitqueue and that in turn you would keep using wake_up for unlock_page. By the time you share the waitqueue nothing can be wake one any longer, this is probably the worst drawback of the wait_table memory-saving patch. Infact I was considering to solve the collisions with additional memory, rather than by having to drop the wake-one behaviour when many threads are working on the same chunk of the file that your design solution requires. quite frankly I don't think this was an urgent thing to change in 2.4 (it only saves some memory and even if 64G will now boot with CONFIG_1G, the lowmem will be way too much unbalanced to be good for general purpose).
Andrea - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |