| Message ID | 20210127201031.98544-1-alobakin@pm.me |
|---|---|
| Headers | show |
| Series | net: consolidate page_is_pfmemalloc() usage | expand |
Alexander Lobakin wrote: > A bunch of drivers test the page before reusing/recycling for two > common conditions: > - if a page was allocated under memory pressure (pfmemalloc page); > - if a page was allocated at a distant memory node (to exclude > slowdowns). > > Introduce and use a new common function for doing this and eliminate > all functions-duplicates from drivers. > > Suggested-by: David Rientjes <rientjes@google.com> > Signed-off-by: Alexander Lobakin <alobakin@pm.me> > --- > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 10 ++-------- > drivers/net/ethernet/intel/fm10k/fm10k_main.c | 9 ++------- > drivers/net/ethernet/intel/i40e/i40e_txrx.c | 15 +-------------- > drivers/net/ethernet/intel/iavf/iavf_txrx.c | 15 +-------------- > drivers/net/ethernet/intel/ice/ice_txrx.c | 11 +---------- > drivers/net/ethernet/intel/igb/igb_main.c | 7 +------ > drivers/net/ethernet/intel/igc/igc_main.c | 7 +------ > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 7 +------ > drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 7 +------ > drivers/net/ethernet/mellanox/mlx5/core/en_rx.c | 7 +------ > include/linux/skbuff.h | 15 +++++++++++++++ > 11 files changed, 27 insertions(+), 83 deletions(-) For the patch, and esp. for the Intel drivers: Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
On Wed, 27 Jan 2021, Alexander Lobakin wrote: > The function only tests for page->index, so its argument should be > const. > > Signed-off-by: Alexander Lobakin <alobakin@pm.me> Acked-by: David Rientjes <rientjes@google.com>
On Wed, 27 Jan 2021, Alexander Lobakin wrote: > A bunch of drivers test the page before reusing/recycling for two > common conditions: > - if a page was allocated under memory pressure (pfmemalloc page); > - if a page was allocated at a distant memory node (to exclude > slowdowns). > > Introduce and use a new common function for doing this and eliminate > all functions-duplicates from drivers. > > Suggested-by: David Rientjes <rientjes@google.com> > Signed-off-by: Alexander Lobakin <alobakin@pm.me> Looks even better than I thought! (Since all of the changes are in drivers/net/ethernet/, I assume everything directly or indirectly includes skbuff.h already.) Acked-by: David Rientjes <rientjes@google.com> Thanks for doing this.
On Wed, 27 Jan 2021 20:11:01 +0000 Alexander Lobakin wrote: > The function only tests for page->index, so its argument should be > const. > > Signed-off-by: Alexander Lobakin <alobakin@pm.me> > --- > include/linux/mm.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index ecdf8a8cd6ae..078633d43af9 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1584,7 +1584,7 @@ struct address_space *page_mapping_file(struct page *page); > * ALLOC_NO_WATERMARKS and the low watermark was not > * met implying that the system is under some pressure. > */ > -static inline bool page_is_pfmemalloc(struct page *page) > +static inline bool page_is_pfmemalloc(const struct page *page) > { > /* > * Page index cannot be this large so this must be No objections for this going via net-next?
On Wed, 27 Jan 2021 20:11:23 +0000 Alexander Lobakin wrote: > + * dev_page_is_reserved - check whether a page can be reused for network Rx > + * @page: the page to test > + * > + * A page shouldn't be considered for reusing/recycling if it was allocated > + * under memory pressure or at a distant memory node. > + * > + * Returns true if this page should be returned to page allocator, false > + * otherwise. > + */ > +static inline bool dev_page_is_reserved(const struct page *page) Am I the only one who feels like "reusable" is a better term than "reserved".
From: Jakub Kicinski <kuba@kernel.org> Date: Fri, 29 Jan 2021 18:39:07 -0800 > On Wed, 27 Jan 2021 20:11:23 +0000 Alexander Lobakin wrote: > > + * dev_page_is_reserved - check whether a page can be reused for network Rx > > + * @page: the page to test > > + * > > + * A page shouldn't be considered for reusing/recycling if it was allocated > > + * under memory pressure or at a distant memory node. > > + * > > + * Returns true if this page should be returned to page allocator, false > > + * otherwise. > > + */ > > +static inline bool dev_page_is_reserved(const struct page *page) > > Am I the only one who feels like "reusable" is a better term than > "reserved". I thought about it, but this will need to inverse the conditions in most of the drivers. I decided to keep it as it is. I can redo if "reusable" is preferred. Regarding "no objectives to take patch 1 through net-next": patches 2-3 depend on it, so I can't put it in a separate series. Thanks, Al
On Sat, 30 Jan 2021 15:42:29 +0000 Alexander Lobakin wrote: > > On Wed, 27 Jan 2021 20:11:23 +0000 Alexander Lobakin wrote: > > > + * dev_page_is_reserved - check whether a page can be reused for network Rx > > > + * @page: the page to test > > > + * > > > + * A page shouldn't be considered for reusing/recycling if it was allocated > > > + * under memory pressure or at a distant memory node. > > > + * > > > + * Returns true if this page should be returned to page allocator, false > > > + * otherwise. > > > + */ > > > +static inline bool dev_page_is_reserved(const struct page *page) > > > > Am I the only one who feels like "reusable" is a better term than > > "reserved". > > I thought about it, but this will need to inverse the conditions in > most of the drivers. I decided to keep it as it is. > I can redo if "reusable" is preferred. Naming is hard. As long as the condition is not a double negative it reads fine to me, but that's probably personal preference. The thing that doesn't sit well is the fact that there is nothing "reserved" about a page from another NUMA node.. But again, if nobody +1s this it's whatever... That said can we move the likely()/unlikely() into the helper itself? People on the internet may say otherwise but according to my tests using __builtin_expect() on a return value of a static inline helper works just fine.
From: Jakub Kicinski <kuba@kernel.org> Date: Sat, 30 Jan 2021 11:07:07 -0800 > On Sat, 30 Jan 2021 15:42:29 +0000 Alexander Lobakin wrote: > > > On Wed, 27 Jan 2021 20:11:23 +0000 Alexander Lobakin wrote: > > > > + * dev_page_is_reserved - check whether a page can be reused for network Rx > > > > + * @page: the page to test > > > > + * > > > > + * A page shouldn't be considered for reusing/recycling if it was allocated > > > > + * under memory pressure or at a distant memory node. > > > > + * > > > > + * Returns true if this page should be returned to page allocator, false > > > > + * otherwise. > > > > + */ > > > > +static inline bool dev_page_is_reserved(const struct page *page) > > > > > > Am I the only one who feels like "reusable" is a better term than > > > "reserved". > > > > I thought about it, but this will need to inverse the conditions in > > most of the drivers. I decided to keep it as it is. > > I can redo if "reusable" is preferred. > > Naming is hard. As long as the condition is not a double negative it > reads fine to me, but that's probably personal preference. > The thing that doesn't sit well is the fact that there is nothing > "reserved" about a page from another NUMA node.. But again, if nobody > +1s this it's whatever... Agree on NUMA and naming. I'm a bit surprised that 95% of drivers have this helper called "reserved" (one of the reasons why I finished with this variant). Let's say, if anybody else will vote for "reusable", I'll pick it for v3. > That said can we move the likely()/unlikely() into the helper itself? > People on the internet may say otherwise but according to my tests > using __builtin_expect() on a return value of a static inline helper > works just fine. Sounds fine, this will make code more elegant. Will publish v3 soon. Thanks, Al
On 1/30/21 11:45 AM, Alexander Lobakin wrote: > From: Jakub Kicinski <kuba@kernel.org> > Date: Sat, 30 Jan 2021 11:07:07 -0800 > >> On Sat, 30 Jan 2021 15:42:29 +0000 Alexander Lobakin wrote: >>>> On Wed, 27 Jan 2021 20:11:23 +0000 Alexander Lobakin wrote: >>>>> + * dev_page_is_reserved - check whether a page can be reused for network Rx >>>>> + * @page: the page to test >>>>> + * >>>>> + * A page shouldn't be considered for reusing/recycling if it was allocated >>>>> + * under memory pressure or at a distant memory node. >>>>> + * >>>>> + * Returns true if this page should be returned to page allocator, false >>>>> + * otherwise. >>>>> + */ >>>>> +static inline bool dev_page_is_reserved(const struct page *page) >>>> >>>> Am I the only one who feels like "reusable" is a better term than >>>> "reserved". >>> >>> I thought about it, but this will need to inverse the conditions in >>> most of the drivers. I decided to keep it as it is. >>> I can redo if "reusable" is preferred. >> >> Naming is hard. As long as the condition is not a double negative it >> reads fine to me, but that's probably personal preference. >> The thing that doesn't sit well is the fact that there is nothing >> "reserved" about a page from another NUMA node.. But again, if nobody >> +1s this it's whatever... > > Agree on NUMA and naming. I'm a bit surprised that 95% of drivers > have this helper called "reserved" (one of the reasons why I finished > with this variant). > Let's say, if anybody else will vote for "reusable", I'll pick it for > v3. Definitely "reusable" seems better to me, and especially anything *other* than "reserved" is a good idea, IMHO. thanks, -- John Hubbard NVIDIA