lkml.org 
[lkml]   [2013]   [Jul]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 4/9] mm, hugetlb: fix and clean-up node iteration code to alloc or free
    Date
    Joonsoo Kim <iamjoonsoo.kim@lge.com> writes:

    > Current node iteration code have a minor problem which do one more
    > node rotation if we can't succeed to allocate. For example,
    > if we start to allocate at node 0, we stop to iterate at node 0.
    > Then we start to allocate at node 1 for next allocation.

    Can you explain the problem in a bit more detail

    >
    > I introduce new macros "for_each_node_mask_to_[alloc|free]" and
    > fix and clean-up node iteration code to alloc or free.
    > This makes code more understandable.
    >

    I found the existing code more readable. Obviously I haven't yet figured
    out the problem you have observed with the code.

    > Signed-off-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
    >
    > diff --git a/mm/hugetlb.c b/mm/hugetlb.c
    > index 0067cf4..a838e6b 100644
    > --- a/mm/hugetlb.c
    > +++ b/mm/hugetlb.c
    > @@ -752,33 +752,6 @@ static int hstate_next_node_to_alloc(struct hstate *h,
    > return nid;
    > }
    >
    > -static int alloc_fresh_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
    > -{
    > - struct page *page;
    > - int start_nid;
    > - int next_nid;
    > - int ret = 0;
    > -
    > - start_nid = hstate_next_node_to_alloc(h, nodes_allowed);
    > - next_nid = start_nid;
    > -
    > - do {
    > - page = alloc_fresh_huge_page_node(h, next_nid);
    > - if (page) {
    > - ret = 1;
    > - break;
    > - }
    > - next_nid = hstate_next_node_to_alloc(h, nodes_allowed);
    > - } while (next_nid != start_nid);
    > -
    > - if (ret)
    > - count_vm_event(HTLB_BUDDY_PGALLOC);
    > - else
    > - count_vm_event(HTLB_BUDDY_PGALLOC_FAIL);
    > -
    > - return ret;
    > -}
    > -
    > /*
    > * helper for free_pool_huge_page() - return the previously saved
    > * node ["this node"] from which to free a huge page. Advance the
    > @@ -797,6 +770,42 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
    > return nid;
    > }
    >
    > +#define for_each_node_mask_to_alloc(hs, nr_nodes, node, mask) \
    > + for (nr_nodes = nodes_weight(*mask), \
    > + node = hstate_next_node_to_alloc(hs, mask); \
    > + nr_nodes > 0 && \
    > + ((node = hstate_next_node_to_alloc(hs, mask)) || 1); \
    > + nr_nodes--)
    > +
    > +#define for_each_node_mask_to_free(hs, nr_nodes, node, mask) \
    > + for (nr_nodes = nodes_weight(*mask), \
    > + node = hstate_next_node_to_free(hs, mask); \
    > + nr_nodes > 0 && \
    > + ((node = hstate_next_node_to_free(hs, mask)) || 1); \
    > + nr_nodes--)
    > +
    > +static int alloc_fresh_huge_page(struct hstate *h, nodemask_t *nodes_allowed)
    > +{
    > + struct page *page;
    > + int nr_nodes, node;
    > + int ret = 0;
    > +
    > + for_each_node_mask_to_alloc(h, nr_nodes, node, nodes_allowed) {


    This check for nodes_weight and fail right ? (nr_nodes == 0). That is
    not the case with the existing code. It will allocate from
    h->next_nid_to_alloc. Is that ok ?


    > + page = alloc_fresh_huge_page_node(h, node);
    > + if (page) {
    > + ret = 1;
    > + break;
    > + }
    > + }
    > +
    > + if (ret)
    > + count_vm_event(HTLB_BUDDY_PGALLOC);
    > + else
    > + count_vm_event(HTLB_BUDDY_PGALLOC_FAIL);
    > +
    > + return ret;
    > +}
    > +

    -aneesh



    \
     
     \ /
      Last update: 2013-07-15 19:01    [W:3.936 / U:0.164 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site