Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1645593 > unrolled thread

Re: [PATCH v5 02/11] mm: mempolicy: add queue_pages_node_check()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 02/11] mm: mempolicy: add queue_pages_node_check() Anshuman Khandual <khandual@linux.vnet.ibm.com> - 2017-05-19 15:20 +0200
    Re: [PATCH v5 02/11] mm: mempolicy: add queue_pages_node_check() Mel Gorman <mgorman@techsingularity.net> - 2017-05-19 18:10 +0200
      Re: [PATCH v5 02/11] mm: mempolicy: add queue_pages_node_check() Mel Gorman <mgorman@techsingularity.net> - 2017-05-19 22:40 +0200
        Re: [PATCH v5 02/11] mm: mempolicy: add queue_pages_node_check() Mel Gorman <mgorman@techsingularity.net> - 2017-05-19 23:40 +0200

#1645593 — Re: [PATCH v5 02/11] mm: mempolicy: add queue_pages_node_check()

On 04/21/2017 09:34 AM, Anshuman Khandual wrote:
> On 04/21/2017 02:17 AM, Zi Yan wrote:
>> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
>>
>> Introduce a separate check routine related to MPOL_MF_INVERT flag.
>> This patch just does cleanup, no behavioral change.
> 
> Can you please send it separately first, this should be debated
> and merged quickly and not hang on to the series if we have to
> respin again.
> 
> Reviewed-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>

Mel/Andrew,

This does not have any functional changes and very much independent.
Can this clean up be accepted as is ? In that case we will have to
carry one less patch in the series which can make the review process
simpler.

- Anshuman

[toc] | [next] | [standalone]

#1645733

On Fri, May 19, 2017 at 06:43:37PM +0530, Anshuman Khandual wrote:
> On 04/21/2017 09:34 AM, Anshuman Khandual wrote:
> > On 04/21/2017 02:17 AM, Zi Yan wrote:
> >> From: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
> >>
> >> Introduce a separate check routine related to MPOL_MF_INVERT flag.
> >> This patch just does cleanup, no behavioral change.
> > 
> > Can you please send it separately first, this should be debated
> > and merged quickly and not hang on to the series if we have to
> > respin again.
> > 
> > Reviewed-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
> 
> Mel/Andrew,
> 
> This does not have any functional changes and very much independent.
> Can this clean up be accepted as is ? In that case we will have to
> carry one less patch in the series which can make the review process
> simpler.
> 

As you say, there is no functional change but the helper name is vague
and gives no hint to what's it's checking for. It's somewhat tolerable as
it is as it's obvious what is being checked but the same is not true with
the helper name.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1645914

On Fri, May 19, 2017 at 12:37:38PM -0400, Zi Yan wrote:
> > As you say, there is no functional change but the helper name is vague
> > and gives no hint to what's it's checking for. It's somewhat tolerable as
> > it is as it's obvious what is being checked but the same is not true with
> > the helper name.
> >
> 
> Does queue_pages_invert_nodemask_check() work? I can change the helper name
> in the next version.
> 

Not particularly, maybe queue_pages_required and invert the check with a
comment above it explaining what it's checking for would be ok.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1645969

On Fri, May 19, 2017 at 04:48:54PM -0400, Zi Yan wrote:
> On 19 May 2017, at 16:28, Mel Gorman wrote:
> 
> > On Fri, May 19, 2017 at 12:37:38PM -0400, Zi Yan wrote:
> >>> As you say, there is no functional change but the helper name is vague
> >>> and gives no hint to what's it's checking for. It's somewhat tolerable as
> >>> it is as it's obvious what is being checked but the same is not true with
> >>> the helper name.
> >>>
> >>
> >> Does queue_pages_invert_nodemask_check() work? I can change the helper name
> >> in the next version.
> >>
> >
> > Not particularly, maybe queue_pages_required and invert the check with a
> > comment above it explaining what it's checking for would be ok.
> >
> 
> queue_pages_required() is too broad,

I'm somewhat amused that you'd complain that "required" is too broad while
thinking "check" is somehow self-explanatory.

> I would take queue_pages_page_nid_check()
> and invert the check with a comment above saying
> 
> /*
>  * Check if the page's nid is in qp->nmask.
>  *
>  * If MPOL_MF_INVERT is set in qp->flags, check if the nid is
>  * in the invert of qp->nmask.
>  */
> 
> Does it work?
> 

I still don't like the name but I also am not interested in debating it
further for something so small. Add the comment, it's better than nothing.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web