change alloc function in pcpu_alloc_pages
in [Kernel]
|
Prev: linux-ide: hda: possibly failed opcode: 0x25 on Alpha with 2.6.34-rc3
Next: [PATCH]: x86: remove extra bootmem.h from arch/x86/mm/init_64.c
From: Tejun Heo on 15 Apr 2010 04:20 Hello, On 04/15/2010 05:00 PM, Minchan Kim wrote: > Yes. I don't like it. > With it, someone who does care about API usage uses alloc_pages_exact_node but > someone who don't have a time or careless uses alloc_pages_node. > It would make API fragmentation and not good. > Maybe we can weed out -1 and make new API which is more clear. > > * struct page *alloc_pages_any_node(gfp_t gfp_mask, unsigned int order); > * struct page *alloc_pages_exact_node(int nid, gfp_mask, unsigned int order); I'm not an expert on that part of the kernel but isn't alloc_pages_any_node() identical to alloc_pages_exact_node()? All that's necessary to do now is to weed out callers which pass in negative nid to alloc_pages_node(), right? If so, why not just do a clean sweep of alloc_pages_node() users and update them so that they don't call in w/ -1 nid and add WARN_ON_ONCE() in alloc_pages_node()? Is there any reason to keep both variants going forward? If not, introducing new API just to weed out invalid usages seems like an overkill. Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Minchan Kim on 15 Apr 2010 05:50 On Thu, Apr 15, 2010 at 5:15 PM, Tejun Heo <tj(a)kernel.org> wrote: > Hello, > > On 04/15/2010 05:00 PM, Minchan Kim wrote: >> Yes. I don't like it. >> With it, someone who does care about API usage uses alloc_pages_exact_node but >> someone who don't have a time or careless uses alloc_pages_node. >> It would make API fragmentation and not good. >> Maybe we can weed out -1 and make new API which is more clear. >> >> * struct page *alloc_pages_any_node(gfp_t gfp_mask, unsigned int order); >> * struct page *alloc_pages_exact_node(int nid, gfp_mask, unsigned int order); > > I'm not an expert on that part of the kernel but isn't > alloc_pages_any_node() identical to alloc_pages_exact_node()? All alloc_pages_any_node means user allows allocated pages in any node(most likely current node) alloc_pages_exact_node means user allows allocated pages in nid node if he doesn't use __GFP_THISNODE. > that's necessary to do now is to weed out callers which pass in > negative nid to alloc_pages_node(), right? If so, why not just do a It might be my final goal. I hope user uses alloc_pages_any_node instead of nid == -1. > clean sweep of alloc_pages_node() users and update them so that they > don't call in w/ -1 nid and add WARN_ON_ONCE() in alloc_pages_node()? > Is there any reason to keep both variants going forward? If not, I am not sure someone still need alloc_pages_node. That's because sometime he want to allocate page from specific node but sometime not. I hope it doesn't happen. Anyway I have to identify the situation. > introducing new API just to weed out invalid usages seems like an > overkill. It might be. It think it's almost same add_to_page_cache and add_to_page_cache_locked. If user knows the page is already locked, he can use add_to_page_cache_locked for performance gain and code readability which we need to lock the page before calling it. The important point is that user uses it as he is conscious of locked page. I think if user already know to want page where from specific node, it would be better to use alloc_pages_exact_node instead of alloc_pages_node. If he want to allocate page from any node[current..fallback list], he have to use alloc_pages_any_node without nid argument. It would make a little performance gain(reduce passing argument) and good readbility. :) Now, most of user uses alloc_pages_exact_node. So I think we can do it. But someone else also might think of overkill. :) It's a not urgent issue. So I will take it easy. Thanks. -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Tejun Heo on 15 Apr 2010 06:10 Hello, On 04/15/2010 06:40 PM, Minchan Kim wrote: >> I'm not an expert on that part of the kernel but isn't >> alloc_pages_any_node() identical to alloc_pages_exact_node()? All > > alloc_pages_any_node means user allows allocated pages in any > node(most likely current node) alloc_pages_exact_node means user > allows allocated pages in nid node if he doesn't use __GFP_THISNODE. Ooh, sorry, I meant alloc_pages(). What would be the difference between alloc_pages_any_node() and alloc_pages()? >> introducing new API just to weed out invalid usages seems like an >> overkill. > > It might be. > > It think it's almost same add_to_page_cache and add_to_page_cache_locked. > If user knows the page is already locked, he can use > add_to_page_cache_locked for performance gain and code readability > which we need to lock the page before calling it. Yeah, if both APIs are necessary at the end of the conversion, sure. I was just saying that introducing new APIs just to weed out invalid usages and then later killing the old API would be rather excessive. I was just wondering whether we could just clean up alloc_pages_node() users and kill alloc_pages_exact_node(). Thanks. -- tejun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Minchan Kim on 15 Apr 2010 06:30 On Thu, Apr 15, 2010 at 7:08 PM, Tejun Heo <tj(a)kernel.org> wrote: > Hello, > > On 04/15/2010 06:40 PM, Minchan Kim wrote: >>> I'm not an expert on that part of the kernel but isn't >>> alloc_pages_any_node() identical to alloc_pages_exact_node()? All >> >> alloc_pages_any_node means user allows allocated pages in any >> node(most likely current node) alloc_pages_exact_node means user >> allows allocated pages in nid node if he doesn't use __GFP_THISNODE. > > Ooh, sorry, I meant alloc_pages(). What would be the difference > between alloc_pages_any_node() and alloc_pages()? It's no different. It's same. Just naming is more explicit. :) I think it could be following as. #define alloc_pages alloc_pages_any_node. strucdt page * alloc_pages_node() { int nid = numa_node_id(); ... return page; } > >>> introducing new API just to weed out invalid usages seems like an >>> overkill. >> >> It might be. >> >> It think it's almost same add_to_page_cache and add_to_page_cache_locked. >> If user knows the page is already locked, he can use >> add_to_page_cache_locked for performance gain and code readability >> which we need to lock the page before calling it. > > Yeah, if both APIs are necessary at the end of the conversion, sure. > I was just saying that introducing new APIs just to weed out invalid > usages and then later killing the old API would be rather excessive. > > I was just wondering whether we could just clean up alloc_pages_node() > users and kill alloc_pages_exact_node(). kill alloc_pages_exact_node? Sorry but I can't understand your point. I don't want to kill user of alloc_pages_exact_node. That's opposite. I want to kill user of alloc_pages_node and change it with alloc_pages_any_node or alloc_pages_exact_node. :) I think we can do it. That's because all of caller already can check nid == -1 before calling allocation function explicitly if he cares node locality. -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Minchan Kim on 15 Apr 2010 06:40
On Thu, Apr 15, 2010 at 7:21 PM, Minchan Kim <minchan.kim(a)gmail.com> wrote: > On Thu, Apr 15, 2010 at 7:08 PM, Tejun Heo <tj(a)kernel.org> wrote: >> Hello, >> >> On 04/15/2010 06:40 PM, Minchan Kim wrote: >>>> I'm not an expert on that part of the kernel but isn't >>>> alloc_pages_any_node() identical to alloc_pages_exact_node()? All >>> >>> alloc_pages_any_node means user allows allocated pages in any >>> node(most likely current node) alloc_pages_exact_node means user >>> allows allocated pages in nid node if he doesn't use __GFP_THISNODE. >> >> Ooh, sorry, I meant alloc_pages(). What would be the difference >> between alloc_pages_any_node() and alloc_pages()? > > It's no different. It's same. Just naming is more explicit. :) > I think it could be following as. > > #define alloc_pages alloc_pages_any_node. > strucdt page * alloc_pages_node() { typo. Sorry. struct page * alloc_pages_any_node { > int nid = numa_node_id(); > ... > return page; > } > -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |