lkml.org 
[lkml]   [2005]   [Jan]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC] per thread page reservation patch
    > diff -puN include/linux/gfp.h~reiser4-perthread-pages include/linux/gfp.h
    > --- linux-2.6.10-rc3/include/linux/gfp.h~reiser4-perthread-pages 2004-12-22 20:09:44.153164276 +0300
    > +++ linux-2.6.10-rc3-vs/include/linux/gfp.h 2004-12-22 20:09:44.199167794 +0300
    > @@ -134,4 +134,8 @@ extern void FASTCALL(free_cold_page(stru
    >
    > void page_alloc_init(void);
    >
    > +int perthread_pages_reserve(int nrpages, int gfp);
    > +void perthread_pages_release(int nrpages);
    > +int perthread_pages_count(void);
    > +
    > #endif /* __LINUX_GFP_H */
    > diff -puN include/linux/init_task.h~reiser4-perthread-pages include/linux/init_task.h
    > --- linux-2.6.10-rc3/include/linux/init_task.h~reiser4-perthread-pages 2004-12-22 20:09:44.160164812 +0300
    > +++ linux-2.6.10-rc3-vs/include/linux/init_task.h 2004-12-22 20:09:44.201167947 +0300
    > @@ -112,8 +112,8 @@ extern struct group_info init_groups;
    > .proc_lock = SPIN_LOCK_UNLOCKED, \
    > .switch_lock = SPIN_LOCK_UNLOCKED, \
    > .journal_info = NULL, \
    > + .private_pages = LIST_HEAD_INIT(tsk.private_pages), \
    > + .private_pages_count = 0, \
    > }
    >
    > -
    > -
    > #endif
    > diff -puN include/linux/sched.h~reiser4-perthread-pages include/linux/sched.h
    > --- linux-2.6.10-rc3/include/linux/sched.h~reiser4-perthread-pages 2004-12-22 20:09:44.170165576 +0300
    > +++ linux-2.6.10-rc3-vs/include/linux/sched.h 2004-12-22 20:09:44.204168176 +0300
    > @@ -691,6 +691,9 @@ struct task_struct {
    > nodemask_t mems_allowed;
    > int cpuset_mems_generation;
    > #endif
    > +
    > + struct list_head private_pages; /* per-process private pages */
    > + int private_pages_count;
    > };
    >
    > static inline pid_t process_group(struct task_struct *tsk)
    > diff -puN kernel/fork.c~reiser4-perthread-pages kernel/fork.c
    > --- linux-2.6.10-rc3/kernel/fork.c~reiser4-perthread-pages 2004-12-22 20:09:44.179166264 +0300
    > +++ linux-2.6.10-rc3-vs/kernel/fork.c 2004-12-22 20:09:44.215169017 +0300
    > @@ -154,6 +154,10 @@ static struct task_struct *dup_task_stru
    > tsk->thread_info = ti;
    > ti->task = tsk;
    >
    > + /* initialize list of pages privately reserved by process */
    > + INIT_LIST_HEAD(&tsk->private_pages);
    > + tsk->private_pages_count = 0;
    > +
    > /* One for us, one for whoever does the "release_task()" (usually parent) */
    > atomic_set(&tsk->usage,2);
    > return tsk;
    > diff -puN mm/page_alloc.c~reiser4-perthread-pages mm/page_alloc.c
    > --- linux-2.6.10-rc3/mm/page_alloc.c~reiser4-perthread-pages 2004-12-22 20:09:44.187166876 +0300
    > +++ linux-2.6.10-rc3-vs/mm/page_alloc.c 2004-12-22 20:09:44.219169323 +0300
    > @@ -551,6 +551,97 @@ void fastcall free_cold_page(struct page
    > free_hot_cold_page(page, 1);
    > }
    >
    > +static inline struct list_head *get_per_thread_pages(void)
    > +{
    > + return &current->private_pages;
    > +}

    this is a completely useless wrapper that doesn't buy anything.

    > +int perthread_pages_reserve(int nrpages, int gfp)
    > +{
    > + int i;
    > + struct list_head accumulator;
    > + struct list_head *per_thread;
    > +
    > + per_thread = get_per_thread_pages();
    > + INIT_LIST_HEAD(&accumulator);
    > + list_splice_init(per_thread, &accumulator);

    Can probably be written much simpler as:

    int perthread_pages_reserve(int nrpages, int gfp)
    {
    LIST_HEAD(accumulator);
    int i;

    list_splice_init(&current->private_pages, &accumulator);

    Now the big question is, what's synchronizing access to
    current->private_pages?

    > + for (i = 0; i < nrpages; ++i) {
    > + struct page *page;
    > +
    > + page = alloc_page(gfp);
    > + if (page != NULL)
    > + list_add(&page->lru, &accumulator);
    > + else {
    > + for (; i > 0; --i) {
    > + page = list_entry(accumulator.next, struct page, lru);
    > + list_del(&page->lru);
    > + page_cache_release(page);
    > + }
    > + return -ENOMEM;
    > + }
    > + }

    Would be much more readable with the error condition handling at the
    end of the function and a goto.

    > +int perthread_pages_count(void)
    > +{
    > + return current->private_pages_count;
    > +}
    > +EXPORT_SYMBOL(perthread_pages_count);

    Again a completely useless wrapper.

    > +static inline struct page *
    > +perthread_pages_alloc(void)
    > +{
    > + struct list_head *perthread_pages;
    > +
    > + /*
    > + * try to allocate pages from the per-thread private_pages pool. No
    > + * locking is needed: this list can only be modified by the thread
    > + * itself, and not by interrupts or other threads.
    > + */
    > + perthread_pages = get_per_thread_pages();
    > + if (!in_interrupt() && !list_empty(perthread_pages)) {
    > + struct page *page;
    > +
    > + page = list_entry(perthread_pages->next, struct page, lru);
    > + list_del(&page->lru);
    > + current->private_pages_count--;
    > + /*
    > + * per-thread page is already initialized, just return it.
    > + */
    > + return page;
    > + } else
    > + return NULL;
    > +}

    Would be written much cleaner with a single return statement and the
    comment above the function, ala:

    /*
    * try to allocate pages from the per-thread private_pages pool. No
    * locking is needed: this list can only be modified by the thread
    * itself, and not by interrupts or other threads.
    */
    static inline struct page *perthread_pages_alloc(void)
    {
    struct list_head *perthread_pages = &current->private_pages;
    struct page *page = NULL;

    if (!in_interrupt() && !list_empty(perthread_pages)) {
    page = list_entry(perthread_pages->next, struct page, lru);
    list_del(&page->lru);
    current->private_pages_count--;
    }

    return page;
    }

    > + if (order == 0) {
    > + page = perthread_pages_alloc();
    > + if (page != NULL)
    > + return page;
    > + }
    > +

    You should at least move the !in_interrupt() condition out here.

    -
    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/

    \
     
     \ /
      Last update: 2005-03-22 14:09    [W:0.031 / U:0.032 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site