From: Toshi Kani <toshi.kani@hp.com>
To: Christoph Hellwig <hch@lst.de>
Cc: Boaz Harrosh <boaz@plexistor.com>,
mingo@redhat.com, hpa@zytor.com, tglx@linutronix.de,
linux-nvdimm@ml01.01.org, linux-fsdevel@vger.kernel.org,
x86@kernel.org, linux-kernel@vger.kernel.org,
Yinghai Lu <yinghai@kernel.org>
Subject: Re: [PATCH v2] x86: Revert E820_PRAM change in e820_end_pfn()
Date: Tue, 07 Apr 2015 07:51:09 -0600 [thread overview]
Message-ID: <1428414669.31093.153.camel@misato.fc.hp.com> (raw)
In-Reply-To: <20150407070453.GB7074@lst.de>
On Tue, 2015-04-07 at 09:04 +0200, Christoph Hellwig wrote:
> On Tue, Apr 07, 2015 at 09:36:37AM +0300, Boaz Harrosh wrote:
> > On 04/06/2015 10:00 PM, Toshi Kani wrote:
> > > 'Commit ec776ef6bbe17 ("x86/mm: Add support for the non-standard
> > > protected e820 type")' added E820_PRAM ranges, which do not have
> > > have struct-page. Therefore, there is no need to update max_pfn
> > > to cover the E820_PRAM ranges.
> >
> > But E820_PRAM ranges will have the possibility for struct-page.
> >
> > That said I have tested with this patch + struct-page and
>
> I'd love to resurrect the old "real page backed" pmem support from
> the old Intel patches eventually, but with all the arguments on
> how we should do I/O on pmem I'd like to keep that a ѕeparate
> discussion. And leaving only fragments of some support in is a bad
> idea,
Agreed -- it should be a separate discussion and we need to get it
straight for 4.1.
> so sorry for letting all this slip through..
No problem.
> > > -static unsigned long __init e820_end_pfn(unsigned long limit_pfn)
> > > +static unsigned long __init e820_end_pfn(unsigned long limit_pfn, unsigned type)
> >
> > Why don't you rename it to say e820_max_ram_pfn or something with ram
> > as you noted, and drop the @type. As Christoph said it is very ugly. You do not
> > put an extra parameter because of a bad name?
> >
> > Anyway you are changing all call sites so it will not even be a bigger
> > change
>
> It's a static function, and we have much worse naming sins in public
> ones, so I'm not worried about a _ram more or less. But if people feel
> stronly about it I'm fine with adding the _ram.
>
> I feel pretty stronly against adding back a pointless argument, though.
We should keep this patch as a revert/fix, and should not combine with
other cleanup. Adding the _ram, etc. can be done as a separate change.
Thanks,
-Toshi
prev parent reply other threads:[~2015-04-07 14:09 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-06 19:00 [PATCH v2] x86: Revert E820_PRAM change in e820_end_pfn() Toshi Kani
2015-04-07 6:36 ` Boaz Harrosh
2015-04-07 7:04 ` Christoph Hellwig
2015-04-07 13:51 ` Toshi Kani [this message]
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=1428414669.31093.153.camel@misato.fc.hp.com \
--to=toshi.kani@hp.com \
--cc=boaz@plexistor.com \
--cc=hch@lst.de \
--cc=hpa@zytor.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nvdimm@ml01.01.org \
--cc=mingo@redhat.com \
--cc=tglx@linutronix.de \
--cc=x86@kernel.org \
--cc=yinghai@kernel.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).