Re: [PATCH 2/2] powerpc/nvdimm: Update vmemmap_populated to check sub-section range
On Tue, Sep 10, 2019 at 4:29 PM Aneesh Kumar K.V
wrote:
>
> With commit: 7cc7867fb061 ("mm/devm_memremap_pages: enable sub-section remap")
> pmem namespaces are remapped in 2M chunks. On architectures like ppc64 we
> can map the memmap area using 16MB hugepage size and that can cover
> a memory range of 16G.
>
> While enabling new pmem namespaces, since memory is added in sub-section
> chunks,
> before creating a new memmap mapping, kernel should check whether there is an
> existing memmap mapping covering the new pmem namespace. Currently, this is
> validated by checking whether the section covering the range is already
> initialized or not. Considering there can be multiple namespaces in the same
> section this can result in wrong validation. Update this to check for
> sub-sections in the range. This is done by checking for all pfns in the range
> we
> are mapping.
>
> We could optimize this by checking only just one pfn in each sub-section. But
> since this is not fast-path we keep this simple.
>
> Signed-off-by: Aneesh Kumar K.V
> ---
> arch/powerpc/mm/init_64.c | 45 ---
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
> index 4e08246acd79..7710ccdc19a2 100644
> --- a/arch/powerpc/mm/init_64.c
> +++ b/arch/powerpc/mm/init_64.c
> @@ -70,30 +70,24 @@ EXPORT_SYMBOL_GPL(kernstart_addr);
>
> #ifdef CONFIG_SPARSEMEM_VMEMMAP
> /*
> - * Given an address within the vmemmap, determine the pfn of the page that
> - * represents the start of the section it is within. Note that we have to
> - * do this by hand as the proffered address may not be correctly aligned.
> - * Subtraction of non-aligned pointers produces undefined results.
> - */
> -static unsigned long __meminit vmemmap_section_start(unsigned long page)
> -{
> - unsigned long offset = page - ((unsigned long)(vmemmap));
> -
> - /* Return the pfn of the start of the section. */
> - return (offset / sizeof(struct page)) & PAGE_SECTION_MASK;
> -}
If you want to go with Dan's suggestion of keeping the function and
using PAGE_SUBSECTION_MASK then can you fold the pfn_to_page() below
into vmemmap_section_start()? The current behaviour of returning a pfn
makes no sense to me.
> - * Check if this vmemmap page is already initialised. If any section
> + * Check if this vmemmap page is already initialised. If any sub section
> * which overlaps this vmemmap page is initialised then this page is
> * initialised already.
> */
> -static int __meminit vmemmap_populated(unsigned long start, int page_size)
> +
> +static int __meminit vmemmap_populated(unsigned long start, int size)
> {
> - unsigned long end = start + page_size;
> - start = (unsigned long)(pfn_to_page(vmemmap_section_start(start)));
> + unsigned long end = start + size;
> - for (; start < end; start += (PAGES_PER_SECTION * sizeof(struct
> page)))
> + /* start is size aligned and it is always > sizeof(struct page) */
> + VM_BUG_ON(start & sizeof(struct page));
Shouldn't the test be: start & (sizeof(struct page) - 1)?
VM_BUG_ON(start != ALIGN(start, page_size)) would be clearer.
> + for (; start < end; start += sizeof(struct page))
> + /*
> +* pfn valid check here is intended to really check
> +* whether we have any subsection already initialized
> +* in this range. We keep it simple by checking every
> +* pfn in the range.
> +*/
> if (pfn_valid(page_to_pfn((struct page *)start)))
> return 1;
Having a few lines of separation between the for () and the loop body
always looks a bit sketch, even if it's just a comment. Wrapping the
block in braces or moving the comment above the loop is probably a
good idea.
Looks fine otherwise
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
找邮箱数据
缺邮箱？缺客户？缺人脉？ 帮你搜索全国各行各业的邮箱数据， 软件全自动化，批量输入关键词搜索 免费测试，下载安装即用 百度云盘下载链接： https://pan.baidu.com/s/1exGR_wwjnmYPOfGnFH23tA 提取码: gqju 详情加我QQ:2124008496 ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
做 采 购必须了解的14大供应市场信息
18146185146 转发邮件信息 发 件 人：gx...@qby.com 发 送 日 期：2019-9-1710:42:09 收 件 人：linux-nvdimm@lists.01.org ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 1/2] libnvdimm/altmap: Track namespace boundaries in altmap
On Mon, Sep 9, 2019 at 11:29 PM Aneesh Kumar K.V wrote: > > With PFN_MODE_PMEM namespace, the memmap area is allocated from the device > area. Some architectures map the memmap area with large page size. On > architectures like ppc64, 16MB page for memap mapping can map 262144 pfns. > This maps a namespace size of 16G. > > When populating memmap region with 16MB page from the device area, > make sure the allocated space is not used to map resources outside this > namespace. Such usage of device area will prevent a namespace destroy. > > Add resource end pnf in altmap and use that to check if the memmap area > allocation can map pfn outside the namespace. On ppc64 in such case we > fallback > to allocation from memory. > > This fix kernel crash reported below: > > [ 132.034989] WARNING: CPU: 13 PID: 13719 at mm/memremap.c:133 > devm_memremap_pages_release+0x2d8/0x2e0 > [ 133.464754] BUG: Unable to handle kernel data access at 0xc00c00010b204000 > [ 133.464760] Faulting instruction address: 0xc007580c > [ 133.464766] Oops: Kernel access of bad area, sig: 11 [#1] > [ 133.464771] LE PAGE_SIZE=64K MMU=Hash SMP NR_CPUS=2048 NUMA pSeries > . > [ 133.464901] NIP [c007580c] vmemmap_free+0x2ac/0x3d0 > [ 133.464906] LR [c00757f8] vmemmap_free+0x298/0x3d0 > [ 133.464910] Call Trace: > [ 133.464914] [c07cbfd0f7b0] [c00757f8] vmemmap_free+0x298/0x3d0 > (unreliable) > [ 133.464921] [c07cbfd0f8d0] [c0370a44] > section_deactivate+0x1a4/0x240 > [ 133.464928] [c07cbfd0f980] [c0386270] > __remove_pages+0x3a0/0x590 > [ 133.464935] [c07cbfd0fa50] [c0074158] > arch_remove_memory+0x88/0x160 > [ 133.464942] [c07cbfd0fae0] [c03be8c0] > devm_memremap_pages_release+0x150/0x2e0 > [ 133.464949] [c07cbfd0fb70] [c0738ea0] > devm_action_release+0x30/0x50 > [ 133.464955] [c07cbfd0fb90] [c073a5a4] release_nodes+0x344/0x400 > [ 133.464961] [c07cbfd0fc40] [c073378c] > device_release_driver_internal+0x15c/0x250 > [ 133.464968] [c07cbfd0fc80] [c072fd14] unbind_store+0x104/0x110 > [ 133.464973] [c07cbfd0fcd0] [c072ee24] drv_attr_store+0x44/0x70 > [ 133.464981] [c07cbfd0fcf0] [c04a32bc] sysfs_kf_write+0x6c/0xa0 > [ 133.464987] [c07cbfd0fd10] [c04a1dfc] > kernfs_fop_write+0x17c/0x250 > [ 133.464993] [c07cbfd0fd60] [c03c348c] __vfs_write+0x3c/0x70 > [ 133.464999] [c07cbfd0fd80] [c03c75d0] vfs_write+0xd0/0x250 Question, does this crash only happen when the namespace is not 16MB aligned? In other words was this bug exposed by the subsection-hotplug changes and should it contain Fixes: tag for those commits? ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [PATCH 2/2] powerpc/nvdimm: Update vmemmap_populated to check sub-section range
On Mon, Sep 9, 2019 at 11:29 PM Aneesh Kumar K.V
wrote:
>
> With commit: 7cc7867fb061 ("mm/devm_memremap_pages: enable sub-section remap")
> pmem namespaces are remapped in 2M chunks. On architectures like ppc64 we
> can map the memmap area using 16MB hugepage size and that can cover
> a memory range of 16G.
>
> While enabling new pmem namespaces, since memory is added in sub-section
> chunks,
> before creating a new memmap mapping, kernel should check whether there is an
> existing memmap mapping covering the new pmem namespace. Currently, this is
> validated by checking whether the section covering the range is already
> initialized or not. Considering there can be multiple namespaces in the same
> section this can result in wrong validation. Update this to check for
> sub-sections in the range. This is done by checking for all pfns in the range
> we
> are mapping.
>
> We could optimize this by checking only just one pfn in each sub-section. But
> since this is not fast-path we keep this simple.
>
> Signed-off-by: Aneesh Kumar K.V
> ---
> arch/powerpc/mm/init_64.c | 45 ---
> 1 file changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
> index 4e08246acd79..7710ccdc19a2 100644
> --- a/arch/powerpc/mm/init_64.c
> +++ b/arch/powerpc/mm/init_64.c
> @@ -70,30 +70,24 @@ EXPORT_SYMBOL_GPL(kernstart_addr);
>
> #ifdef CONFIG_SPARSEMEM_VMEMMAP
> /*
> - * Given an address within the vmemmap, determine the pfn of the page that
> - * represents the start of the section it is within. Note that we have to
> - * do this by hand as the proffered address may not be correctly aligned.
> - * Subtraction of non-aligned pointers produces undefined results.
> - */
> -static unsigned long __meminit vmemmap_section_start(unsigned long page)
> -{
> - unsigned long offset = page - ((unsigned long)(vmemmap));
> -
> - /* Return the pfn of the start of the section. */
> - return (offset / sizeof(struct page)) & PAGE_SECTION_MASK;
> -}
> -
> -/*
> - * Check if this vmemmap page is already initialised. If any section
> + * Check if this vmemmap page is already initialised. If any sub section
> * which overlaps this vmemmap page is initialised then this page is
> * initialised already.
> */
> -static int __meminit vmemmap_populated(unsigned long start, int page_size)
> +
> +static int __meminit vmemmap_populated(unsigned long start, int size)
> {
> - unsigned long end = start + page_size;
> - start = (unsigned long)(pfn_to_page(vmemmap_section_start(start)));
> + unsigned long end = start + size;
>
> - for (; start < end; start += (PAGES_PER_SECTION * sizeof(struct
> page)))
> + /* start is size aligned and it is always > sizeof(struct page) */
> + VM_BUG_ON(start & sizeof(struct page));
If start is size aligned why not include that assumption in the VM_BUG_ON()?
Otherwise it seems this patch could be reduced simply by:
s/PAGE_SECTION_MASK/PAGE_SUBSECTION_MASK/
s/PAGES_PER_SECTION/PAGES_PER_SUBSECTION/
...and leave the vmemmap_section_start() function in place? In other
words this path used to guarantee that 'start' was aligned to the
minimum mem-hotplug granularity, the change looks ok on the surface,
but it seems a subtle change in semantics.
Can you get an ack from a powerpc maintainer, or maybe this patch
should route through the powerpc tree?
I'll take patch1 through the nvdimm tree.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile
Dan, >> One pet peeve I have is that people are pretty bad at indicating the >> intended target tree. I often ask for it in private mail but the >> practice doesn't seem to stick. I spend a ton of time guessing whether a >> patch is a fix for a new feature in the x+1 queue or a fix for the >> current release. It is not always obvious. > > The Fixes tag doesn't help? Always. > Of course, you've never asked me or anyone on kernel-newbies that > question. We don't normally know the answer either. I do always try > to figure it out for networking, but it's kind of a pain in the butt > and I mess up surpisingly often for how much effort I put into getting > it right. It's not a big issue for your patches. These are inevitably fixes and I'll pick an appropriate tree depending on where we are in the cycle, how likely one is to hit the issue, whether the driver is widely used, etc. Vendor driver submissions, however, are generally huge. Sometimes 50+ patches per submission window. And during this window I often get on the order of 10 to 20 patches for the same driver in the fixes tree. It is not always easy to determine whether a bug fix series is for one tree or the other. -- Martin K. Petersen Oracle Linux Engineering ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile
On Fri, 13 Sep 2019, Dan Carpenter wrote: > And the documentation doesn't help. For example, I knew people's rules > about capitalizing the subject but I'd just forget. I say that if you > can't be bothered to add it to checkpatch then it means you don't really > care that strongly. It would be entirely possible to add the subsystem/driver specific checkpatch rules to MAINTAINERS entries or directory specific config files. As checkpatch options directly. For example --strict --max-line-length=100 --ignore=BIT_MACRO,SPLIT_STRING,LONG_LINE_STRING,BOOL_MEMBER or whatever. SMOP. ;) BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Ksummit-discuss] [PATCH v2 2/3] Maintainer Handbook: Maintainer Entry Profile
On Wed, 11 Sep 2019, Dan Williams wrote: > As presented at the 2018 Linux Plumbers conference [1], the Maintainer > Entry Profile (formerly Subsystem Profile) is proposed as a way to reduce > friction between committers and maintainers and encourage conversations > amongst maintainers about common best practices. While coding-style, > submit-checklist, and submitting-drivers lay out some common expectations > there remain local customs and maintainer preferences that vary by > subsystem. > > The profile contains short answers to some of the common policy questions a > contributor might have that are local to the subsystem / device-driver, or > otherwise not covered by the top-level process documents. > > Overview: General introduction to how the subsystem operates > Submit Checklist Addendum: Mechanical items that gate submission staging > Key Cycle Dates: > - Last -rc for new feature submissions: Expected lead time for submissions > - Last -rc to merge features: Deadline for merge decisions > Coding Style Addendum: Clarifications of local style preferences > Resubmit Cadence: When to ping the maintainer > Checkpatch / Style Cleanups: Policy on pure cleanup patches > > See Documentation/maintainer/maintainer-entry-profile.rst for more details, > and a follow-on example profile for the libnvdimm subsystem. > > [1]: https://linuxplumbersconf.org/event/2/contributions/59/ > > Cc: Jonathan Corbet > Cc: Thomas Gleixner > Cc: Mauro Carvalho Chehab > Cc: Steve French > Cc: Greg Kroah-Hartman > Cc: Linus Torvalds > Cc: Tobin C. Harding > Cc: Olof Johansson > Cc: Martin K. Petersen > Cc: Daniel Vetter > Cc: Joe Perches > Cc: Dmitry Vyukov > Cc: Alexandre Belloni > Signed-off-by: Dan Williams > --- > Documentation/maintainer/index.rst |1 > .../maintainer/maintainer-entry-profile.rst| 99 > > MAINTAINERS|4 + > 3 files changed, 104 insertions(+) > create mode 100644 Documentation/maintainer/maintainer-entry-profile.rst > > diff --git a/Documentation/maintainer/index.rst > b/Documentation/maintainer/index.rst > index 56e2c09dfa39..d904e74e1159 100644 > --- a/Documentation/maintainer/index.rst > +++ b/Documentation/maintainer/index.rst > @@ -12,4 +12,5 @@ additions to this manual. > configure-git > rebasing-and-merging > pull-requests > + maintainer-entry-profile > > diff --git a/Documentation/maintainer/maintainer-entry-profile.rst > b/Documentation/maintainer/maintainer-entry-profile.rst > new file mode 100644 > index ..aaedf4d6dbf7 > --- /dev/null > +++ b/Documentation/maintainer/maintainer-entry-profile.rst > @@ -0,0 +1,99 @@ > +.. _maintainerentryprofile: > + > +Maintainer Entry Profile > + > + > +The Maintainer Entry Profile supplements the top-level process documents > +(coding-style, submitting-patches...) with subsystem/device-driver-local > +customs as well as details about the patch submission lifecycle. A > +contributor uses this document to level set their expectations and avoid > +common mistakes, maintainers may use these profiles to look across > +subsystems for opportunities to converge on common practices. In general I think we're already pretty good at adding and accumulating documentation, but not so much cleaning up existing and outright throwing out obsolete documentation. 'wc -l Documentation/process/*' says we already have 11k lines of generic process documentation. I would much rather see a streamlined and friendly guide to contributing to the kernel than more rules, and driver/subsystem specific rules at that. I would much rather get the feeling from submissions that the long tail of contributors have actually read and understood the most relevant parts of Documentation/process. I'm pretty sure *I* haven't read all of Documentation/process. My fear is that the entry profiles will only be helpful to the contributors that already "get it". It may be helpful to the maintainers in the sense that they can reply to patches with a pointer to the relevant hoops to jump through. --- Even so, if this gets merged, I'll probably add something helpful for drm and/or i915 contributors. But what's the buy-in across the kernel? If my grep serves me right, there are something like 2000+ entries in MAINTAINERS. If 10% of them adds a profile, that's a fairly serious amount of documentation. But can you actually expect more than a handful of subsystems/drivers to add a profile? Then what's the coverage? For reference, I've added or promoted adding the B: and C: entries to MAINTAINERS. Various subsystems and drivers are really picky about where and how they want bugs reported; in particular some folks only accept email. Yet networking is the only one to indicate the email preference in MAINTAINERS. And adding that is a one line change. (There are 19 B: entries and 7 C: entries in total.) BR, Jani. > + > + > +Overview > + > +Pr
Re: [PATCH 01/13] nvdimm: Use more typical whitespace
On Thu, Sep 12, 2019 at 08:01:45AM -0700, Joe Perches wrote:
> On Thu, 2019-09-12 at 05:17 -0700, Christoph Hellwig wrote:
> > Instead of arguing what is better just stick to what the surrounding
> > code does.
>
> That's not always feasible nor readable.
>
> Especially for the logic inversion blocks where
> the existing code does unreadable and error prone
> things like hiding semicolons immediately after
> comments.
>
> if (foo)
> /* longish comment */;
> else {
> ;
> }
Which has nothing to do with your patch.
> > Or in other words: Feel free to be a codingstyle nazi for your code
> > (I am for some of mine), but leave others peoples code alone with
> > "cleanup" patches.
>
> My point was to avoid documenting per-subsystem
> coding style rules.
It is called common sense. In many cases different parts of the
subsystem might have slight variations. Just stick to your
preferred style in the bounds of coding style. Maintainers will
either remind you if they feel strongly that they have a slightly
different preference or just fix it up. What we really don't need
need it whitespace cleanup patches in the micro variation area.
___
Linux-nvdimm mailing list
Linux-nvdimm@lists.01.org
https://lists.01.org/mailman/listinfo/linux-nvdimm
Re: [Ksummit-discuss] [PATCH v2 3/3] libnvdimm, MAINTAINERS: Maintainer Entry Profile
On Fri, Sep 13, 2019 at 05:44:39PM -0400, Martin K. Petersen wrote: > One pet peeve I have is that people are pretty bad at indicating the > intended target tree. I often ask for it in private mail but the > practice doesn't seem to stick. I spend a ton of time guessing whether a > patch is a fix for a new feature in the x+1 queue or a fix for the > current release. It is not always obvious. The Fixes tag doesn't help? Of course, you've never asked me or anyone on kernel-newbies that question. We don't normally know the answer either. I do always try to figure it out for networking, but it's kind of a pain in the butt and I mess up surpisingly often for how much effort I put into getting it right. regards, dan carpenter ___ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm
