* Re: Review request: draft ioctl_userfaultfd(2) manual page
From: Andrea Arcangeli @ 2017-05-03 21:46 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Mike Rapoport, lkml, linux-mm@kvack.org, linux-man
In-Reply-To: <e8c5ca4a-0710-7206-b96e-10d171bda218@gmail.com>
On Fri, Apr 21, 2017 at 11:11:18AM +0200, Michael Kerrisk (man-pages) wrote:
> Hello Mike,
> Hello Andrea (we need your help!),
Sorry for not answering sooner! (I had a vacation last week)
>
> On 03/22/2017 02:54 PM, Mike Rapoport wrote:
> >> The various ioctl(2) operations are described below. The UFFDIO_API,
> >> UFFDIO_REGISTER, and UFFDIO_UNREGISTER operations are used to configure
> >> userfaultfd behavior. These operations allow the caller to choose what
> >> features will be enabled and what kinds of events will be delivered to
> >> the application. The remaining operations are range operations. These
> >> operations enable the calling application to resolve page-fault events
> >> in a consistent way.
> >>
> >>
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??FIXME a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??Above: What does "consistent" mean? a??
> >> a?? a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >
> > Andrea, can you please help with this one?
>
> Let's see what Andrea has to say.
I think it doesn't mean anything and I see you already removed it, fine!
> So, the thing that was not clear, but now I think I understand:
> 'features' is an input field where one can ask about supported features
> (but none are supported, before Linux 4.11). Is that correct? I've changed
> the text here to read:
>
> Before the call, the features field must be initialized
> to zero. In the future, it is intended that this field can be
> used to ask whether particular features are supported.
>
> Seem okay?
Yes, but in reality nothing has changed. Simply the kernels before
4.11 had no feature support at all.
===
Starting from Linux 4.11, the features field can be used to ask
whether particular features are supported and explicitly enable
userfaultfd features that are disabled by default. The kernel
always reports all the available features in the features
field.
=====
I would prefer if we removed this 4.11 difference here.
We should be able to describe it simply as:
"The features field can be used to ask whether particular features are
supported and explicitly enable userfaultfd features that are disabled
by default. The kernel always reports all the available features in
the features field."
The whole point of this feature flag thing, is so the app at runtime
can check if the feature is available and ask for it. The fact kernels
before 4.11 don't support any feature is a detail.
> > There's a check in uffdio_api call that the user is not trying to enable
> > any other functionality and it asserts that uffdio_api.featurs is zero [1].
> > Starting from 4.11 the features negotiation is different. Now uffdio_call
> > verifies that it can support features the application requested [2].
>
> Okay.
I don't like the differentiation here between 4.11 and before, because
from user point of view nothing has changed.
I think this description is enough " Since Linux 4.11, the
following feature bits may be set: " and no other mention of 4.11 is
needed in the manpage. It looks an unnecessary complication to the reader.
>
> >> The kernel verifies that it can support the requested API version, and
> >> sets the features and ioctls fields to bit masks representing all the
> >> available features and the generic ioctl(2) operations available. Cura??
> >> rently, zero (i.e., no feature bits) is placed in the features field.
> >> The returned ioctls field can contain the following bits:
> >>
> >>
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??FIXME a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??This user-space API seems not fully polished. Why a??
> >> a??are there not constants defined for each of the bit- a??
> >> a??mask values listed below? a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >>
> >> 1 << _UFFDIO_API
> >> The UFFDIO_API operation is supported.
> >>
> >> 1 << _UFFDIO_REGISTER
> >> The UFFDIO_REGISTER operation is supported.
> >>
> >> 1 << _UFFDIO_UNREGISTER
> >> The UFFDIO_UNREGISTER operation is supported.
> >
> > Well, I tend to agree. I believe the original intention was to use the
> > OR'ed mask, like UFFD_API_IOCTLS.
> > Andrea, can you add somthing?
>
> Yes, Andrea, please!
I agree it can be polished, but that's not something the manpage can
fix... For now the above is correct.
So about the error retvals I reviewed the final manpage from git which
is the latest version.
> >>
> >> EINVAL The userfaultfd has already been enabled by a previous UFFa??
> >> DIO_API operation.
> >>
> >> EINVAL The API version requested in the api field is not supported by
> >> this kernel, or the features field was not zero.
> >>
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??FIXME a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??In the above error case, the returned 'uffdio_api' a??
> >> a??structure zeroed out. Why is this done? This should a??
> >> a??be explained in the manual page. a??
> >> a?? a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >
> > In my understanding the uffdio_api structure is zeroed to allow the caller
> > to distinguish the reasons for -EINVAL.
>
> Andrea, can you please help here?
It is zeroed out just for robustness, it's a slow path. If userland by
mistake won't check -EINVAL but it checks uffdio_api.features to be
set or uffdio_api.ioctls or uffdio_api.api after the UFFDIO_API ioctl
returns, it will have a chance to catch the failure (it won't risk to
parse random uninitialized values at least).
I don't think it should be documented the uffdio_api is zeroed out or
if it is documented, we should say userland shouldn't depend on it and
it's done just for robustness.
The normal correct way to catch an error is to check -EINVAL, after
getting -EINVAL the contents of uffdio_api should be ignored by
userland.
> >> UFFDIO_REGISTER
> >> (Since Linux 4.3.) Register a memory address range with the usera??
> >> faultfd object. The argp argument is a pointer to a uffdio_register
> >> structure, defined as:
> >>
> >> struct uffdio_range {
> >> __u64 start; /* Start of range */
> >> __u64 len; /* Length of rnage (bytes) */
> >> };
> >>
> >> struct uffdio_register {
> >> struct uffdio_range range;
> >> __u64 mode; /* Desired mode of operation (input) */
> >> __u64 ioctls; /* Available ioctl() operations (output) */
> >> };
> >>
> >>
> >> The range field defines a memory range starting at start and continuing
> >> for len bytes that should be handled by the userfaultfd.
> >>
> >> The mode field defines the mode of operation desired for this memory
> >> region. The following values may be bitwise ORed to set the usera??
> >> faultfd mode for the specified range:
> >>
> >> UFFDIO_REGISTER_MODE_MISSING
> >> Track page faults on missing pages.
> >>
> >> UFFDIO_REGISTER_MODE_WP
> >> Track page faults on write-protected pages.
> >>
> >> Currently, the only supported mode is UFFDIO_REGISTER_MODE_MISSING.
> >>
> >> If the operation is successful, the kernel modifies the ioctls bit-mask
> >> field to indicate which ioctl(2) operations are available for the speca??
> >> ified range. This returned bit mask is as for UFFDIO_API.
> >>
> >> This ioctl(2) operation returns 0 on success. On error, -1 is returned
> >> and errno is set to indicate the cause of the error. Possible errors
> >> include:
> >>
> >>
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??FIXME a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??Is the following error list correct? a??
> >> a?? a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >
> > Here again it maybe -EFAULT to indicate copy_{from,to}_user failure.
> > And, UFFDIO_REGISTER may return -ENOMEM if the process is exiting and the
> > mm_struct has gone by the time userfault grabs it.
>
> Okay -- added EFAULT. I think I'll skip ENOMEM for the moment, but
> will note the possibility in the page source.
There is also ENOMEM as result of split_vma failing, and it isn't the
cleanest thing that it means both real OOM or out of vmas
(mm->map_count >= sysctl_max_map_count, not real OOM) and that the
process is exiting or there isn't a single vma in the mm.
If there's one vma but it isn't in the range we return -EINVAL so we
could return probably -EINVAL if it's exiting or if there isn't a
single vma in the mm. And leave -ENOMEM for split_vma only.
In general -EINVAL is programmer error of some kind, -ENOMEM is
returned in memory related cases that trigger at runtime, however if
the process is exiting it's debatable if it's programmer error too
which is why I think we could return -EINVAL there.
I'd expect userland to threat -ENOMEM and -EINVAL about the same way.
> >> EINVAL There was no mapping in the specified address range.
> >>
> >> UFFDIO_COPY
> >> (Since Linux 4.3.) Atomically copy a continuous memory chunk into the
> >> userfault registered range and optionally wake up the blocked thread.
> >> The source and destination addresses and the number of bytes to copy
> >> are specified by the src, dst, and len fields of the uffdio_copy struca??
> >> ture pointed to by argp:
> >>
> >> struct uffdio_copy {
> >> __u64 dst; /* Source of copy */
> >> __u64 src; /* Destinate of copy */
> >> __u64 len; /* Number of bytes to copy */
> >> __u64 mode; /* Flags controlling behavior of copy */
> >> __s64 copy; /* Number of bytes copied, or negated error */
> >> };
> >>
> >> The following value may be bitwise ORed in mode to change the behavior
> >> of the UFFDIO_COPY operation:
> >>
> >> UFFDIO_COPY_MODE_DONTWAKE
> >> Do not wake up the thread that waits for page-fault resolution
> >>
> >> The copy field is used by the kernel to return the number of bytes that
> >> was actually copied, or an error (a negated errno-style value).
> >>
> >>
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??FIXME a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??Above: Why is the 'copy' field used to return error a??
> >> a??values? This should be explained in the manual a??
> >> a??page. a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >
> > Andrea, can you help with this one, please?
>
> Yes, Andrea, please.
Well not just for error values. copy also tells how much did it copy
if a signal made it return short with -EINTR or some other error
happened in the middle of the copy after we already did some
copying-progress.
Writing any other error into .copy (and not only writing positive
values there) is for robustness in case userland won't check the ioctl
retval but nobody should depend on it. After the ioctl returns an
error userland should not check the uffdio_copy structure at all.
The thing to document is the amount of bytes successfully copied in
uffdio_copy.copy (which may be a short copy and in turn must be
checked... unless one knows the len matches the arch PAGE_SIZE but
it's still safer to check the uffdio_copy.copy field and be
consistent).
One more relevant retvals for UFFDIO_COPY and UFFDIO_ZEROPAGE that I
noticed is missing in the current manpage, is
-EEXIST. UFFDIO_COPY/ZEROPAGE don't behave like mmap/mremap,
UFFDIO_COPY/ZEROPAGE will never teardown any existing established
mapping to guarantee even if the user has race condition in its code,
there's no risk of silent memory corruption, instead a meaningful
error is returned by UFFDIO_COPY/ZEROPAGE. For example if two
UFFDIO_COPY run concurrently on the same page, only the first one will
succeed, the second will return -EEXIST and only the first page will
be copied and no memory corruption can happen this way (furthermore
the programmer can be notified of the race condition in the code which
might even be intentional, but more likely is not).
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??FIXME a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
> >> a??Why is the 'zeropage' field used to return error a??
> >> a??values? This should be explained in the manual a??
> >> a??page. a??
> >> a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??a??
>
> Help is still needed for this FIXME!
Same as uffdio_copy.copy: because we've to return the number of pages
that have been zeroed out in case we run into an error (signal -EINTR
or -EEXIST etc..) after we already succeeded partially on a couple of
pages. So we may as well write the error in the same field if no pages
were zeroed out. This way the program will misbehave more than if that
field is left untouched (and it could even be random in such case as
it can be left uninitialized by userland). Clearly it only makes a
difference if the programmer forgets to check the UFFDIO_ZEROPAGE
ioctl retval and the retval must always be checked, so again, this is
only for robustness.
Awesome manpage! Super useful, it's fundamental to have a manpage
especially when the syscall is not simple and strightforward in
functionality.
Thanks!
Andrea
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH][RFC] mm: make kswapd try harder to keep active pages in cache
From: Rik van Riel @ 2017-05-03 19:20 UTC (permalink / raw)
To: Josef Bacik; +Cc: linux-mm, hannes, kernel-team
In-Reply-To: <20170503183814.GA11572@destiny>
[-- Attachment #1: Type: text/plain, Size: 1954 bytes --]
On Wed, 2017-05-03 at 14:38 -0400, Josef Bacik wrote:
>
>
> > > + if (nr_inactive > total_high_wmark && nr_inactive >
> > > nr_slab)
> > > + skip_slab = true;
> >
> > I worry that this may be a little too aggressive,
> > and result in the slab cache growing much larger
> > than it should be on some systems.
> >
> > I wonder if it may make more sense to have the
> > aggressiveness of slab scanning depend on the
> > ratio of inactive to reclaimable slab pages, rather
> > than having a hard cut-off like this?
> >
>
> So I originally had a thing that kept track of the rate of change of
> inactive vs
> slab between kswapd runs, but this worked fine so I figured simpler
> was better.
> Keep in mind that we only skip slab the first loop through, so if we
> fail to
> free enough on the inactive list the first time through then we start
> evicting
> slab as well. The idea is (and my testing bore this out) that with
> the new size
> ratio way of shrinking slab we would sometimes be over zealous and
> evict slab
> that we were actively using, even though we had reclaimed plenty of
> pages from
> our inactive list to satisfy our sc->nr_to_reclaim.
My worry is that, since we try to keep the active to
inactive ratio about equal for file pages, many systems
could end up with equal amounts of active file pages,
inactive file pages, and reclaimable slab.
That could not be a gigantic waste of memory for many
workloads, but it could also exacerbate the "reclaim
slab objects forever without freeing any memory" problem
once we do need the memory for something else later on.
> I could probably change the ratio in the sc->inactive_only case to be
> based on
> the slab to inactive ratio and see how that turns out, I'll get that
> wired up
> and let you know how it goes. Thanks,
Looking forward to it.
I am glad to see this problem being attacked :)
--
All rights reversed
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [PATCH RFC] hugetlbfs 'noautofill' mount option
From: Prakash Sangappa @ 2017-05-03 19:02 UTC (permalink / raw)
To: Dave Hansen, linux-kernel, linux-mm
In-Reply-To: <22557bf3-14bb-de02-7b1b-a79873c583f1@intel.com>
On 5/2/17 4:43 PM, Dave Hansen wrote:
> On 05/02/2017 04:34 PM, Prakash Sangappa wrote:
>> Similarly, a madvise() option also requires additional system call by every
>> process mapping the file, this is considered a overhead for the database.
> How long-lived are these processes? For a database, I'd assume that
> this would happen a single time, or a single time per mmap() at process
> startup time. Such a syscall would be doing something on the order of
> taking mmap_sem, walking the VMA tree, setting a bit per VMA, and
> unlocking. That's a pretty cheap one-time cost...
Plus a call into the filesystem (a_ops?) to check if the underlying
filesystem
supports not filling holes to mapped access before setting the bit per vma.
Although the overhead may not be that bad.
Database processes can exit and new once started, for instance, depending on
database activity.
>> If we do consider a new madvise() option, will it be acceptable
>> since this will be specifically for hugetlbfs file mappings?
> Ideally, it would be something that is *not* specifically for hugetlbfs.
> MADV_NOAUTOFILL, for instance, could be defined to SIGSEGV whenever
> memory is touched that was not populated with MADV_WILLNEED, mlock(), etc...
If this is a generic advice type, necessary support will have to be
implemented
in various filesystems which can support this.
The proposed behavior for 'noautofill' was to not fill holes in
files(like sparse files).
In the page fault path, mm would not know if the mmapped address on which
the fault occurred, is over a hole in the file or just that the page is
not available
in the page cache. The underlying filesystem would be called and it
determines
if it is a hole and that is where it would fail and not fill the hole,
if this support is added.
Normally, filesystem which support sparse files(holes in file)
automatically fill the hole
when accessed. Then there is the issue of file system block size and
page size. If the
block sizes are smaller then page size, it could mean the noautofill
would only work
if the hole size is equal to or a multiple of, page size?
In case of hugetlbfs it is much straight forward. Since this filesystem
is not like a normal
filesystems and and the file sizes are multiple of huge pages. The hole
will be a multiple
of the huge page size. For this reason then should the advise be
specific to hugetlbfs?
>
>> If so,
>> would a new flag to mmap() call itself be acceptable, which would
>> define the proposed behavior?. That way no additional system calls
>> need to be made.
> I don't feel super strongly about it, but I guess an mmap() flag could
> work too.
>
Same goes with the mmap call, if it is a generic flag.
-Prakash.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RESENT PATCH] x86/mem: fix the offset overflow when read/write mem
From: Rik van Riel @ 2017-05-03 18:46 UTC (permalink / raw)
To: David Rientjes, zhongjiang, Bjorn Helgaas, Yoshinori Sato
Cc: Rich Felker, Andrew Morton, arnd, hannes, kirill, mgorman, hughd,
linux-mm, linux-kernel
In-Reply-To: <alpine.DEB.2.10.1705021350510.116499@chino.kir.corp.google.com>
[-- Attachment #1: Type: text/plain, Size: 1243 bytes --]
On Tue, 2017-05-02 at 13:54 -0700, David Rientjes wrote:
> > diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> > index 7e4a9d1..3a765e02 100644
> > --- a/drivers/char/mem.c
> > +++ b/drivers/char/mem.c
> > @@ -55,7 +55,7 @@ static inline int
> valid_phys_addr_range(phys_addr_t addr, size_t count)
> >
> > static inline int valid_mmap_phys_addr_range(unsigned long pfn,
> size_t size)
> > {
> > - return 1;
> > + return (pfn << PAGE_SHIFT) + size <= __pa(high_memory);
> > }
> > #endif
> >
>
> I suppose you are correct that there should be some sanity checking
> on the
> size used for the mmap().
My apologies for not responding earlier. It may
indeed make sense to have a sanity check here.
However, it is not as easy as simply checking the
end against __pa(high_memory). Some systems have
non-contiguous physical memory ranges, with gaps
of invalid addresses in-between.
You would have to make sure that both the beginning
and the end are valid, and that there are no gaps of
invalid pfns in the middle...
At that point, is the complexity so much that it no
longer makes sense to try to protect against root
crashing the system?
--
All rights reversed
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* [patch 1/3] MM: remove unused quiet_vmstat function
From: Marcelo Tosatti @ 2017-05-03 18:40 UTC (permalink / raw)
To: linux-kernel, linux-mm
Cc: Luiz Capitulino, Rik van Riel, Linux RT Users, Marcelo Tosatti
In-Reply-To: <20170503184007.174707977@redhat.com>
[-- Attachment #1: remove-vmstat-quiet --]
[-- Type: text/plain, Size: 2250 bytes --]
Remove unused quiet_vmstat function.
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
---
include/linux/vmstat.h | 1 -
mm/vmstat.c | 25 -------------------------
2 files changed, 26 deletions(-)
Index: linux-2.6-git-disable-vmstat-worker/include/linux/vmstat.h
===================================================================
--- linux-2.6-git-disable-vmstat-worker.orig/include/linux/vmstat.h 2017-04-24 18:52:42.957724687 -0300
+++ linux-2.6-git-disable-vmstat-worker/include/linux/vmstat.h 2017-04-24 18:53:15.086793496 -0300
@@ -233,7 +233,6 @@
extern void __dec_zone_state(struct zone *, enum zone_stat_item);
extern void __dec_node_state(struct pglist_data *, enum node_stat_item);
-void quiet_vmstat(void);
void cpu_vm_stats_fold(int cpu);
void refresh_zone_stat_thresholds(void);
Index: linux-2.6-git-disable-vmstat-worker/mm/vmstat.c
===================================================================
--- linux-2.6-git-disable-vmstat-worker.orig/mm/vmstat.c 2017-04-24 18:52:42.957724687 -0300
+++ linux-2.6-git-disable-vmstat-worker/mm/vmstat.c 2017-04-24 18:53:53.075874785 -0300
@@ -1657,31 +1657,6 @@
}
/*
- * Switch off vmstat processing and then fold all the remaining differentials
- * until the diffs stay at zero. The function is used by NOHZ and can only be
- * invoked when tick processing is not active.
- */
-void quiet_vmstat(void)
-{
- if (system_state != SYSTEM_RUNNING)
- return;
-
- if (!delayed_work_pending(this_cpu_ptr(&vmstat_work)))
- return;
-
- if (!need_update(smp_processor_id()))
- return;
-
- /*
- * Just refresh counters and do not care about the pending delayed
- * vmstat_update. It doesn't fire that often to matter and canceling
- * it would be too expensive from this path.
- * vmstat_shepherd will take care about that for us.
- */
- refresh_cpu_vm_stats(false);
-}
-
-/*
* Shepherd worker thread that checks the
* differentials of processors that have their worker
* threads for vm statistics updates disabled because of
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [patch 3/3] MM: allow per-cpu vmstat_worker configuration
From: Marcelo Tosatti @ 2017-05-03 18:40 UTC (permalink / raw)
To: linux-kernel, linux-mm
Cc: Luiz Capitulino, Rik van Riel, Linux RT Users, Marcelo Tosatti
In-Reply-To: <20170503184007.174707977@redhat.com>
[-- Attachment #1: vmstat-worker-disinterface --]
[-- Type: text/plain, Size: 4008 bytes --]
Following the reasoning on the last patch in the series,
this patch allows configuration of the per-CPU vmstat worker:
it allows the user to disable the per-CPU vmstat worker.
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
--- linux/mm/vmstat.c.sothresh 2017-05-03 11:01:17.465914562 -0300
+++ linux/mm/vmstat.c 2017-05-03 11:01:39.746961917 -0300
@@ -92,6 +92,7 @@
EXPORT_SYMBOL(vm_node_stat);
struct vmstat_uparam {
+ atomic_t vmstat_work_enabled;
atomic_t user_stat_thresh;
};
@@ -1606,6 +1607,9 @@
long val;
int err;
int i;
+ int cpu;
+ struct work_struct __percpu *works;
+ static struct cpumask has_work;
/*
* The regular update, every sysctl_stat_interval, may come later
@@ -1619,9 +1623,31 @@
* transiently negative values, report an error here if any of
* the stats is negative, so we know to go looking for imbalance.
*/
- err = schedule_on_each_cpu(refresh_vm_stats);
- if (err)
- return err;
+
+ works = alloc_percpu(struct work_struct);
+ if (!works)
+ return -ENOMEM;
+
+ cpumask_clear(&has_work);
+ get_online_cpus();
+
+ for_each_online_cpu(cpu) {
+ struct work_struct *work = per_cpu_ptr(works, cpu);
+ struct vmstat_uparam *vup = &per_cpu(vmstat_uparam, cpu);
+
+ if (atomic_read(&vup->vmstat_work_enabled)) {
+ INIT_WORK(work, refresh_vm_stats);
+ schedule_work_on(cpu, work);
+ cpumask_set_cpu(cpu, &has_work);
+ }
+ }
+
+ for_each_cpu(cpu, &has_work)
+ flush_work(per_cpu_ptr(works, cpu));
+
+ put_online_cpus();
+ free_percpu(works);
+
for (i = 0; i < NR_VM_ZONE_STAT_ITEMS; i++) {
val = atomic_long_read(&vm_zone_stat[i]);
if (val < 0) {
@@ -1713,6 +1739,10 @@
/* Check processors whose vmstat worker threads have been disabled */
for_each_online_cpu(cpu) {
struct delayed_work *dw = &per_cpu(vmstat_work, cpu);
+ struct vmstat_uparam *vup = &per_cpu(vmstat_uparam, cpu);
+
+ if (atomic_read(&vup->vmstat_work_enabled) == 0)
+ continue;
if (!delayed_work_pending(dw) && need_update(cpu))
queue_delayed_work_on(cpu, mm_percpu_wq, dw, 0);
@@ -1737,6 +1767,40 @@
#ifdef CONFIG_SYSFS
+static ssize_t vmstat_worker_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ unsigned int cpu = dev->id;
+ struct vmstat_uparam *vup = &per_cpu(vmstat_uparam, cpu);
+
+ return sprintf(buf, "%d\n", atomic_read(&vup->vmstat_work_enabled));
+}
+
+static ssize_t vmstat_worker_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret, val;
+ struct vmstat_uparam *vup;
+ unsigned int cpu = dev->id;
+
+ ret = sscanf(buf, "%d", &val);
+ if (ret != 1 || val > 1 || val < 0)
+ return -EINVAL;
+
+ preempt_disable();
+
+ if (cpu_online(cpu)) {
+ vup = &per_cpu(vmstat_uparam, cpu);
+ atomic_set(&vup->vmstat_work_enabled, val);
+ } else
+ count = -EINVAL;
+
+ preempt_enable();
+
+ return count;
+}
+
static ssize_t vmstat_thresh_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -1779,10 +1843,14 @@
return count;
}
+struct device_attribute vmstat_worker_attr =
+ __ATTR(vmstat_worker, 0644, vmstat_worker_show, vmstat_worker_store);
+
struct device_attribute vmstat_threshold_attr =
__ATTR(vmstat_threshold, 0644, vmstat_thresh_show, vmstat_thresh_store);
static struct attribute *vmstat_attrs[] = {
+ &vmstat_worker_attr.attr,
&vmstat_threshold_attr.attr,
NULL
};
@@ -1820,6 +1888,7 @@
struct vmstat_uparam *vup = &per_cpu(vmstat_uparam, cpu);
atomic_set(&vup->user_stat_thresh, 0);
+ atomic_set(&vup->vmstat_work_enabled, 1);
}
}
@@ -1857,6 +1926,7 @@
node = cpu_to_node(cpu);
atomic_set(&vup->user_stat_thresh, 0);
+ atomic_set(&vup->vmstat_work_enabled, 1);
refresh_zone_stat_thresholds();
node_cpus = cpumask_of_node(node);
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [patch 0/3] per-CPU vmstat thresholds and vmstat worker disablement (v2)
From: Marcelo Tosatti @ 2017-05-03 18:40 UTC (permalink / raw)
To: linux-kernel, linux-mm; +Cc: Luiz Capitulino, Rik van Riel, Linux RT Users
The per-CPU vmstat worker is a problem on -RT workloads (because
ideally the CPU is entirely reserved for the -RT app, without
interference). The worker transfers accumulated per-CPU
vmstat counters to global counters.
To resolve the problem, create two tunables:
* Userspace configurable per-CPU vmstat threshold: by default the
VM code calculates the size of the per-CPU vmstat arrays. This
tunable allows userspace to configure the values.
* Userspace configurable per-CPU vmstat worker: allow disabling
the per-CPU vmstat worker.
v2:
- Improve documentation (Rik/Luiz).
- Split patch in two (Luiz).
- Fix comparison to include equal, in the helpers for
stats accounting.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* [patch 2/3] MM: allow per-cpu vmstat_threshold configuration
From: Marcelo Tosatti @ 2017-05-03 18:40 UTC (permalink / raw)
To: linux-kernel, linux-mm
Cc: Luiz Capitulino, Rik van Riel, Linux RT Users, Marcelo Tosatti
In-Reply-To: <20170503184007.174707977@redhat.com>
[-- Attachment #1: vmstat-configurable-thresh --]
[-- Type: text/plain, Size: 11062 bytes --]
The per-CPU vmstat worker is a problem on -RT workloads (because
ideally the CPU is entirely reserved for the -RT app, without
interference). The worker transfers accumulated per-CPU
vmstat counters to global counters.
To resolve the problem, create a userspace configurable per-CPU
vmstat threshold: by default the VM code calculates the size of
the per-CPU vmstat arrays. This tunable allows userspace to
configure the vmstat threshold values.
The patch below contains documentation which describes the tunables
in more detail.
Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
---
Documentation/vm/vmstat_thresholds.txt | 78 +++++++++++++
mm/vmstat.c | 188 +++++++++++++++++++++++++++++----
2 files changed, 247 insertions(+), 19 deletions(-)
Index: linux-2.6-git-disable-vmstat-worker/mm/vmstat.c
===================================================================
--- linux-2.6-git-disable-vmstat-worker.orig/mm/vmstat.c 2017-04-25 07:39:13.941019853 -0300
+++ linux-2.6-git-disable-vmstat-worker/mm/vmstat.c 2017-05-03 10:59:43.495714336 -0300
@@ -91,8 +91,16 @@
EXPORT_SYMBOL(vm_zone_stat);
EXPORT_SYMBOL(vm_node_stat);
+struct vmstat_uparam {
+ atomic_t user_stat_thresh;
+};
+
+static DEFINE_PER_CPU(struct vmstat_uparam, vmstat_uparam);
+
#ifdef CONFIG_SMP
+#define MAX_THRESHOLD 125
+
int calculate_pressure_threshold(struct zone *zone)
{
int threshold;
@@ -110,9 +118,9 @@
threshold = max(1, (int)(watermark_distance / num_online_cpus()));
/*
- * Maximum threshold is 125
+ * Maximum threshold is MAX_THRESHOLD == 125
*/
- threshold = min(125, threshold);
+ threshold = min(MAX_THRESHOLD, threshold);
return threshold;
}
@@ -188,15 +196,31 @@
threshold = calculate_normal_threshold(zone);
for_each_online_cpu(cpu) {
- int pgdat_threshold;
+ int pgdat_threshold, ustat_thresh;
+ struct vmstat_uparam *vup;
- per_cpu_ptr(zone->pageset, cpu)->stat_threshold
- = threshold;
+ struct per_cpu_nodestat __percpu *pcp;
+ struct per_cpu_pageset *p;
+
+ p = per_cpu_ptr(zone->pageset, cpu);
+
+ vup = &per_cpu(vmstat_uparam, cpu);
+ ustat_thresh = atomic_read(&vup->user_stat_thresh);
+
+ if (ustat_thresh)
+ p->stat_threshold = ustat_thresh;
+ else
+ p->stat_threshold = threshold;
+
+ pcp = per_cpu_ptr(pgdat->per_cpu_nodestats, cpu);
/* Base nodestat threshold on the largest populated zone. */
- pgdat_threshold = per_cpu_ptr(pgdat->per_cpu_nodestats, cpu)->stat_threshold;
- per_cpu_ptr(pgdat->per_cpu_nodestats, cpu)->stat_threshold
- = max(threshold, pgdat_threshold);
+ pgdat_threshold = pcp->stat_threshold;
+ if (ustat_thresh)
+ pcp->stat_threshold = ustat_thresh;
+ else
+ pcp->stat_threshold = max(threshold,
+ pgdat_threshold);
}
/*
@@ -226,9 +250,24 @@
continue;
threshold = (*calculate_pressure)(zone);
- for_each_online_cpu(cpu)
+ for_each_online_cpu(cpu) {
+ int t, ustat_thresh;
+ struct vmstat_uparam *vup;
+
+ vup = &per_cpu(vmstat_uparam, cpu);
+ ustat_thresh = atomic_read(&vup->user_stat_thresh);
+ t = threshold;
+
+ /*
+ * min because pressure could cause
+ * calculate_pressure'ed value to be smaller.
+ */
+ if (ustat_thresh)
+ t = min(threshold, ustat_thresh);
+
per_cpu_ptr(zone->pageset, cpu)->stat_threshold
- = threshold;
+ = t;
+ }
}
}
@@ -249,7 +288,7 @@
t = __this_cpu_read(pcp->stat_threshold);
- if (unlikely(x > t || x < -t)) {
+ if (unlikely(x >= t || x <= -t)) {
zone_page_state_add(x, zone, item);
x = 0;
}
@@ -269,7 +308,7 @@
t = __this_cpu_read(pcp->stat_threshold);
- if (unlikely(x > t || x < -t)) {
+ if (unlikely(x >= t || x <= -t)) {
node_page_state_add(x, pgdat, item);
x = 0;
}
@@ -308,7 +347,7 @@
v = __this_cpu_inc_return(*p);
t = __this_cpu_read(pcp->stat_threshold);
- if (unlikely(v > t)) {
+ if (unlikely(v >= t)) {
s8 overstep = t >> 1;
zone_page_state_add(v + overstep, zone, item);
@@ -324,7 +363,7 @@
v = __this_cpu_inc_return(*p);
t = __this_cpu_read(pcp->stat_threshold);
- if (unlikely(v > t)) {
+ if (unlikely(v >= t)) {
s8 overstep = t >> 1;
node_page_state_add(v + overstep, pgdat, item);
@@ -352,7 +391,7 @@
v = __this_cpu_dec_return(*p);
t = __this_cpu_read(pcp->stat_threshold);
- if (unlikely(v < - t)) {
+ if (unlikely(v <= - t)) {
s8 overstep = t >> 1;
zone_page_state_add(v - overstep, zone, item);
@@ -368,7 +407,7 @@
v = __this_cpu_dec_return(*p);
t = __this_cpu_read(pcp->stat_threshold);
- if (unlikely(v < - t)) {
+ if (unlikely(v <= - t)) {
s8 overstep = t >> 1;
node_page_state_add(v - overstep, pgdat, item);
@@ -426,7 +465,7 @@
o = this_cpu_read(*p);
n = delta + o;
- if (n > t || n < -t) {
+ if (n >= t || n <= -t) {
int os = overstep_mode * (t >> 1) ;
/* Overflow must be added to zone counters */
@@ -483,7 +522,7 @@
o = this_cpu_read(*p);
n = delta + o;
- if (n > t || n < -t) {
+ if (n >= t || n <= -t) {
int os = overstep_mode * (t >> 1) ;
/* Overflow must be added to node counters */
@@ -1696,6 +1735,96 @@
round_jiffies_relative(sysctl_stat_interval));
}
+#ifdef CONFIG_SYSFS
+
+static ssize_t vmstat_thresh_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ int ret;
+ struct vmstat_uparam *vup;
+ unsigned int cpu = dev->id;
+
+ preempt_disable();
+
+ vup = &per_cpu(vmstat_uparam, cpu);
+ ret = sprintf(buf, "%d\n", atomic_read(&vup->user_stat_thresh));
+
+ preempt_enable();
+
+ return ret;
+}
+
+static ssize_t vmstat_thresh_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ int ret, val;
+ unsigned int cpu = dev->id;
+ struct vmstat_uparam *vup;
+
+ ret = sscanf(buf, "%d", &val);
+ if (ret != 1 || val < 1 || val > MAX_THRESHOLD)
+ return -EINVAL;
+
+ preempt_disable();
+
+ if (cpu_online(cpu)) {
+ vup = &per_cpu(vmstat_uparam, cpu);
+ atomic_set(&vup->user_stat_thresh, val);
+ } else
+ count = -EINVAL;
+
+ preempt_enable();
+
+ return count;
+}
+
+struct device_attribute vmstat_threshold_attr =
+ __ATTR(vmstat_threshold, 0644, vmstat_thresh_show, vmstat_thresh_store);
+
+static struct attribute *vmstat_attrs[] = {
+ &vmstat_threshold_attr.attr,
+ NULL
+};
+
+static struct attribute_group vmstat_attr_group = {
+ .attrs = vmstat_attrs,
+ .name = "vmstat"
+};
+
+static int vmstat_thresh_cpu_online(unsigned int cpu)
+{
+ struct device *dev = get_cpu_device(cpu);
+ int ret;
+
+ ret = sysfs_create_group(&dev->kobj, &vmstat_attr_group);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int vmstat_thresh_cpu_down_prep(unsigned int cpu)
+{
+ struct device *dev = get_cpu_device(cpu);
+
+ sysfs_remove_group(&dev->kobj, &vmstat_attr_group);
+ return 0;
+}
+
+static void init_vmstat_sysfs(void)
+{
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ struct vmstat_uparam *vup = &per_cpu(vmstat_uparam, cpu);
+
+ atomic_set(&vup->user_stat_thresh, 0);
+ }
+}
+
+#endif /* CONFIG_SYSFS */
+
static void __init init_cpu_node_state(void)
{
int node;
@@ -1723,9 +1852,12 @@
{
const struct cpumask *node_cpus;
int node;
+ struct vmstat_uparam *vup = &per_cpu(vmstat_uparam, cpu);
node = cpu_to_node(cpu);
+ atomic_set(&vup->user_stat_thresh, 0);
+
refresh_zone_stat_thresholds();
node_cpus = cpumask_of_node(node);
if (cpumask_weight(node_cpus) > 0)
@@ -1735,7 +1867,7 @@
return 0;
}
-#endif
+#endif /* CONFIG_SMP */
struct workqueue_struct *mm_percpu_wq;
@@ -1772,6 +1904,24 @@
#endif
}
+static int __init init_mm_internals_late(void)
+{
+#ifdef CONFIG_SYSFS
+ int ret;
+
+ init_vmstat_sysfs();
+
+ ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "mm/vmstat_thresh:online",
+ vmstat_thresh_cpu_online,
+ vmstat_thresh_cpu_down_prep);
+ if (ret < 0)
+ pr_err("vmstat_thresh: failed to register 'online' hotplug state\n");
+#endif
+ return 0;
+}
+
+late_initcall(init_mm_internals_late);
+
#if defined(CONFIG_DEBUG_FS) && defined(CONFIG_COMPACTION)
/*
Index: linux-2.6-git-disable-vmstat-worker/Documentation/vm/vmstat_thresholds.txt
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6-git-disable-vmstat-worker/Documentation/vm/vmstat_thresholds.txt 2017-05-02 13:48:45.946840708 -0300
@@ -0,0 +1,78 @@
+Userspace configurable vmstat thresholds
+========================================
+
+This document describes the tunables to control
+per-CPU vmstat threshold and per-CPU vmstat worker
+thread.
+
+/sys/devices/system/cpu/cpuN/vmstat/vmstat_threshold:
+
+This file contains the per-CPU vmstat threshold.
+This value is the maximum that a single per-CPU vmstat statistic
+can accumulate before transferring to the global counters.
+
+A value of 0 indicates that the value is set
+by the in kernel algorithm.
+
+A value different than 0 indicates that particular
+value is used for vmstat_threshold.
+
+/sys/devices/system/cpu/cpuN/vmstat/vmstat_worker:
+
+Enable/disable the per-CPU vmstat worker.
+
+What does the vmstat_threshold value mean? What are the implications
+of changing this value? What's the difference in choosing 1, 2, 3
+or 500?
+====================================================================
+
+Its the maximum value for a vmstat statistics counter to hold. After
+that value, the statistics are transferred to the global counter:
+
+void __mod_node_page_state(struct pglist_data *pgdat, enum node_stat_item item,
+ long delta)
+{
+ struct per_cpu_nodestat __percpu *pcp = pgdat->per_cpu_nodestats;
+ s8 __percpu *p = pcp->vm_node_stat_diff + item;
+ long x;
+ long t;
+
+ x = delta + __this_cpu_read(*p);
+
+ t = __this_cpu_read(pcp->stat_threshold);
+
+ if (unlikely(x > t || x < -t)) {
+ node_page_state_add(x, pgdat, item);
+ x = 0;
+ }
+ __this_cpu_write(*p, x);
+}
+
+Increasing the threshold value does two things:
+ 1) It decreases the number of inter-processor accesses.
+ 2) It increases how much the global counters stay out of
+ sync relative to actual current values.
+
+
+Usage example:
+=============
+
+In a realtime system, the worker thread waking up and executing
+vmstat_update can be an undesired source of latencies.
+
+To avoid the worker thread from waking up, executing vmstat_update
+on cpu 1, for example, perform the following steps:
+
+
+cd /sys/devices/system/cpu/cpu0/vmstat/
+
+# Set vmstat threshold to 1 for cpu1, so that no
+# vmstat statistics are collected in cpu1's per-cpu
+# stats, instead they are immediately transferred
+# to the global counter.
+
+$ echo 1 > vmstat_threshold
+
+# Disable vmstat_update worker for cpu1:
+$ echo 0 > vmstat_worker
+
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH][RFC] mm: make kswapd try harder to keep active pages in cache
From: Josef Bacik @ 2017-05-03 18:38 UTC (permalink / raw)
To: Rik van Riel; +Cc: Josef Bacik, linux-mm, hannes, kernel-team
In-Reply-To: <1493835888.20270.4.camel@redhat.com>
On Wed, May 03, 2017 at 02:24:48PM -0400, Rik van Riel wrote:
> On Tue, 2017-05-02 at 17:27 -0400, Josef Bacik wrote:
>
> > + /*
> > + * If we don't have a lot of inactive or slab pages then
> > there's no
> > + * point in trying to free them exclusively, do the normal
> > scan stuff.
> > + */
> > + if (nr_inactive < total_high_wmark && nr_slab <
> > total_high_wmark)
> > + sc->inactive_only = 0;
>
> This part looks good. Below this point, there is obviously no
> point in skipping the active list.
>
> > + if (!global_reclaim(sc))
> > + sc->inactive_only = 0;
>
> Why the different behaviour with and without cgroups?
>
> Have you tested both of these?
>
Huh oops I thought I deleted that, sorry I'll kill that part.
> > + /*
> > + * We still want to slightly prefer slab over inactive, so
> > if inactive
> > + * is large enough just skip slab shrinking for now. If we
> > aren't able
> > + * to reclaim enough exclusively from the inactive lists
> > then we'll
> > + * reset this on the first loop and dip into slab.
> > + */
> > + if (nr_inactive > total_high_wmark && nr_inactive > nr_slab)
> > + skip_slab = true;
>
> I worry that this may be a little too aggressive,
> and result in the slab cache growing much larger
> than it should be on some systems.
>
> I wonder if it may make more sense to have the
> aggressiveness of slab scanning depend on the
> ratio of inactive to reclaimable slab pages, rather
> than having a hard cut-off like this?
>
So I originally had a thing that kept track of the rate of change of inactive vs
slab between kswapd runs, but this worked fine so I figured simpler was better.
Keep in mind that we only skip slab the first loop through, so if we fail to
free enough on the inactive list the first time through then we start evicting
slab as well. The idea is (and my testing bore this out) that with the new size
ratio way of shrinking slab we would sometimes be over zealous and evict slab
that we were actively using, even though we had reclaimed plenty of pages from
our inactive list to satisfy our sc->nr_to_reclaim.
I could probably change the ratio in the sc->inactive_only case to be based on
the slab to inactive ratio and see how that turns out, I'll get that wired up
and let you know how it goes. Thanks,
Josef
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH][RFC] mm: make kswapd try harder to keep active pages in cache
From: Rik van Riel @ 2017-05-03 18:24 UTC (permalink / raw)
To: Josef Bacik, linux-mm, hannes, kernel-team
In-Reply-To: <1493760444-18250-1-git-send-email-jbacik@fb.com>
[-- Attachment #1.1: Type: text/plain, Size: 1352 bytes --]
On Tue, 2017-05-02 at 17:27 -0400, Josef Bacik wrote:
> + /*
> + * If we don't have a lot of inactive or slab pages then
> there's no
> + * point in trying to free them exclusively, do the normal
> scan stuff.
> + */
> + if (nr_inactive < total_high_wmark && nr_slab <
> total_high_wmark)
> + sc->inactive_only = 0;
This part looks good. Below this point, there is obviously no
point in skipping the active list.
> + if (!global_reclaim(sc))
> + sc->inactive_only = 0;
Why the different behaviour with and without cgroups?
Have you tested both of these?
> + /*
> + * We still want to slightly prefer slab over inactive, so
> if inactive
> + * is large enough just skip slab shrinking for now. If we
> aren't able
> + * to reclaim enough exclusively from the inactive lists
> then we'll
> + * reset this on the first loop and dip into slab.
> + */
> + if (nr_inactive > total_high_wmark && nr_inactive > nr_slab)
> + skip_slab = true;
I worry that this may be a little too aggressive,
and result in the slab cache growing much larger
than it should be on some systems.
I wonder if it may make more sense to have the
aggressiveness of slab scanning depend on the
ratio of inactive to reclaimable slab pages, rather
than having a hard cut-off like this?
--
All rights reversed
[-- Attachment #1.2: Type: text/html, Size: 1942 bytes --]
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 473 bytes --]
^ permalink raw reply
* Re: [v2 3/5] mm: add "zero" argument to vmemmap allocators
From: Pasha Tatashin @ 2017-05-03 15:05 UTC (permalink / raw)
To: David Miller
Cc: linux-kernel, sparclinux, linux-mm, linuxppc-dev, linux-s390,
borntraeger, heiko.carstens, willy
In-Reply-To: <20170503.103428.1598887340082574002.davem@davemloft.net>
Hi Dave,
Thank you for the review. I will address your comment and update patchset..
Pasha
On 05/03/2017 10:34 AM, David Miller wrote:
> From: Pavel Tatashin <pasha.tatashin@oracle.com>
> Date: Fri, 24 Mar 2017 15:19:50 -0400
>
>> Allow clients to request non-zeroed memory from vmemmap allocator.
>> The following two public function have a new boolean argument called zero:
>>
>> __vmemmap_alloc_block_buf()
>> vmemmap_alloc_block()
>>
>> When zero is true, memory that is allocated by memblock allocator is zeroed
>> (the current behavior), when argument is false, the memory is not zeroed.
>>
>> This change allows for optimizations where client knows when it is better
>> to zero memory: may be later when other CPUs are started, or may be client
>> is going to set every byte in the allocated memory, so no need to zero
>> memory beforehand.
>>
>> Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
>> Reviewed-by: Shannon Nelson <shannon.nelson@oracle.com>
>
> I think when you add a new argument that can adjust behavior, you
> should add the new argument but retain exactly the current behavior in
> the existing calls.
>
> Then later you can piece by piece change behavior, and document properly
> in the commit message what is happening and why the transformation is
> legal.
>
> Here, you are adding the new boolean to __earlyonly_bootmem_alloc() and
> then making sparse_mem_maps_populate_node() pass false, which changes
> behavior such that it doesn't get zero'd memory any more.
>
> Please make one change at a time. Otherwise review and bisection is
> going to be difficult.
>
> --
> To unsubscribe from this list: send the line "unsubscribe sparclinux" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [v2 3/5] mm: add "zero" argument to vmemmap allocators
From: David Miller @ 2017-05-03 14:34 UTC (permalink / raw)
To: pasha.tatashin
Cc: linux-kernel, sparclinux, linux-mm, linuxppc-dev, linux-s390,
borntraeger, heiko.carstens, willy
In-Reply-To: <1490383192-981017-4-git-send-email-pasha.tatashin@oracle.com>
From: Pavel Tatashin <pasha.tatashin@oracle.com>
Date: Fri, 24 Mar 2017 15:19:50 -0400
> Allow clients to request non-zeroed memory from vmemmap allocator.
> The following two public function have a new boolean argument called zero:
>
> __vmemmap_alloc_block_buf()
> vmemmap_alloc_block()
>
> When zero is true, memory that is allocated by memblock allocator is zeroed
> (the current behavior), when argument is false, the memory is not zeroed.
>
> This change allows for optimizations where client knows when it is better
> to zero memory: may be later when other CPUs are started, or may be client
> is going to set every byte in the allocated memory, so no need to zero
> memory beforehand.
>
> Signed-off-by: Pavel Tatashin <pasha.tatashin@oracle.com>
> Reviewed-by: Shannon Nelson <shannon.nelson@oracle.com>
I think when you add a new argument that can adjust behavior, you
should add the new argument but retain exactly the current behavior in
the existing calls.
Then later you can piece by piece change behavior, and document properly
in the commit message what is happening and why the transformation is
legal.
Here, you are adding the new boolean to __earlyonly_bootmem_alloc() and
then making sparse_mem_maps_populate_node() pass false, which changes
behavior such that it doesn't get zero'd memory any more.
Please make one change at a time. Otherwise review and bisection is
going to be difficult.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm, vmalloc: properly track vmalloc users
From: Michal Hocko @ 2017-05-03 13:09 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, Andrew Morton, Vlastimil Babka, linux-mm, LKML
In-Reply-To: <20170503063750.GC1236@dhcp22.suse.cz>
On Wed 03-05-17 08:37:50, Michal Hocko wrote:
> On Wed 03-05-17 08:52:01, kbuild test robot wrote:
> > Hi Michal,
> >
> > [auto build test ERROR on mmotm/master]
> > [also build test ERROR on next-20170502]
> > [cannot apply to v4.11]
> > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
> >
> > url: https://github.com/0day-ci/linux/commits/Michal-Hocko/mm-vmalloc-properly-track-vmalloc-users/20170503-065022
> > base: git://git.cmpxchg.org/linux-mmotm.git master
> > config: m68k-m5475evb_defconfig (attached as .config)
> > compiler: m68k-linux-gcc (GCC) 4.9.0
> > reproduce:
> > wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> > chmod +x ~/bin/make.cross
> > # save the attached .config to linux build tree
> > make.cross ARCH=m68k
> >
> > All error/warnings (new ones prefixed by >>):
> >
> > In file included from arch/m68k/include/asm/pgtable_mm.h:145:0,
> > from arch/m68k/include/asm/pgtable.h:4,
> > from include/linux/vmalloc.h:9,
> > from arch/m68k/kernel/module.c:9:
>
> OK, I was little bit worried to pull pgtable.h include in, but my cross
> compile build test battery didn't show any issues. I do not have m68k
> there though. So let's just do this differently. The following updated
> patch hasn't passed the full build test battery but it should just work.
I assume that the silence from the kbuild robot means good to go here.
Andrew, could you replace the previous patch by the following one please?
> From 33a6239135cb444654f48d5e942e7f34898e24ea Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Tue, 2 May 2017 11:18:29 +0200
> Subject: [PATCH] mm, vmalloc: properly track vmalloc users
>
> __vmalloc_node_flags used to be static inline but this has changed by
> "mm: introduce kv[mz]alloc helpers" because kvmalloc_node needs to use
> it as well and the code is outside of the vmalloc proper. I haven't
> realized that changing this will lead to a subtle bug though. The
> function is responsible to track the caller as well. This caller is
> then printed by /proc/vmallocinfo. If __vmalloc_node_flags is not inline
> then we would get only direct users of __vmalloc_node_flags as callers
> (e.g. v[mz]alloc) which reduces usefulness of this debugging feature
> considerably. It simply doesn't help to see that the given range belongs
> to vmalloc as a caller:
> 0xffffc90002c79000-0xffffc90002c7d000 16384 vmalloc+0x16/0x18 pages=3 vmalloc N0=3
> 0xffffc90002c81000-0xffffc90002c85000 16384 vmalloc+0x16/0x18 pages=3 vmalloc N1=3
> 0xffffc90002c8d000-0xffffc90002c91000 16384 vmalloc+0x16/0x18 pages=3 vmalloc N1=3
> 0xffffc90002c95000-0xffffc90002c99000 16384 vmalloc+0x16/0x18 pages=3 vmalloc N1=3
>
> We really want to catch the _caller_ of the vmalloc function. Fix this
> issue by making __vmalloc_node_flags static inline again and export
> __vmalloc_node_flags_caller for kvmalloc_node().
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> include/linux/vmalloc.h | 16 +++++++++++++++-
> mm/util.c | 3 ++-
> mm/vmalloc.c | 8 +++++++-
> 3 files changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/include/linux/vmalloc.h b/include/linux/vmalloc.h
> index 46991ad3ddd5..4a0fabeb1e92 100644
> --- a/include/linux/vmalloc.h
> +++ b/include/linux/vmalloc.h
> @@ -80,7 +80,21 @@ extern void *__vmalloc_node_range(unsigned long size, unsigned long align,
> unsigned long start, unsigned long end, gfp_t gfp_mask,
> pgprot_t prot, unsigned long vm_flags, int node,
> const void *caller);
> -extern void *__vmalloc_node_flags(unsigned long size, int node, gfp_t flags);
> +#ifndef CONFIG_MMU
> +extern void *__vmalloc_node_flags_caller(unsigned long size, int node, gfp_t flags);
> +static inline void *__vmalloc_node_flags_caller(unsigned long size, int node, gfp_t flags, void* caller)
> +{
> + return __vmalloc_node_flags(size, node, flags);
> +}
> +#else
> +/*
> + * We really want to have this inlined due to caller tracking. This
> + * function is used by the highlevel vmalloc apis and so we want to track
> + * their callers and inlining will achieve that.
> + */
> +extern void *__vmalloc_node_flags_caller(unsigned long size,
> + int node, gfp_t flags, void* caller);
> +#endif
>
> extern void vfree(const void *addr);
> extern void vfree_atomic(const void *addr);
> diff --git a/mm/util.c b/mm/util.c
> index 3022051da938..c35e5870921d 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -380,7 +380,8 @@ void *kvmalloc_node(size_t size, gfp_t flags, int node)
> if (ret || size <= PAGE_SIZE)
> return ret;
>
> - return __vmalloc_node_flags(size, node, flags);
> + return __vmalloc_node_flags_caller(size, node, flags,
> + __builtin_return_address(0));
> }
> EXPORT_SYMBOL(kvmalloc_node);
>
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 65912eb93a2c..1a97d4a31406 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -1809,13 +1809,19 @@ void *__vmalloc(unsigned long size, gfp_t gfp_mask, pgprot_t prot)
> }
> EXPORT_SYMBOL(__vmalloc);
>
> -void *__vmalloc_node_flags(unsigned long size,
> +static inline void *__vmalloc_node_flags(unsigned long size,
> int node, gfp_t flags)
> {
> return __vmalloc_node(size, 1, flags, PAGE_KERNEL,
> node, __builtin_return_address(0));
> }
>
> +
> +void *__vmalloc_node_flags_caller(unsigned long size, int node, gfp_t flags, void *caller)
> +{
> + return __vmalloc_node(size, 1, flags, PAGE_KERNEL, node, caller);
> +}
> +
> /**
> * vmalloc - allocate virtually contiguous memory
> * @size: allocation size
> --
> 2.11.0
>
> --
> Michal Hocko
> SUSE Labs
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [RFC v3 03/17] mm: Introduce pte_spinlock
From: Laurent Dufour @ 2017-05-03 13:01 UTC (permalink / raw)
To: Matthew Wilcox
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
linux-kernel, linux-mm, haren, khandual, npiggin, bsingharora
In-Reply-To: <20170430044700.GF27790@bombadil.infradead.org>
On 30/04/2017 06:47, Matthew Wilcox wrote:
> On Thu, Apr 27, 2017 at 05:52:42PM +0200, Laurent Dufour wrote:
>> +++ b/mm/memory.c
>> @@ -2100,6 +2100,13 @@ static inline void wp_page_reuse(struct vm_fault *vmf)
>> pte_unmap_unlock(vmf->pte, vmf->ptl);
>> }
>>
>> +static bool pte_spinlock(struct vm_fault *vmf)
>> +{
>> + vmf->ptl = pte_lockptr(vmf->vma->vm_mm, vmf->pmd);
>> + spin_lock(vmf->ptl);
>> + return true;
>> +}
>
> To me 'pte_spinlock' is a noun, but this is really pte_spin_lock() (a verb).
Fair enough. Even pte_trylock() should be more accurate since patch 8/17
changes this function to call spin_trylock().
> Actually, it's really vmf_lock_pte(). We're locking the pte
> referred to by this vmf. And so we should probably have a matching
> vmf_unlock_pte(vmf) to preserve the abstraction.
I'm not sure this will ease the reading. In most of this code, the pte
are unlocked through the call to pte_unmap_unlock().
The call to pte_trylock() has been introduced because in few cases there
is the need to check the VMA validity before calling spinlock(ptl). The
unlock is then managed through pte_unmap_unlock().
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* RFC v2: post-init-read-only protection for data allocated dynamically
From: Igor Stoppa @ 2017-05-03 12:06 UTC (permalink / raw)
To: Michal Hocko; +Cc: linux-mm, linux-kernel
Hello,
please review my (longish) line of thoughts, below.
I've restructured them so that they should be easier to follow.
Observations
------------
* it is currently possible, by using prefix "__read_only", to have the
linker place a static variable into a special memory region, which will
become write-protected at the end of the init phase.
* the purpose is to write-protect data which is not expected to change,
ever, after it has been initialized.
* The mechanism used for locking down the memory region is to program
the MMU to trap writes to said region. It is fairly efficient and
HW-backed, so it doesn't introduce any major overhead, but the MMU deals
only with pages or supersets of pages, hence the need to collect all the
soon-to-be-read-only data - and only that - into the "special region".
The "__read_only" modifier is the admission ticket.
* the write-protecting feature helps supporting memory integrity in
general and can also help spotting rogue writes, whatever their origin
might be: uninitialized or expired pointers, wrong pointer arithmetic, etc.
Problem
-------
The feature is available only for *static* data - it will not work with
something like a linked list that is put together during init, for example.
Wish
----
My starting point are the policy DB of SE Linux and the LSM Hooks, but
eventually I would like to extend the protection also to other
subsystems, in a way that can be merged into mainline.
Analysis
--------
* the solution I come up with has to be as little invasive as possible,
at least for what concerns the various subsystems whose integrity I want
to enhance.
* In most, if not all, the cases that could be enhanced, the code will
be calling kmalloc/vmalloc, indicating GFP_KERNEL as the desired type of
memory.
* I suspect/hope that the various maintainer won't object too much if my
changes are limited to replacing GFP_KERNEL with some other macro, for
example what I previously called GFP_LOCKABLE, provided I can ensure that:
-1) no penalty is introduced, at least when the extra protection
feature is not enabled, iow nobody has to suffer from my changes.
This means that GFP_LOCKABLE should fall back to GFP_KERNEL, when
it's not enabled.
-2) when the extra protection feature is enabled, the code still
works as expected, as long as the data identified for this
enhancement is really unmodified after init.
* In my quest for improved memory integrity, I will deal with very
different memory size being allocated, so if I start writing my own
memory allocator, starting from a page-aligned chunk of normal memory,
at best I will end up with a replica of kmalloc, at worst with something
buggy. Either way, it will be extremely harder to push other subsystems
to use it.
I probably wouldn't like it either, if I was a maintainer.
* While I do not strictly need a new memory zone, memory zones are what
kmalloc understands at the moment: AFAIK, it is not possible to tell
kmalloc from which memory pool it should fish out the memory, other than
having a reference to a memory zone.
If it was possible to aim kmalloc at arbitrary memory pools, probably we
would not be having this exchange right now. And probably there would
not be so many other folks trying to have their memory zone of interest
being merged. However I suspect this solution would be sub-optimal for
the normal use cases, because there would be the extra overhead of
passing the reference to the memory pool, instead of encoding it into
bitfields, together with other information.
* there are very slim chances (to be optimistic :) that I can get away
with having my custom zone merged, because others are trying with
similar proposals and they get rejected, so maybe I can have better luck
if I propose something that can also work for others.
* currently memory zones are mapped 1:1 to bits in crowded a bitmask,
but not all these zones are really needed in a typical real system, some
are kept for backward compatibility and supporting distros, which cannot
know upfront the quirks of the HW they will be running on.
Conclusions
-----------
* the solution that seems to be more likely to succeed is to remove the
1:1 mapping between optional zones and their respective bits.
* the bits previously assigned to the optional zones would become
available for mapping whatever zone a system integrator wants to support.
Cons:
There would be still a hard constraint on the maximum number of zones
available simultaneously, so one will have to choose which of the
optional zones to enable, and be ready to deal with own zone
disappearing (ex: fall back to normal memory and give up some/all
functionality)
Pros:
* No bit would go to waste: those who want to have own custom zone could
make a better use of the allocated-but-not-necessary-to-them bits.
* There would be a standard way for people to add non-standard zones.
* It doesn't alter the hot paths that are critical to efficient memory
handling.
So it seems a win-win scenario, apart from the fact that I will probably
have to reshuffle a certain amount of macros :-)
P.S.
There was an early advice of creating and using a custom-made memory
allocator, I hope it's now clear why I don't think it's viable: it might
work if I use it only for further code that I will write, but it really
doesn't seem the best way to convince other subsystem maintainers to
take in my changes, if I suggest them to give up the super optimized
kmalloc (and friends) in favor of some homebrew allocator I wrote :-/
---
thanks, igor
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v2 1/2] mm: Uncharge poisoned pages
From: Laurent Dufour @ 2017-05-03 11:34 UTC (permalink / raw)
To: Michal Hocko, Andi Kleen, Johannes Weiner
Cc: Naoya Horiguchi, linux-kernel, linux-mm, akpm, Vladimir Davydov
In-Reply-To: <20170502185507.GB19165@dhcp22.suse.cz>
On 02/05/2017 20:55, Michal Hocko wrote:
> On Tue 02-05-17 16:59:30, Laurent Dufour wrote:
>> On 28/04/2017 15:48, Michal Hocko wrote:
> [...]
>>> This is getting quite hairy. What is the expected page count of the
>>> hwpoison page?
>
> OK, so from the quick check of the hwpoison code it seems that the ref
> count will be > 1 (from get_hwpoison_page).
>
>>> I guess we would need to update the VM_BUG_ON in the
>>> memcg uncharge code to ignore the page count of hwpoison pages if it can
>>> be arbitrary.
>>
>> Based on the experiment I did, page count == 2 when isolate_lru_page()
>> succeeds, even in the case of a poisoned page.
>
> that would make some sense to me. The page should have been already
> unmapped therefore but memory_failure increases the ref count and 1 is
> for isolate_lru_page().
>
>> In my case I think this
>> is because the page is still used by the process which is calling madvise().
>>
>> I'm wondering if I'm looking at the right place. May be the poisoned
>> page should remain attach to the memory_cgroup until no one is using it.
>> In that case this means that something should be done when the page is
>> off-lined... I've to dig further here.
>
> No, AFAIU the page will not drop the reference count down to 0 in most
> cases. Maybe there are some scenarios where this can happen but I would
> expect that the poisoned page will be mapped and in use most of the time
> and won't drop down 0. And then we should really uncharge it because it
> will pin the memcg and make it unfreeable which doesn't seem to be what
> we want. So does the following work reasonable? Andi, Johannes, what do
> you think? I cannot say I would be really comfortable touching hwpoison
> code as I really do not understand the workflow. Maybe we want to move
> this uncharge down to memory_failure() right before we report success?
> ---
> From 8bf0791bcf35996a859b6d33fb5494e5b53de49d Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Tue, 2 May 2017 20:32:24 +0200
> Subject: [PATCH] hwpoison, memcg: forcibly uncharge LRU pages
>
> Laurent Dufour has noticed that hwpoinsoned pages are kept charged. In
> his particular case he has hit a bad_page("page still charged to cgroup")
> when onlining a hwpoison page. While this looks like something that shouldn't
> happen in the first place because onlining hwpages and returning them to
> the page allocator makes only little sense it shows a real problem.
>
> hwpoison pages do not get freed usually so we do not uncharge them (at
> least not since 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")).
> Each charge pins memcg (since e8ea14cc6ead ("mm: memcontrol: take a css
> reference for each charged page")) as well and so the mem_cgroup and the
> associated state will never go away. Fix this leak by forcibly
> uncharging a LRU hwpoisoned page in delete_from_lru_cache(). We also
> have to tweak uncharge_list because it cannot rely on zero ref count
> for these pages.
>
> Fixes: 0a31bc97c80c ("mm: memcontrol: rewrite uncharge API")
> Reported-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
FWIW:
Tested-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/memcontrol.c | 2 +-
> mm/memory-failure.c | 7 +++++++
> 2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 16c556ac103d..4cf26059adb1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -5527,7 +5527,7 @@ static void uncharge_list(struct list_head *page_list)
> next = page->lru.next;
>
> VM_BUG_ON_PAGE(PageLRU(page), page);
> - VM_BUG_ON_PAGE(page_count(page), page);
> + VM_BUG_ON_PAGE(!PageHWPoison(page) && page_count(page), page);
>
> if (!page->mem_cgroup)
> continue;
> diff --git a/mm/memory-failure.c b/mm/memory-failure.c
> index 8a6bd3a9eb1e..4497d9619bb4 100644
> --- a/mm/memory-failure.c
> +++ b/mm/memory-failure.c
> @@ -541,6 +541,13 @@ static int delete_from_lru_cache(struct page *p)
> */
> ClearPageActive(p);
> ClearPageUnevictable(p);
> +
> + /*
> + * Poisoned page might never drop its ref count to 0 so we have to
> + * uncharge it manually from its memcg.
> + */
> + mem_cgroup_uncharge(p);
> +
> /*
> * drop the page count elevated by isolate_lru_page()
> */
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Improve documentation request for /proc/meminfo (proc.txt)
From: Ulrich Windl @ 2017-05-03 11:32 UTC (permalink / raw)
To: linux-mm@kvack.org
In-Reply-To: <5909BFD6020000A100025CBF@gwsmtp1.uni-regensburg.de>
Hi!
Reading /usr/src/linux/Documentation/filesystems/proc.txt leaves some questions open. For example some values are break-downs of others. I'd like to see documentation on how these relate. For example (3.0.101):
MemTotal: 132156332 kB
MemFree: 22448480 kB
Buffers: 1484072 kB
Cached: 81252832 kB
SwapCached: 0 kB
Active: 24216920 kB
Inactive: 65789500 kB
Active(anon): 20268724 kB
Inactive(anon): 4617808 kB
Active(file): 3948196 kB
Inactive(file): 61171692 kB
Unevictable: 48840 kB
Mlocked: 30444 kB
SwapTotal: 20964788 kB
SwapFree: 20964788 kB
Dirty: 496 kB
Writeback: 0 kB
AnonPages: 7317972 kB
Mapped: 15927688 kB
Shmem: 17602008 kB
Slab: 1162160 kB
SReclaimable: 907496 kB
SUnreclaim: 254664 kB
KernelStack: 8864 kB
PageTables: 1361780 kB
NFS_Unstable: 0 kB
Bounce: 0 kB
WritebackTmp: 0 kB
CommitLimit: 79158152 kB
Committed_AS: 66056192 kB
VmallocTotal: 34359738367 kB
VmallocUsed: 305164 kB
VmallocChunk: 34292267888 kB
HardwareCorrupted: 0 kB
AnonHugePages: 4997120 kB
HugePages_Total: 7700
HugePages_Free: 62
HugePages_Rsvd: 44
HugePages_Surp: 0
Hugepagesize: 2048 kB
DirectMap4k: 301296 kB
DirectMap2M: 19611648 kB
DirectMap1G: 114294784 kB
Which numbers sum up to MemTotal? It seems Active(anon) + Active(file) == Active (and similar for inactive).
What is the relation between Unevictable and Mlocked? Is "Unevictable >= Mlocked" all the time?
It would give some insight how things work if you document the relations between some of these numbers.
(I'm hunting for a condition for very bad disk response times, suspecting some memory pressure. I suspect too many dirty pages for some reason...)
Regards,
Ulrich
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH v3 3/3] arm64: Silence first allocation with CONFIG_ARM64_MODULE_PLTS=y
From: Will Deacon @ 2017-05-03 11:18 UTC (permalink / raw)
To: Florian Fainelli
Cc: linux-arm-kernel, Russell King, Catalin Marinas, Ard Biesheuvel,
Andrew Morton, Michal Hocko, zijun_hu, Kirill A. Shutemov,
Andrey Ryabinin, Chris Wilson, open list,
open list:MEMORY MANAGEMENT, angus
In-Reply-To: <20170427181902.28829-4-f.fainelli@gmail.com>
On Thu, Apr 27, 2017 at 11:19:02AM -0700, Florian Fainelli wrote:
> When CONFIG_ARM64_MODULE_PLTS is enabled, the first allocation using the
> module space fails, because the module is too big, and then the module
> allocation is attempted from vmalloc space. Silence the first allocation
> failure in that case by setting __GFP_NOWARN.
>
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> arch/arm64/kernel/module.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
I'm not sure what the merge plan is for these, but the arm64 bit here
looks fine to me:
Acked-by: Will Deacon <will.deacon@arm.com>
Will
> diff --git a/arch/arm64/kernel/module.c b/arch/arm64/kernel/module.c
> index 7f316982ce00..093c13541efb 100644
> --- a/arch/arm64/kernel/module.c
> +++ b/arch/arm64/kernel/module.c
> @@ -32,11 +32,16 @@
>
> void *module_alloc(unsigned long size)
> {
> + gfp_t gfp_mask = GFP_KERNEL;
> void *p;
>
> + /* Silence the initial allocation */
> + if (IS_ENABLED(CONFIG_ARM64_MODULE_PLTS))
> + gfp_mask |= __GFP_NOWARN;
> +
> p = __vmalloc_node_range(size, MODULE_ALIGN, module_alloc_base,
> module_alloc_base + MODULES_VSIZE,
> - GFP_KERNEL, PAGE_KERNEL_EXEC, 0,
> + gfp_mask, PAGE_KERNEL_EXEC, 0,
> NUMA_NO_NODE, __builtin_return_address(0));
>
> if (!p && IS_ENABLED(CONFIG_ARM64_MODULE_PLTS) &&
> --
> 2.9.3
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: RFC: post-init-read-only protection for data allocated dynamically
From: Igor Stoppa @ 2017-05-03 9:44 UTC (permalink / raw)
To: Michal Hocko; +Cc: linux-mm
In-Reply-To: <20170502164748.GA19165@dhcp22.suse.cz>
On 02/05/17 19:47, Michal Hocko wrote:
> [You have already started new thread with the way how to introduce a new
> zone and that might turn out to be useful but I think it is much more
> important to understand requirements for the usecase you have in mind as
> first]
Ok, I'm sorry this was not clear, I'll detail it more.
---
thanks, igor
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low
From: Michal Hocko @ 2017-05-03 8:49 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Minchan Kim, Johannes Weiner, Mel Gorman,
linux-kernel, linux-mm
In-Reply-To: <20170503070656.GA8836@dhcp22.suse.cz>
On Wed 03-05-17 09:06:56, Michal Hocko wrote:
[...]
> This is still untested but should be much closer to what I've had in
> mind.
> ---
> diff --git a/mm/vmscan.c b/mm/vmscan.c
> index 24efcc20af91..bcdad30f942d 100644
> --- a/mm/vmscan.c
> +++ b/mm/vmscan.c
> @@ -2174,8 +2174,14 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
> }
>
> if (unlikely(pgdatfile + pgdatfree <= total_high_wmark)) {
> - scan_balance = SCAN_ANON;
> - goto out;
> + unsigned long pgdatanon;
> +
> + pgdatanon = node_page_state(pgdat, NR_ACTIVE_ANON) +
> + node_page_state(pgdat, NR_INACTIVE_ANON);
> + if (pgdatanon + pgdatfree > total_high_wmark) {
> + scan_balance = SCAN_ANON;
> + goto out;
> + }
> }
> }
I've realized that this just makes the situation more obscure than
necessary after thinking some more about it. It also doesn't achieve my
original intention to treat biased anon and file LRUs the same. Now that
I've digested the change more thoroughly I am willing to ack your patch.
So feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>
And sorry about the diversion here but I am always nervous when touching
g_s_c because this tends to lead to subtle issues.
Maybe we could make this aspect of the biased LRUs more explicit by
doing the following rather than duplicating the condition. What do you
think?
---
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 24efcc20af91..f3ec8760dc06 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2113,16 +2113,14 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
u64 denominator = 0; /* gcc */
struct pglist_data *pgdat = lruvec_pgdat(lruvec);
unsigned long anon_prio, file_prio;
- enum scan_balance scan_balance;
+ enum scan_balance scan_balance = SCAN_FILE;
unsigned long anon, file;
unsigned long ap, fp;
enum lru_list lru;
/* If we have no swap space, do not bother scanning anon pages. */
- if (!sc->may_swap || mem_cgroup_get_nr_swap_pages(memcg) <= 0) {
- scan_balance = SCAN_FILE;
+ if (!sc->may_swap || mem_cgroup_get_nr_swap_pages(memcg) <= 0)
goto out;
- }
/*
* Global reclaim will swap to prevent OOM even with no
@@ -2131,10 +2129,8 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
* using the memory controller's swap limit feature would be
* too expensive.
*/
- if (!global_reclaim(sc) && !swappiness) {
- scan_balance = SCAN_FILE;
+ if (!global_reclaim(sc) && !swappiness)
goto out;
- }
/*
* Do not apply any pressure balancing cleverness when the
@@ -2147,8 +2143,9 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
}
/*
- * Prevent the reclaimer from falling into the cache trap: as
- * cache pages start out inactive, every cache fault will tip
+ * We usually want to bias page cache reclaim over anonymous
+ * memory. Prevent the reclaimer from falling into the cache trap:
+ * as cache pages start out inactive, every cache fault will tip
* the scan balance towards the file LRU. And as the file LRU
* shrinks, so does the window for rotation from references.
* This means we have a runaway feedback loop where a tiny
@@ -2173,26 +2170,24 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
total_high_wmark += high_wmark_pages(zone);
}
- if (unlikely(pgdatfile + pgdatfree <= total_high_wmark)) {
+ if (unlikely(pgdatfile + pgdatfree <= total_high_wmark))
scan_balance = SCAN_ANON;
- goto out;
- }
}
/*
- * If there is enough inactive page cache, i.e. if the size of the
- * inactive list is greater than that of the active list *and* the
- * inactive list actually has some pages to scan on this priority, we
- * do not reclaim anything from the anonymous working set right now.
- * Without the second condition we could end up never scanning an
- * lruvec even if it has plenty of old anonymous pages unless the
- * system is under heavy pressure.
+ * Make sure there are enough pages on the biased LRU before we go
+ * and do an exclusive reclaim from that list, i.e. if the
+ * size of the inactive list is greater than that of the active list
+ * *and* the inactive list actually has some pages to scan on this
+ * priority.
+ * Without the second condition we could end up never scanning other
+ * lruvecs even if they have plenty of old pages unless the system is
+ * under heavy pressure.
*/
- if (!inactive_list_is_low(lruvec, true, memcg, sc, false) &&
- lruvec_lru_size(lruvec, LRU_INACTIVE_FILE, sc->reclaim_idx) >> sc->priority) {
- scan_balance = SCAN_FILE;
+ lru = LRU_INACTIVE_ANON + LRU_FILE * (scan_balance == SCAN_FILE);
+ if (!inactive_list_is_low(lruvec, is_file_lru(lru), memcg, sc, false) &&
+ lruvec_lru_size(lruvec, lru, sc->reclaim_idx) >> sc->priority)
goto out;
- }
scan_balance = SCAN_FRACT;
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [RFC v3 05/17] RCU free VMAs
From: Laurent Dufour @ 2017-05-03 7:23 UTC (permalink / raw)
To: Matthew Wilcox
Cc: paulmck, peterz, akpm, kirill, ak, mhocko, dave, jack,
linux-kernel, linux-mm, haren, khandual, npiggin, bsingharora
In-Reply-To: <20170430050529.GH27790@bombadil.infradead.org>
On 30/04/2017 07:05, Matthew Wilcox wrote:
> On Thu, Apr 27, 2017 at 05:52:44PM +0200, Laurent Dufour wrote:
>> +static inline bool vma_is_dead(struct vm_area_struct *vma, unsigned int sequence)
>> +{
>> + int ret = RB_EMPTY_NODE(&vma->vm_rb);
>> + unsigned seq = ACCESS_ONCE(vma->vm_sequence.sequence);
>> +
>> + /*
>> + * Matches both the wmb in write_seqlock_{begin,end}() and
>> + * the wmb in vma_rb_erase().
>> + */
>> + smp_rmb();
>> +
>> + return ret || seq != sequence;
>> +}
>
> Hang on, this isn't vma_is_dead(). This is vma_has_changed() (possibly
> from live to dead, but also possibly grown or shrunk; see your earlier
> patch).
This makes sense.
Thanks,
Laurent
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low
From: Michal Hocko @ 2017-05-03 7:06 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Minchan Kim, Johannes Weiner, Mel Gorman,
linux-kernel, linux-mm
In-Reply-To: <20170503061528.GB1236@dhcp22.suse.cz>
On Wed 03-05-17 08:15:28, Michal Hocko wrote:
> On Tue 02-05-17 13:41:23, David Rientjes wrote:
> > On Tue, 2 May 2017, Michal Hocko wrote:
[...]
> > > I do agree that blindly
> > > scanning anon pages when file pages are low is very suboptimal but this
> > > adds yet another heuristic without _any_ numbers. Why cannot we simply
> > > treat anon and file pages equally? Something like the following
> > >
> > > if (pgdatfile + pgdatanon + pgdatfree > 2*total_high_wmark) {
> > > scan_balance = SCAN_FILE;
> > > if (pgdatfile < pgdatanon)
> > > scan_balance = SCAN_ANON;
> > > goto out;
> > > }
> > >
> >
> > This would be substantially worse than the current code because it
> > thrashes the anon lru when anon out numbers file pages rather than at the
> > point we fall under the high watermarks for all eligible zones. If you
> > tested your suggestion, you could see gigabytes of memory left untouched
> > on the file lru. Anonymous memory is more probable to be part of the
> > working set.
>
> This was supposed to be more an example of a direction I was thinking,
> definitely not a final patch. I will think more to come up with a
> more complete proposal.
This is still untested but should be much closer to what I've had in
mind.
---
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 24efcc20af91..bcdad30f942d 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2174,8 +2174,14 @@ static void get_scan_count(struct lruvec *lruvec, struct mem_cgroup *memcg,
}
if (unlikely(pgdatfile + pgdatfree <= total_high_wmark)) {
- scan_balance = SCAN_ANON;
- goto out;
+ unsigned long pgdatanon;
+
+ pgdatanon = node_page_state(pgdat, NR_ACTIVE_ANON) +
+ node_page_state(pgdat, NR_INACTIVE_ANON);
+ if (pgdatanon + pgdatfree > total_high_wmark) {
+ scan_balance = SCAN_ANON;
+ goto out;
+ }
}
}
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [RESENT PATCH] x86/mem: fix the offset overflow when read/write mem
From: zhong jiang @ 2017-05-03 6:42 UTC (permalink / raw)
To: David Rientjes
Cc: Bjorn Helgaas, Yoshinori Sato, Rich Felker, Andrew Morton, arnd,
hannes, kirill, mgorman, hughd, riel, linux-mm, linux-kernel
In-Reply-To: <alpine.DEB.2.10.1705021350510.116499@chino.kir.corp.google.com>
On 2017/5/3 4:54, David Rientjes wrote:
> On Thu, 27 Apr 2017, zhongjiang wrote:
>
>> From: zhong jiang <zhongjiang@huawei.com>
>>
>> Recently, I found the following issue, it will result in the panic.
>>
>> [ 168.739152] mmap1: Corrupted page table at address 7f3e6275a002
>> [ 168.745039] PGD 61f4a1067
>> [ 168.745040] PUD 61ab19067
>> [ 168.747730] PMD 61fb8b067
>> [ 168.750418] PTE 8000100000000225
>> [ 168.753109]
>> [ 168.757795] Bad pagetable: 000d [#1] SMP
>> [ 168.761696] Modules linked in: intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_simd iTCO_wdt glue_helper cryptd sg iTCO_vendor_support i7core_edac edac_core shpchp lpc_ich i2c_i801 pcspkr mfd_core acpi_cpufreq ip_tables xfs libcrc32c sd_mod igb ata_generic ptp pata_acpi pps_core mptsas ata_piix scsi_transport_sas i2c_algo_bit libata mptscsih i2c_core serio_raw crc32c_intel bnx2 mptbase dca dm_mirror dm_region_hash dm_log dm_mod
>> [ 168.805983] CPU: 15 PID: 10369 Comm: mmap1 Not tainted 4.11.0-rc2-327.28.3.53.x86_64+ #345
>> [ 168.814202] Hardware name: Huawei Technologies Co., Ltd. Tecal RH2285 /BC11BTSA , BIOS CTSAV036 04/27/2011
>> [ 168.825704] task: ffff8806207d5200 task.stack: ffffc9000c340000
>> [ 168.831592] RIP: 0033:0x7f3e622c5360
>> [ 168.835147] RSP: 002b:00007ffe2bb7a098 EFLAGS: 00010203
>> [ 168.840344] RAX: 00007ffe2bb7a0c0 RBX: 0000000000000000 RCX: 00007f3e6275a000
>> [ 168.847439] RDX: 00007f3e622c5360 RSI: 00007f3e6275a000 RDI: 00007ffe2bb7a0c0
>> [ 168.854535] RBP: 00007ffe2bb7a4e0 R08: 00007f3e621c3d58 R09: 000000000000002d
>> [ 168.861632] R10: 00007ffe2bb79e20 R11: 00007f3e622fbcb0 R12: 00000000004005d0
>> [ 168.868728] R13: 00007ffe2bb7a5c0 R14: 0000000000000000 R15: 0000000000000000
>> [ 168.875825] FS: 00007f3e62752740(0000) GS:ffff880627bc0000(0000) knlGS:0000000000000000
>> [ 168.883870] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>> [ 168.889583] CR2: 00007f3e6275a002 CR3: 0000000622845000 CR4: 00000000000006e0
>> [ 168.896680] RIP: 0x7f3e622c5360 RSP: 00007ffe2bb7a098
>> [ 168.901713] ---[ end trace ef98fa9f2a01cbc6 ]---
>> [ 168.90630 arch/x86/kernel/smp.c:127 native_smp_send_reschedule+0x3f/0x50
>> [ 168.935410] Modules linked in: intel_powerclamp coretemp kvm_intel kvm irqbypass crct10dif_pclmul crc32_pclmul ghash_clmulni_intel pcbc aesni_intel crypto_simd iTCO_wdt glue_helper cryptd sg iTCO_vendor_support i7core_edac edac_core shpchp lpc_ich i2c_i801 pcspkr mfd_core acpi_cpufreq ip_tables xfs libcrc32c sd_mod igb ata_generic ptp pata_acpi pps_core mptsas ata_piix scsi_transport_sas i2c_algo_bit libata mptscsih i2c_core serio_raw crc32c_intel bnx2 mptbase dca dm_mirror dm_region_hash dm_log dm_mod
>> [ 168.979686] CPU: 15 PID: 10369 Comm: mmap1 Tainted: G D 4.11.0-rc2-327.28.3.53.x86_64+ #345
>> [ 168.989114] Hardware name: Huawei Technologies Co., Ltd. Tecal RH2285 /BC11BTSA , BIOS CTSAV036 04/27/2011
>> [ 169.000616] Call Trace:
>> [ 169.003049] <IRQ>
>> [ 169.005050] dump_stack+0x63/0x84
>> [ 169.008348] __warn+0xd1/0xf0
>> [ 169.011297] warn_slowpath_null+0x1d/0x20
>> [ 169.015282] native_smp_send_reschedule+0x3f/0x50
>> [ 169.019961] resched_curr+0xa1/0xc0
>> [ 169.023428] check_preempt_curr+0x70/0x90
>> [ 169.027415] ttwu_do_wakeup+0x1e/0x160
>> [ 169.031142] ttwu_do_activate+0x77/0x80
>> [ 169.034956] try_to_wake_up+0x1c3/0x430
>> [ 169.038771] default_wake_function+0x12/0x20
>> [ 169.043019] __wake_up_common+0x55/0x90
>> [ 169.046833] __wake_up_locked+0x13/0x20
>> [ 169.050649] ep_poll_callback+0xbb/0x240
>> [ 169.054550] __wake_up_common+0x55/0x90
>> [ 169.058363] __wake_up+0x39/0x50
>> [ 169.061574] wake_up_klogd_work_func+0x40/0x60
>> [ 169.065994] irq_work_run_list+0x4d/0x70
>> [ 169.069895] irq_work_tick+0x40/0x50
>> [ 169.073452] update_process_times+0x42/0x60
>> [ 169.077612] tick_periodic+0x2b/0x80
>> [ 169.081166] tick_handle_periodic+0x25/0x70
>> [ 169.085326] local_apic_timer_interrupt+0x35/0x60
>> [ 169.090004] smp_apic_timer_interrupt+0x38/0x50
>> [ 169.094507] apic_timer_interrupt+0x93/0xa0
>> [ 169.098667] RIP: 0010:panic+0x1f5/0x239
>> [ 169.102480] RSP: 0000:ffffc9000c343dd8 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff10
>> [ 169.110010] RAX: 0000000000000034 RBX: 0000000000000000 RCX: 0000000000000006
>> [ 169.117106] RDX: 0000000000000000 RSI: 0000000000000086 RDI: ffff880627bcdfe0
>> [ 169.124201] RBP: ffffc9000c343e48 R08: 00000000fffffffe R09: 0000000000000395
>> [ 169.131298] R10: 0000000000000005 R11: 0000000000000394 R12: ffffffff81a0c475
>> [ 169.138395] R13: 0000000000000000 R14: 0000000000000000 R15: 000000000000000d
>> [ 169.145491] </IRQ>
>> [ 169.147578] ? panic+0x1f1/0x239
>> [ 169.150789] oops_end+0xb8/0xd0
>> [ 169.153910] pgtable_bad+0x8a/0x95
>> [ 169.157294] __do_page_fault+0x3aa/0x4a0
>> [ 169.161194] do_page_fault+0x30/0x80
>> [ 169.164750] ? do_syscall_64+0x175/0x180
>> [ 169.168649] page_fault+0x28/0x30
>>
>> the following case can reproduce the issue.
>>
>> int mem_fd = 0;
>> char rw_buf[1024];
>> unsigned char * map_base_s;
>> unsigned long show_addr = 0x100000000000;
>> unsigned long show_len = 0x10;
>>
>> if(argc !=2 )
>> {
>> printf( "%s show_addr\n", argv[0] );
>> return 0;
>> }
>> else
>> {
>> char *stop;
>> show_addr = strtoul( argv[1], &stop, 0 );
>> printf("show_addr= 0x%lu\n", show_addr );
>> }
>>
>> mem_fd = open(DEV_NAME, O_RDONLY);
>> if (mem_fd == -1)
>> {
>> printf("open %s failed.", DEV_NAME);
>> return 0;
>> }
>>
>> map_base_s = mmap(NULL, show_len, PROT_READ, MAP_SHARED, mem_fd, show_addr);
>> if ((long)map_base_s == -1)
>> {
>> printf("input address map to user space fail!\n");
>> return 0;
>> }
>> else
>> {
>> printf("mmap successfull!\n");
>> }
>>
>> memcpy( rw_buf, map_base_s, show_len );
>>
>> The pgoff is enough large, it exceed the size of the real memory.
>> and the mmap can return the success.
>>
>> I fix it by checking the conditions. it can make it suitable for
>> the mapped and use.
>>
>> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
>> ---
>> drivers/char/mem.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
>> index 7e4a9d1..3a765e02 100644
>> --- a/drivers/char/mem.c
>> +++ b/drivers/char/mem.c
>> @@ -55,7 +55,7 @@ static inline int valid_phys_addr_range(phys_addr_t addr, size_t count)
>>
>> static inline int valid_mmap_phys_addr_range(unsigned long pfn, size_t size)
>> {
>> - return 1;
>> + return (pfn << PAGE_SHIFT) + size <= __pa(high_memory);
>> }
>> #endif
>>
> I suppose you are correct that there should be some sanity checking on the
> size used for the mmap(). Are you using the default size in your test
> program or are you passing it a different size to induce the panic?
Just use the default size.
> There's also the issue of architectures that override this function, such
> as on sh that simply returns 1, so I think that would need to be addressed
> as well.
yes
> Let's add a few people to the cc.
>
> .
I get less feedback so far, but It indeed will induce the panic.
Thanks
zhongjiang
>
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH] mm, vmalloc: properly track vmalloc users
From: Michal Hocko @ 2017-05-03 6:37 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, Andrew Morton, Vlastimil Babka, linux-mm, LKML
In-Reply-To: <201705030806.pzzQRBiN%fengguang.wu@intel.com>
On Wed 03-05-17 08:52:01, kbuild test robot wrote:
> Hi Michal,
>
> [auto build test ERROR on mmotm/master]
> [also build test ERROR on next-20170502]
> [cannot apply to v4.11]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Michal-Hocko/mm-vmalloc-properly-track-vmalloc-users/20170503-065022
> base: git://git.cmpxchg.org/linux-mmotm.git master
> config: m68k-m5475evb_defconfig (attached as .config)
> compiler: m68k-linux-gcc (GCC) 4.9.0
> reproduce:
> wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=m68k
>
> All error/warnings (new ones prefixed by >>):
>
> In file included from arch/m68k/include/asm/pgtable_mm.h:145:0,
> from arch/m68k/include/asm/pgtable.h:4,
> from include/linux/vmalloc.h:9,
> from arch/m68k/kernel/module.c:9:
OK, I was little bit worried to pull pgtable.h include in, but my cross
compile build test battery didn't show any issues. I do not have m68k
there though. So let's just do this differently. The following updated
patch hasn't passed the full build test battery but it should just work.
Thanks for your testing.
---
^ permalink raw reply
* Re: [patch v2] mm, vmscan: avoid thrashing anon lru when free + file is low
From: Michal Hocko @ 2017-05-03 6:15 UTC (permalink / raw)
To: David Rientjes
Cc: Andrew Morton, Minchan Kim, Johannes Weiner, Mel Gorman,
linux-kernel, linux-mm
In-Reply-To: <alpine.DEB.2.10.1705021331450.116499@chino.kir.corp.google.com>
On Tue 02-05-17 13:41:23, David Rientjes wrote:
> On Tue, 2 May 2017, Michal Hocko wrote:
>
> > I have already asked and my questions were ignored. So let me ask again
> > and hopefuly not get ignored this time. So Why do we need a different
> > criterion on anon pages than file pages?
>
> The preference in get_scan_count() as already implemented is to reclaim
> from file pages if there is enough memory on the inactive list to reclaim.
> That is unchanged with this patch.
My fault, I was too vague. My question was basically why should we use
a different criterion to SCAN_ANON than SCAN_FILE.
> > I do agree that blindly
> > scanning anon pages when file pages are low is very suboptimal but this
> > adds yet another heuristic without _any_ numbers. Why cannot we simply
> > treat anon and file pages equally? Something like the following
> >
> > if (pgdatfile + pgdatanon + pgdatfree > 2*total_high_wmark) {
> > scan_balance = SCAN_FILE;
> > if (pgdatfile < pgdatanon)
> > scan_balance = SCAN_ANON;
> > goto out;
> > }
> >
>
> This would be substantially worse than the current code because it
> thrashes the anon lru when anon out numbers file pages rather than at the
> point we fall under the high watermarks for all eligible zones. If you
> tested your suggestion, you could see gigabytes of memory left untouched
> on the file lru. Anonymous memory is more probable to be part of the
> working set.
This was supposed to be more an example of a direction I was thinking,
definitely not a final patch. I will think more to come up with a
more complete proposal.
> > Also it would help to describe the workload which can trigger this
> > behavior so that we can compare numbers before and after this patch.
>
> Any workload that fills system RAM with anonymous memory that cannot be
> reclaimed will thrash the anon lru without this patch.
I have already asked, but I do not understand why this anon memory
couldn't be reclaimed. Who is pinning it? Why cannot it be swapped out?
If it is mlocked it should be moved to unevictable LRU. What am I
missing?
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox