All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: Anthony Yznaga <anthony.yznaga@oracle.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	adobriyan@gmail.com, akpm@linux-foundation.org, vbabka@suse.cz,
	sfr@canb.auug.org.au, kirill.shutemov@linux.intel.com,
	rppt@linux.vnet.ibm.com, mhocko@suse.com,
	alexander.h.duyck@linux.intel.com, hannes@cmpxchg.org,
	miles.chen@mediatek.com, n-horiguchi@ah.jp.nec.com
Subject: Re: [PATCH] /proc/kpagecount: return 0 for special pages that are never mapped
Date: Wed, 5 Dec 2018 11:44:09 -0800	[thread overview]
Message-ID: <20181205194409.GB11646@bombadil.infradead.org> (raw)
In-Reply-To: <540f7690-5fcd-04d6-edb3-a44ebd09e70c@oracle.com>

On Wed, Dec 05, 2018 at 11:40:51AM -0800, Anthony Yznaga wrote:
> On 12/04/2018 05:25 PM, Matthew Wilcox wrote:
> > On Tue, Dec 04, 2018 at 05:18:32PM -0800, anthony.yznaga@oracle.com wrote:
> >> On 12/04/2018 04:48 PM, Matthew Wilcox wrote:
> >>> On Tue, Dec 04, 2018 at 02:45:26PM -0800, Anthony Yznaga wrote:
> >>>> +static inline int page_has_type(struct page *page)
> >>>> +{
> >>>> +	return (PageType(page, 0) &&
> >>>> +	       ((page->page_type & PAGE_TYPE_ALL) != PAGE_TYPE_ALL));
> >>>> +}
> >>>> +
> >>> I think this is a bit complex, and a bit of a pain to update as we add
> >>> new page types.  How about this?
> >>>
> >>> 	return (int)page_type < -128;
> >>>
> >>> (I'm open to appropriate #defines to make this more obvious that it's ~0x7F)
> >> I thought about having this:
> >>
> >> #define PAGE_TYPE_END    0xffffff80
> >>
> >> static int inline page_has_type(struct page *page)
> >> {
> >>     return page->page_type > PAGE_TYPE_BASE &&
> >>            page->page_type < PAGE_TYPE_END;
> >> }
> >>
> >> But I opted for the additional complexity to avoid more false-positives from
> >> possibly corrupted values.  I'm certainly fine with a simple approach, though.
> > The way I'm thinking about this field is that usually it's _mapcount
> > which is 0xffffffff to represent 0.  We allow a certain small amount
> > of underflow and still treat it as a mapcount.  We also allow for some
> > amount of overflow.  So to be utterly precise, what you had there would
> > have been fine, but for simplicity, I'd rather just do a signed compare
> > against -128.
> The signed compare does not allow for mapcount overflow.  Is that acceptable?
> False-positives would be benign for /proc/kpagecount though from a debug
> perspective it could be helpful to see overflowed mapcounts.  Some future
> caller would need separate consideration.

Nobody seems terribly interested in mapcount overflows.  I got no response
to https://lkml.org/lkml/2018/3/2/991

WARNING: multiple messages have this Message-ID (diff)
From: Matthew Wilcox <willy@infradead.org>
To: Anthony Yznaga <anthony.yznaga@oracle.com>
Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	adobriyan@gmail.com, akpm@linux-foundation.org, vbabka@suse.cz,
	sfr@canb.auug.org.au, kirill.shutemov@linux.intel.com,
	rppt@linux.vnet.ibm.com, mhocko@suse.com,
	alexander.h.duyck@linux.intel.com, hannes@cmpxchg.org,
	miles.chen@mediatek.com, n-horiguchi@ah.jp.nec.com
Subject: Re: [PATCH] /proc/kpagecount: return 0 for special pages that are never mapped
Date: Wed, 5 Dec 2018 11:44:09 -0800	[thread overview]
Message-ID: <20181205194409.GB11646@bombadil.infradead.org> (raw)
In-Reply-To: <540f7690-5fcd-04d6-edb3-a44ebd09e70c@oracle.com>

On Wed, Dec 05, 2018 at 11:40:51AM -0800, Anthony Yznaga wrote:
> On 12/04/2018 05:25 PM, Matthew Wilcox wrote:
> > On Tue, Dec 04, 2018 at 05:18:32PM -0800, anthony.yznaga@oracle.com wrote:
> >> On 12/04/2018 04:48 PM, Matthew Wilcox wrote:
> >>> On Tue, Dec 04, 2018 at 02:45:26PM -0800, Anthony Yznaga wrote:
> >>>> +static inline int page_has_type(struct page *page)
> >>>> +{
> >>>> +	return (PageType(page, 0) &&
> >>>> +	       ((page->page_type & PAGE_TYPE_ALL) != PAGE_TYPE_ALL));
> >>>> +}
> >>>> +
> >>> I think this is a bit complex, and a bit of a pain to update as we add
> >>> new page types.  How about this?
> >>>
> >>> 	return (int)page_type < -128;
> >>>
> >>> (I'm open to appropriate #defines to make this more obvious that it's ~0x7F)
> >> I thought about having this:
> >>
> >> #define PAGE_TYPE_END��� 0xffffff80
> >>
> >> static int inline page_has_type(struct page *page)
> >> {
> >> ��� return page->page_type > PAGE_TYPE_BASE &&
> >> ��� ������ page->page_type < PAGE_TYPE_END;
> >> }
> >>
> >> But I opted for the additional complexity to avoid more false-positives from
> >> possibly corrupted values.� I'm certainly fine with a simple approach, though.
> > The way I'm thinking about this field is that usually it's _mapcount
> > which is 0xffffffff to represent 0.  We allow a certain small amount
> > of underflow and still treat it as a mapcount.  We also allow for some
> > amount of overflow.  So to be utterly precise, what you had there would
> > have been fine, but for simplicity, I'd rather just do a signed compare
> > against -128.
> The signed compare does not allow for mapcount overflow.� Is that acceptable?
> False-positives would be benign for /proc/kpagecount though from a debug
> perspective it could be helpful to see overflowed mapcounts.� Some future
> caller would need separate consideration.

Nobody seems terribly interested in mapcount overflows.  I got no response
to https://lkml.org/lkml/2018/3/2/991

  reply	other threads:[~2018-12-05 19:44 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-12-04 22:45 [PATCH] /proc/kpagecount: return 0 for special pages that are never mapped Anthony Yznaga
2018-12-05  0:48 ` Matthew Wilcox
2018-12-05  1:18   ` anthony.yznaga
2018-12-05  1:25     ` Matthew Wilcox
2018-12-05  1:25       ` Matthew Wilcox
2018-12-05 19:40       ` Anthony Yznaga
2018-12-05 19:44         ` Matthew Wilcox [this message]
2018-12-05 19:44           ` Matthew Wilcox
2018-12-06  0:44           ` Anthony Yznaga
2018-12-06  4:26             ` Matthew Wilcox
2018-12-06  4:26               ` Matthew Wilcox
2018-12-06  6:07               ` Anthony Yznaga
2018-12-08  0:27                 ` Andrew Morton
2018-12-08  0:27                   ` Andrew Morton
2018-12-05  8:28   ` David Hildenbrand

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181205194409.GB11646@bombadil.infradead.org \
    --to=willy@infradead.org \
    --cc=adobriyan@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=alexander.h.duyck@linux.intel.com \
    --cc=anthony.yznaga@oracle.com \
    --cc=hannes@cmpxchg.org \
    --cc=kirill.shutemov@linux.intel.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhocko@suse.com \
    --cc=miles.chen@mediatek.com \
    --cc=n-horiguchi@ah.jp.nec.com \
    --cc=rppt@linux.vnet.ibm.com \
    --cc=sfr@canb.auug.org.au \
    --cc=vbabka@suse.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.