* [PATCH 1/1] Remove hardcoding of ___GFP_xxx bitmasks
From: Igor Stoppa @ 2017-04-26 13:35 UTC (permalink / raw)
To: mhocko, namhyung; +Cc: linux-mm, linux-kernel, Igor Stoppa
In-Reply-To: <20170426133549.22603-1-igor.stoppa@huawei.com>
The bitmasks used for ___GFP_xxx can be defined in terms of an enum,
which doesn't require manual updates to its values.
As bonus, __GFP_BITS_SHIFT is automatically kept consistent.
Signed-off-by: Igor Stoppa <igor.stoppa@huawei.com>
---
include/linux/gfp.h | 82 +++++++++++++++++++++++++++++++++++------------------
1 file changed, 55 insertions(+), 27 deletions(-)
diff --git a/include/linux/gfp.h b/include/linux/gfp.h
index 0fe0b62..2f894c5 100644
--- a/include/linux/gfp.h
+++ b/include/linux/gfp.h
@@ -14,33 +14,62 @@ struct vm_area_struct;
* include/trace/events/mmflags.h and tools/perf/builtin-kmem.c
*/
+enum gfp_bitmask_shift {
+ __GFP_DMA_SHIFT = 0,
+ __GFP_HIGHMEM_SHIFT,
+ __GFP_DMA32_SHIFT,
+ __GFP_MOVABLE_SHIFT,
+ __GFP_RECLAIMABLE_SHIFT,
+ __GFP_HIGH_SHIFT,
+ __GFP_IO_SHIFT,
+ __GFP_FS_SHIFT,
+ __GFP_COLD_SHIFT,
+ __GFP_NOWARN_SHIFT,
+ __GFP_REPEAT_SHIFT,
+ __GFP_NOFAIL_SHIFT,
+ __GFP_NORETRY_SHIFT,
+ __GFP_MEMALLOC_SHIFT,
+ __GFP_COMP_SHIFT,
+ __GFP_ZERO_SHIFT,
+ __GFP_NOMEMALLOC_SHIFT,
+ __GFP_HARDWALL_SHIFT,
+ __GFP_THISNODE_SHIFT,
+ __GFP_ATOMIC_SHIFT,
+ __GFP_ACCOUNT_SHIFT,
+ __GFP_NOTRACK_SHIFT,
+ __GFP_DIRECT_RECLAIM_SHIFT,
+ __GFP_WRITE_SHIFT,
+ __GFP_KSWAPD_RECLAIM_SHIFT,
+ __GFP_BITS_SHIFT
+};
+
+
/* Plain integer GFP bitmasks. Do not use this directly. */
-#define ___GFP_DMA 0x01u
-#define ___GFP_HIGHMEM 0x02u
-#define ___GFP_DMA32 0x04u
-#define ___GFP_MOVABLE 0x08u
-#define ___GFP_RECLAIMABLE 0x10u
-#define ___GFP_HIGH 0x20u
-#define ___GFP_IO 0x40u
-#define ___GFP_FS 0x80u
-#define ___GFP_COLD 0x100u
-#define ___GFP_NOWARN 0x200u
-#define ___GFP_REPEAT 0x400u
-#define ___GFP_NOFAIL 0x800u
-#define ___GFP_NORETRY 0x1000u
-#define ___GFP_MEMALLOC 0x2000u
-#define ___GFP_COMP 0x4000u
-#define ___GFP_ZERO 0x8000u
-#define ___GFP_NOMEMALLOC 0x10000u
-#define ___GFP_HARDWALL 0x20000u
-#define ___GFP_THISNODE 0x40000u
-#define ___GFP_ATOMIC 0x80000u
-#define ___GFP_ACCOUNT 0x100000u
-#define ___GFP_NOTRACK 0x200000u
-#define ___GFP_DIRECT_RECLAIM 0x400000u
-#define ___GFP_WRITE 0x800000u
-#define ___GFP_KSWAPD_RECLAIM 0x1000000u
-/* If the above are modified, __GFP_BITS_SHIFT may need updating */
+#define ___GFP_DMA (1u << __GFP_DMA_SHIFT)
+#define ___GFP_HIGHMEM (1u << __GFP_HIGHMEM_SHIFT)
+#define ___GFP_DMA32 (1u << __GFP_DMA32_SHIFT)
+#define ___GFP_MOVABLE (1u << __GFP_MOVABLE_SHIFT)
+#define ___GFP_RECLAIMABLE (1u << __GFP_RECLAIMABLE_SHIFT)
+#define ___GFP_HIGH (1u << __GFP_HIGH_SHIFT)
+#define ___GFP_IO (1u << __GFP_IO_SHIFT)
+#define ___GFP_FS (1u << __GFP_FS_SHIFT)
+#define ___GFP_COLD (1u << __GFP_COLD_SHIFT)
+#define ___GFP_NOWARN (1u << __GFP_NOWARN_SHIFT)
+#define ___GFP_REPEAT (1u << __GFP_REPEAT_SHIFT)
+#define ___GFP_NOFAIL (1u << __GFP_NOFAIL_SHIFT)
+#define ___GFP_NORETRY (1u << __GFP_NORETRY_SHIFT)
+#define ___GFP_MEMALLOC (1u << __GFP_MEMALLOC_SHIFT)
+#define ___GFP_COMP (1u << __GFP_COMP_SHIFT)
+#define ___GFP_ZERO (1u << __GFP_ZERO_SHIFT)
+#define ___GFP_NOMEMALLOC (1u << __GFP_NOMEMALLOC_SHIFT)
+#define ___GFP_HARDWALL (1u << __GFP_HARDWALL_SHIFT)
+#define ___GFP_THISNODE (1u << __GFP_THISNODE_SHIFT)
+#define ___GFP_ATOMIC (1u << __GFP_ATOMIC_SHIFT)
+#define ___GFP_ACCOUNT (1u << __GFP_ACCOUNT_SHIFT)
+#define ___GFP_NOTRACK (1u << __GFP_NOTRACK_SHIFT)
+#define ___GFP_DIRECT_RECLAIM (1u << __GFP_DIRECT_RECLAIM_SHIFT)
+#define ___GFP_WRITE (1u << __GFP_WRITE_SHIFT)
+#define ___GFP_KSWAPD_RECLAIM (1u << __GFP_KSWAPD_RECLAIM_SHIFT)
/*
* Physical address zone modifiers (see linux/mmzone.h - low four bits)
@@ -180,7 +209,6 @@ struct vm_area_struct;
#define __GFP_NOTRACK_FALSE_POSITIVE (__GFP_NOTRACK)
/* Room for N __GFP_FOO bits */
-#define __GFP_BITS_SHIFT 25
#define __GFP_BITS_MASK ((__force gfp_t)((1 << __GFP_BITS_SHIFT) - 1))
/*
--
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 related
* [PATCH 0/1] mm: Improve consistency of ___GFP_xxx masks
From: Igor Stoppa @ 2017-04-26 13:35 UTC (permalink / raw)
To: mhocko, namhyung; +Cc: linux-mm, linux-kernel, Igor Stoppa
The GFP bitmasks and the __GFP_BITS_SHIFT defines are expressed as
hardcoded constants.
This can be expressed in a more consistent way by relying on an enum of
shift positions.
Igor Stoppa (1):
Remove hardcoding of ___GFP_xxx bitmasks
include/linux/gfp.h | 82 +++++++++++++++++++++++++++++++++++------------------
1 file changed, 55 insertions(+), 27 deletions(-)
--
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: [PATCH -mm -v3] mm, swap: Sort swap entries before free
From: Huang, Ying @ 2017-04-26 12:42 UTC (permalink / raw)
To: Minchan Kim
Cc: Huang, Ying, Andrew Morton, linux-mm, linux-kernel, Hugh Dickins,
Shaohua Li, Rik van Riel
In-Reply-To: <20170424045213.GA11287@bbox>
Minchan Kim <minchan@kernel.org> writes:
> On Fri, Apr 21, 2017 at 08:29:30PM +0800, Huang, Ying wrote:
>> "Huang, Ying" <ying.huang@intel.com> writes:
>>
>> > Minchan Kim <minchan@kernel.org> writes:
>> >
>> >> On Wed, Apr 19, 2017 at 04:14:43PM +0800, Huang, Ying wrote:
>> >>> Minchan Kim <minchan@kernel.org> writes:
>> >>>
>> >>> > Hi Huang,
>> >>> >
>> >>> > On Fri, Apr 07, 2017 at 02:49:01PM +0800, Huang, Ying wrote:
>> >>> >> From: Huang Ying <ying.huang@intel.com>
>> >>> >>
>> >>> >> void swapcache_free_entries(swp_entry_t *entries, int n)
>> >>> >> {
>> >>> >> struct swap_info_struct *p, *prev;
>> >>> >> @@ -1075,6 +1083,10 @@ void swapcache_free_entries(swp_entry_t *entries, int n)
>> >>> >>
>> >>> >> prev = NULL;
>> >>> >> p = NULL;
>> >>> >> +
>> >>> >> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> >>> >> + if (nr_swapfiles > 1)
>> >>> >> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> >>> >
>> >>> > Let's think on other cases.
>> >>> >
>> >>> > There are two swaps and they are configured by priority so a swap's usage
>> >>> > would be zero unless other swap used up. In case of that, this sorting
>> >>> > is pointless.
>> >>> >
>> >>> > As well, nr_swapfiles is never decreased so if we enable multiple
>> >>> > swaps and then disable until a swap is remained, this sorting is
>> >>> > pointelss, too.
>> >>> >
>> >>> > How about lazy sorting approach? IOW, if we found prev != p and,
>> >>> > then we can sort it.
>> >>>
>> >>> Yes. That should be better. I just don't know whether the added
>> >>> complexity is necessary, given the array is short and sort is fast.
>> >>
>> >> Huh?
>> >>
>> >> 1. swapon /dev/XXX1
>> >> 2. swapon /dev/XXX2
>> >> 3. swapoff /dev/XXX2
>> >> 4. use only one swap
>> >> 5. then, always pointless sort.
>> >
>> > Yes. In this situation we will do unnecessary sorting. What I don't
>> > know is whether the unnecessary sorting will hurt performance in real
>> > life. I can do some measurement.
>>
>> I tested the patch with 1 swap device and 1 process to eat memory
>> (remove the "if (nr_swapfiles > 1)" for test). I think this is the
>> worse case because there is no lock contention. The memory freeing time
>> increased from 1.94s to 2.12s (increase ~9.2%). So there is some
>> overhead for some cases. I change the algorithm to something like
>> below,
>>
>> void swapcache_free_entries(swp_entry_t *entries, int n)
>> {
>> struct swap_info_struct *p, *prev;
>> int i;
>> + swp_entry_t entry;
>> + unsigned int prev_swp_type;
>>
>> if (n <= 0)
>> return;
>>
>> + prev_swp_type = swp_type(entries[0]);
>> + for (i = n - 1; i > 0; i--) {
>> + if (swp_type(entries[i]) != prev_swp_type)
>> + break;
>> + }
>
> That's really what I want to avoid. For many swap usecases,
> it adds unnecessary overhead.
>
>> +
>> + /* Sort swap entries by swap device, so each lock is only taken once. */
>> + if (i)
>> + sort(entries, n, sizeof(entries[0]), swp_entry_cmp, NULL);
>> prev = NULL;
>> p = NULL;
>> for (i = 0; i < n; ++i) {
>> - p = swap_info_get_cont(entries[i], prev);
>> + entry = entries[i];
>> + p = swap_info_get_cont(entry, prev);
>> if (p)
>> - swap_entry_free(p, entries[i]);
>> + swap_entry_free(p, entry);
>> prev = p;
>> }
>> if (p)
>>
>> With this patch, the memory freeing time increased from 1.94s to 1.97s.
>> I think this is good enough. Do you think so?
>
> What I mean is as follows(I didn't test it at all):
>
> With this, sort entries if we found multiple entries in current
> entries. It adds some condition checks for non-multiple swap
> usecase but it would be more cheaper than the sorting.
> And it adds a [un]lock overhead for multiple swap usecase but
> it should be a compromise for single-swap usecase which is more
> popular.
>
How about the following solution? It can avoid [un]lock overhead and
double lock issue for multiple swap user case and has good performance
for one swap user case too.
Best Regards,
Huang, Ying
^ permalink raw reply
* RE: [virtio-dev] Re: [PATCH v9 2/5] virtio-balloon: VIRTIO_BALLOON_F_BALLOON_CHUNKS
From: Wang, Wei W @ 2017-04-26 11:03 UTC (permalink / raw)
To: Wang, Wei W, Michael S. Tsirkin
Cc: virtio-dev@lists.oasis-open.org, linux-kernel@vger.kernel.org,
qemu-devel@nongnu.org, virtualization@lists.linux-foundation.org,
kvm@vger.kernel.org, linux-mm@kvack.org, david@redhat.com,
Hansen, Dave, cornelia.huck@de.ibm.com, akpm@linux-foundation.org,
mgorman@techsingularity.net, aarcange@redhat.com,
amit.shah@redhat.com, pbonzini@redhat.com,
liliang.opensource@gmail.com
In-Reply-To: <58F43801.7060004@intel.com>
Hi Michael, could you please give some feedback?
On Monday, April 17, 2017 11:35 AM, Wei Wang wrote:
> On 04/15/2017 05:38 AM, Michael S. Tsirkin wrote:
> > On Fri, Apr 14, 2017 at 04:37:52PM +0800, Wei Wang wrote:
> >> On 04/14/2017 12:34 AM, Michael S. Tsirkin wrote:
> >>> On Thu, Apr 13, 2017 at 05:35:05PM +0800, Wei Wang wrote:
> >>>
> >>> So we don't need the bitmap to talk to host, it is just a data
> >>> structure we chose to maintain lists of pages, right?
> >> Right. bitmap is the way to gather pages to chunk.
> >> It's only needed in the balloon page case.
> >> For the unused page case, we don't need it, since the free page
> >> blocks are already chunks.
> >>
> >>> OK as far as it goes but you need much better isolation for it.
> >>> Build a data structure with APIs such as _init, _cleanup, _add,
> >>> _clear, _find_first, _find_next.
> >>> Completely unrelated to pages, it just maintains bits.
> >>> Then use it here.
> >>>
> >>>
> >>>> static int oom_pages = OOM_VBALLOON_DEFAULT_PAGES;
> >>>> module_param(oom_pages, int, S_IRUSR | S_IWUSR);
> >>>> MODULE_PARM_DESC(oom_pages, "pages to free on OOM"); @@ -50,6
> >>>> +54,10 @@ MODULE_PARM_DESC(oom_pages, "pages to free on OOM");
> >>>> static struct vfsmount *balloon_mnt;
> >>>> #endif
> >>>> +/* Types of pages to chunk */
> >>>> +#define PAGE_CHUNK_TYPE_BALLOON 0
> >>>> +
> >>> Doesn't look like you are ever adding more types in this patchset.
> >>> Pls keep code simple, generalize it later.
> >>>
> >> "#define PAGE_CHUNK_TYPE_UNUSED 1" is added in another patch.
> > I would say add the extra code there too. Or maybe we can avoid adding
> > it altogether.
>
> I'm trying to have the two features( i.e. "balloon pages" and "unused pages")
> decoupled while trying to use common functions to deal with the commonalities.
> That's the reason to define the above macro.
> Without the macro, we will need to have separate functions, for example,
> instead of one "add_one_chunk()", we need to have
> add_one_balloon_page_chunk() and add_one_unused_page_chunk(), and some
> of the implementations will be kind of duplicate in the two functions.
> Probably we can add it when the second feature comes to the code.
>
> >
> >> Types of page to chunk are treated differently. Different types of
> >> page chunks are sent to the host via different protocols.
> >>
> >> 1) PAGE_CHUNK_TYPE_BALLOON: Ballooned (i.e. inflated/deflated) pages
> >> to chunk. For the ballooned type, it uses the basic chunk msg format:
> >>
> >> virtio_balloon_page_chunk_hdr +
> >> virtio_balloon_page_chunk * MAX_PAGE_CHUNKS
> >>
> >> 2) PAGE_CHUNK_TYPE_UNUSED: unused pages to chunk. It uses this miscq
> >> msg
> >> format:
> >> miscq_hdr +
> >> virtio_balloon_page_chunk_hdr +
> >> virtio_balloon_page_chunk * MAX_PAGE_CHUNKS
> >>
> >> The chunk msg is actually the payload of the miscq msg.
> >>
> >>
> > So just combine the two message formats and then it'll all be easier?
> >
>
> Yes, it'll be simple with only one msg format. But the problem I see here is that
> miscq hdr is something necessary for the "unused page"
> usage, but not needed by the "balloon page" usage. To be more precise, struct
> virtio_balloon_miscq_hdr {
> __le16 cmd;
> __le16 flags;
> };
> 'cmd' specifies the command from the miscq (I envision that miscq will be
> further used to handle other possible miscellaneous requests either from the
> host or to the host), so 'cmd' is necessary for the miscq. But the inflateq is
> exclusively used for inflating pages, so adding a command to it would be
> redundant and look a little bewildered there.
> 'flags': We currently use bit 0 of flags to indicate the completion ofa command,
> this is also useful in the "unused page" usage, and not needed by the "balloon
> page" usage.
> >>>> +#define MAX_PAGE_CHUNKS 4096
> >>> This is an order-4 allocation. I'd make it 4095 and then it's an
> >>> order-3 one.
> >> Sounds good, thanks.
> >> I think it would be better to make it 4090. Leave some space for the
> >> hdr as well.
> > And miscq hdr. In fact just let compiler do the math - something like:
> > (8 * PAGE_SIZE - sizeof(hdr)) / sizeof(chunk)
> Agree, thanks.
>
> >
> > I skimmed explanation of algorithms below but please make sure code
> > speaks for itself and add comments inline to document it.
> > Whenever you answered me inline this is where you want to try to make
> > code clearer and add comments.
> >
> > Also, pls find ways to abstract the data structure so we don't need to
> > deal with its internals all over the code.
> >
> >
> > ....
> >
> >>>> {
> >>>> struct scatterlist sg;
> >>>> + struct virtio_balloon_page_chunk_hdr *hdr;
> >>>> + void *buf;
> >>>> unsigned int len;
> >>>> - sg_init_one(&sg, vb->pfns, sizeof(vb->pfns[0]) * vb->num_pfns);
> >>>> + switch (type) {
> >>>> + case PAGE_CHUNK_TYPE_BALLOON:
> >>>> + hdr = vb->balloon_page_chunk_hdr;
> >>>> + len = 0;
> >>>> + break;
> >>>> + default:
> >>>> + dev_warn(&vb->vdev->dev, "%s: chunk %d of unknown
> pages\n",
> >>>> + __func__, type);
> >>>> + return;
> >>>> + }
> >>>> - /* We should always be able to add one buffer to an empty queue. */
> >>>> - virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> >>>> - virtqueue_kick(vq);
> >>>> + buf = (void *)hdr - len;
> >>> Moving back to before the header? How can this make sense?
> >>> It works fine since len is 0, so just buf = hdr.
> >>>
> >> For the unused page chunk case, it follows its own protocol:
> >> miscq_hdr + payload(chunk msg).
> >> "buf = (void *)hdr - len" moves the buf pointer to the miscq_hdr,
> >> to send the entire miscq msg.
> > Well just pass the correct pointer in.
> >
> OK. The miscq msg is
> {
> miscq_hdr;
> chunk_msg;
> }
>
> We can probably change the code like this:
>
> #define CHUNK_TO_MISCQ_MSG(chunk) (chunk - sizeof(struct
> virtio_balloon_miscq_hdr))
>
> switch (type) {
> case PAGE_CHUNK_TYPE_BALLOON:
> msg_buf = vb->balloon_page_chunk_hdr;
> msg_len = sizeof(struct virtio_balloon_page_chunk_hdr) +
> nr_chunks * sizeof(struct virtio_balloon_page_chunk_entry);
> break;
> case PAGE_CHUNK_TYPE_UNUSED:
> msg_buf = CHUNK_TO_MISCQ_MSG(vb->unused_page_chunk_hdr);
> msg_len = sizeof(struct virtio_balloon_miscq_hdr) + sizeof(struct
> virtio_balloon_page_chunk_hdr) +
> nr_chunks * sizeof(struct virtio_balloon_page_chunk_entry);
> break;
> default:
> dev_warn(&vb->vdev->dev, "%s: chunk %d of unknown pages\n",
> __func__, type);
> return;
> }
>
>
>
> >> Please check the patch for implementing the unused page chunk, it
> >> will be clear. If necessary, I can put "buf = (void *)hdr - len" from
> >> that patch.
> > Exactly. And all this pointer math is very messy. Please look for ways
> > to clean it. It's generally easy to fill structures:
> >
> > struct foo *foo = kmalloc(..., sizeof(*foo) + n * sizeof(foo->a[0]));
> > for (i = 0; i < n; ++i)
> > foo->a[i] = b;
> >
> > this is the kind of code that's easy to understand and it's obvious
> > there are no overflows and no info leaks here.
> >
> OK, will take your suggestion:
>
> struct virtio_balloon_page_chunk {
> struct virtio_balloon_page_chunk_hdr hdr;
> struct virtio_balloon_page_chunk_entry entries[]; };
>
>
> >>>> + len += sizeof(struct virtio_balloon_page_chunk_hdr);
> >>>> + len += hdr->chunks * sizeof(struct virtio_balloon_page_chunk);
> >>>> + sg_init_table(&sg, 1);
> >>>> + sg_set_buf(&sg, buf, len);
> >>>> + if (!virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL)) {
> >>>> + virtqueue_kick(vq);
> >>>> + if (busy_wait)
> >>>> + while (!virtqueue_get_buf(vq, &len) &&
> >>>> + !virtqueue_is_broken(vq))
> >>>> + cpu_relax();
> >>>> + else
> >>>> + wait_event(vb->acked, virtqueue_get_buf(vq, &len));
> >>>> + hdr->chunks = 0;
> >>> Why zero it here after device used it? Better to zero before use.
> >> hdr->chunks tells the host how many chunks are there in the payload.
> >> After the device use it, it is ready to zero it.
> > It's rather confusing. Try to pass # of chunks around in some other
> > way.
>
> Not sure if this was explained clearly - we just let the chunk msg hdr indicates
> the # of chunks in the payload. I think this should be a pretty normal usage, like
> the network UDP hdr, which uses a length field to indicate the packet length.
>
> >>>> + }
> >>>> +}
> >>>> +
> >>>> +static void add_one_chunk(struct virtio_balloon *vb, struct virtqueue *vq,
> >>>> + int type, u64 base, u64 size)
> >>> what are the units here? Looks like it's in 4kbyte units?
> >> what is the "unit" you referred to?
> >> This is the function to add one chunk, base pfn and size of the chunk
> >> are supplied to the function.
> >>
> > Are both size and base in bytes then?
> > But you do not send them to host as is, you shift them for some reason
> > before sending them to host.
> >
> Not in bytes actually. base is a base pfn, which is the starting address of the
> continuous pfns. Size is the chunk size, which is the number of continuous pfns.
>
> They are shifted based on the chunk format we agreed before:
>
> --------------------------------------------------------
> | Base (52 bit) | Rsvd (12 bit) |
> --------------------------------------------------------
> --------------------------------------------------------
> | Size (52 bit) | Rsvd (12 bit) |
> --------------------------------------------------------
>
>
> Here, the pfn will be the balloon page pfn (4KB).In this way, the host doesn't
> need to know PAGE_SIZE of the guest.
>
>
>
> >>>> + if (zero >= end)
> >>>> + chunk_size = end - one;
> >>>> + else
> >>>> + chunk_size = zero - one;
> >>>> +
> >>>> + if (chunk_size)
> >>>> + add_one_chunk(vb, vq,
> PAGE_CHUNK_TYPE_BALLOON,
> >>>> + pfn_start + one, chunk_size);
> >>> Still not so what does a bit refer to? page or 4kbytes?
> >>> I think it should be a page.
> >> A bit in the bitmap corresponds to a pfn of a balloon page(4KB).
> > That's a waste on systems with large page sizes, and it does not look
> > like you handle that case correctly.
>
> OK, I will change the bitmap to be PAGE_SIZE based here, instead of
> BALLOON_PAGE_SIZE based. When convert them into chunks, making it based
> on BALLOON_PAGE_SIZE.
>
>
> Best,
> Wei
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
--
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: your mail
From: Michal Hocko @ 2017-04-26 9:19 UTC (permalink / raw)
To: Joonsoo Kim
Cc: linux-mm, Andrew Morton, Mel Gorman, Vlastimil Babka,
Andrea Arcangeli, Jerome Glisse, Reza Arbab, Yasuaki Ishimatsu,
qiuxishi, Kani Toshimitsu, slaoub, Andi Kleen, David Rientjes,
Daniel Kiper, Igor Mammedov, Vitaly Kuznetsov, LKML
In-Reply-To: <20170425025043.GA32583@js1304-desktop>
On Tue 25-04-17 11:50:45, Joonsoo Kim wrote:
> On Mon, Apr 24, 2017 at 09:53:12AM +0200, Michal Hocko wrote:
> > On Mon 24-04-17 10:44:43, Joonsoo Kim wrote:
> > > On Fri, Apr 21, 2017 at 09:16:16AM +0200, Michal Hocko wrote:
> > > > On Fri 21-04-17 13:38:28, Joonsoo Kim wrote:
> > > > > On Thu, Apr 20, 2017 at 09:28:20AM +0200, Michal Hocko wrote:
> > > > > > On Thu 20-04-17 10:27:55, Joonsoo Kim wrote:
> > > > > > > On Mon, Apr 17, 2017 at 10:15:15AM +0200, Michal Hocko wrote:
> > > > > > [...]
> > > > > > > > Which pfn walkers you have in mind?
> > > > > > >
> > > > > > > For example, kpagecount_read() in fs/proc/page.c. I searched it by
> > > > > > > using pfn_valid().
> > > > > >
> > > > > > Yeah, I've checked that one and in fact this is a good example of the
> > > > > > case where you do not really care about holes. It just checks the page
> > > > > > count which is a valid information under any circumstances.
> > > > >
> > > > > I don't think so. First, it checks the page *map* count. Is it still valid
> > > > > even if PageReserved() is set?
> > > >
> > > > I do not know about any user which would manipulate page map count for
> > > > referenced pages. The core MM code doesn't.
> > >
> > > That's weird that we can get *map* count without PageReserved() check,
> > > but we cannot get zone information.
> > > Zone information is more static information than map count.
> >
> > As I've already pointed out the rework of the hotplug code is mainly
> > about postponing the zone initialization from the physical hot add to
> > the logical onlining. The zone is really not clear until that moment.
> >
> > > It should be defined/documented in this time that what information in
> > > the struct page is valid even if PageReserved() is set. And then, we
> > > need to fix all the things based on this design decision.
> >
> > Where would you suggest documenting this? We do have
> > Documentation/memory-hotplug.txt but it is not really specific about
> > struct page.
>
> pfn_valid() in include/linux/mmzone.h looks proper place.
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index c412e6a3a1e9..443258fcac93 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -1288,10 +1288,14 @@ unsigned long __init node_memmap_size_bytes(int, unsigned long, unsigned long);
#ifdef CONFIG_ARCH_HAS_HOLES_MEMORYMODEL
/*
* pfn_valid() is meant to be able to tell if a given PFN has valid memmap
- * associated with it or not. In FLATMEM, it is expected that holes always
- * have valid memmap as long as there is valid PFNs either side of the hole.
- * In SPARSEMEM, it is assumed that a valid section has a memmap for the
- * entire section.
+ * associated with it or not. This means that a struct page exists for this
+ * pfn. The caller cannot assume the page is fully initialized though.
+ * pfn_to_online_page() should be used to make sure the struct page is fully
+ * initialized.
+ *
+ * In FLATMEM, it is expected that holes always have valid memmap as long as
+ * there is valid PFNs either side of the hole. In SPARSEMEM, it is assumed
+ * that a valid section has a memmap for the entire section.
*
* However, an ARM, and maybe other embedded architectures in the future
* free memmap backing holes to save memory on the assumption the memmap is
> > [...]
> >
> > > > You are trying to change a semantic of something that has a well defined
> > > > meaning. I disagree that we should change it. It might sound like a
> > > > simpler thing to do because pfn walkers will have to be checked but what
> > > > you are proposing is conflating two different things together.
> > >
> > > I don't think that *I* try to change the semantic of pfn_valid().
> > > It would be original semantic of pfn_valid().
> > >
> > > "If pfn_valid() returns true, we can get proper struct page and the
> > > zone information,"
> >
> > I do not see any guarantee about the zone information anywhere. In fact
> > this is not true with the original implementation as I've tried to
> > explain already. We do have new pages associated with a zone but that
> > association might change during the online phase. So you cannot really
> > rely on that information until the page is online. There is no real
> > change in that regards after my rework.
>
> I know that what you did doesn't change thing much. What I try to say
> is that previous implementation related to pfn_valid() in hotplug is
> wrong. Please do not assume that hotplug implementation is correct and
> other pfn_valid() users are incorrect. There is no design document so
> I'm not sure which one is correct but assumption that pfn_valid() user
> can access whole the struct page information makes much sense to me.
Not really. E.g. ZONE_DEVICE pages are never online AFAIK. I believe we
still need pfn_valid to work for those pfns. Really, pfn_valid has a
different meaning than you would like it to have. Who knows how many
others like that are lurking there. I feel much more comfortable to go
and hunt already broken code and fix it rathert than break something
unexpectedly.
--
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
* [PATCH v1 2/2] mm: hwpoison: call shake_page() after try_to_unmap() for mlocked page
From: Naoya Horiguchi @ 2017-04-26 9:10 UTC (permalink / raw)
To: linux-mm
Cc: xiaolong.ye, Andrew Morton, Chen Gong, lkp, linux-kernel,
Naoya Horiguchi, Naoya Horiguchi
In-Reply-To: <1493197841-23986-1-git-send-email-n-horiguchi@ah.jp.nec.com>
Memory error handler calls try_to_unmap() for error pages in various
states. If the error page is a mlocked page, error handling could fail
with "still referenced by 1 users" message. This is because the page
is linked to and stays in lru cache after the following call chain.
try_to_unmap_one
page_remove_rmap
clear_page_mlock
putback_lru_page
lru_cache_add
memory_failure() calls shake_page() to hanlde the similar issue, but
current code doesn't cover because shake_page() is called only before
try_to_unmap(). So this patches adds shake_page().
Link: http://lkml.kernel.org/r/20170417055948.GM31394@yexl-desktop
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
mm/memory-failure.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git v4.11-rc6-mmotm-2017-04-13-14-50/mm/memory-failure.c v4.11-rc6-mmotm-2017-04-13-14-50_patched/mm/memory-failure.c
index 77cf9c3..57f07ec 100644
--- v4.11-rc6-mmotm-2017-04-13-14-50/mm/memory-failure.c
+++ v4.11-rc6-mmotm-2017-04-13-14-50_patched/mm/memory-failure.c
@@ -919,6 +919,7 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
bool unmap_success;
int kill = 1, forcekill;
struct page *hpage = *hpagep;
+ bool mlocked = PageMlocked(hpage);
/*
* Here we are interested only in user-mapped pages, so skip any
@@ -983,6 +984,13 @@ static bool hwpoison_user_mappings(struct page *p, unsigned long pfn,
pfn, page_mapcount(hpage));
/*
+ * try_to_unmap() might put mlocked page in lru cache, so call
+ * shake_page() again to ensure that it's flushed.
+ */
+ if (mlocked)
+ shake_page(hpage, 0);
+
+ /*
* Now that the dirty bit has been propagated to the
* struct page and all unmaps done we can decide if
* killing is needed or not. Only kill when the page
--
2.7.0
--
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
* [PATCH v1 1/2] mm: hwpoison: call shake_page() unconditionally
From: Naoya Horiguchi @ 2017-04-26 9:10 UTC (permalink / raw)
To: linux-mm
Cc: xiaolong.ye, Andrew Morton, Chen Gong, lkp, linux-kernel,
Naoya Horiguchi, Naoya Horiguchi
In-Reply-To: <1493197841-23986-1-git-send-email-n-horiguchi@ah.jp.nec.com>
shake_page() is called before going into core error handling code in order
to ensure that the error page is flushed from lru_cache lists where pages
stay during transferring among LRU lists.
But currently it's not fully functional because when the page is linked to
lru_cache by calling activate_page(), its PageLRU flag is set and
shake_page() is skipped. The result is to fail error handling with "still
referenced by 1 users" message.
When the page is linked to lru_cache by isolate_lru_page(), its PageLRU is
clear, so that's fine.
This patch makes shake_page() unconditionally called to avoild the failure.
Link: http://lkml.kernel.org/r/20170417055948.GM31394@yexl-desktop
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Naoya Horiguchi <n-horiguchi@ah.jp.nec.com>
---
mm/hwpoison-inject.c | 3 +--
mm/memory-failure.c | 27 +++++++++++----------------
2 files changed, 12 insertions(+), 18 deletions(-)
diff --git v4.11-rc6-mmotm-2017-04-13-14-50/mm/hwpoison-inject.c v4.11-rc6-mmotm-2017-04-13-14-50_patched/mm/hwpoison-inject.c
index 9d26fd9..356df05 100644
--- v4.11-rc6-mmotm-2017-04-13-14-50/mm/hwpoison-inject.c
+++ v4.11-rc6-mmotm-2017-04-13-14-50_patched/mm/hwpoison-inject.c
@@ -34,8 +34,7 @@ static int hwpoison_inject(void *data, u64 val)
if (!hwpoison_filter_enable)
goto inject;
- if (!PageLRU(hpage) && !PageHuge(p))
- shake_page(hpage, 0);
+ shake_page(hpage, 0);
/*
* This implies unable to support non-LRU pages.
*/
diff --git v4.11-rc6-mmotm-2017-04-13-14-50/mm/memory-failure.c v4.11-rc6-mmotm-2017-04-13-14-50_patched/mm/memory-failure.c
index 8c02811..77cf9c3 100644
--- v4.11-rc6-mmotm-2017-04-13-14-50/mm/memory-failure.c
+++ v4.11-rc6-mmotm-2017-04-13-14-50_patched/mm/memory-failure.c
@@ -220,6 +220,9 @@ static int kill_proc(struct task_struct *t, unsigned long addr, int trapno,
*/
void shake_page(struct page *p, int access)
{
+ if (PageHuge(p))
+ return;
+
if (!PageSlab(p)) {
lru_add_drain_all();
if (PageLRU(p))
@@ -1140,22 +1143,14 @@ int memory_failure(unsigned long pfn, int trapno, int flags)
* The check (unnecessarily) ignores LRU pages being isolated and
* walked by the page reclaim code, however that's not a big loss.
*/
- if (!PageHuge(p)) {
- if (!PageLRU(p))
- shake_page(p, 0);
- if (!PageLRU(p)) {
- /*
- * shake_page could have turned it free.
- */
- if (is_free_buddy_page(p)) {
- if (flags & MF_COUNT_INCREASED)
- action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
- else
- action_result(pfn, MF_MSG_BUDDY_2ND,
- MF_DELAYED);
- return 0;
- }
- }
+ shake_page(p, 0);
+ /* shake_page could have turned it free. */
+ if (!PageLRU(p) && is_free_buddy_page(p)) {
+ if (flags & MF_COUNT_INCREASED)
+ action_result(pfn, MF_MSG_BUDDY, MF_DELAYED);
+ else
+ action_result(pfn, MF_MSG_BUDDY_2ND, MF_DELAYED);
+ return 0;
}
lock_page(hpage);
--
2.7.0
--
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
* [PATCH v1 0/2] mm: hwpoison: fix mce-test.ras.fail on 23a003bfd2
From: Naoya Horiguchi @ 2017-04-26 9:10 UTC (permalink / raw)
To: linux-mm
Cc: xiaolong.ye, Andrew Morton, Chen Gong, lkp, linux-kernel,
Naoya Horiguchi, Naoya Horiguchi
Hello,
I wrote fixes for the problems reported by http://lkml.kernel.org/r/20170417055948.GM31394@yexl-desktop.
These 2 patches make tsimpleinj.c delivered in mce-test test suite pass.
[build8:~/mce-test-official/cases/function/hwpoison]$ ./tsimpleinj
dirty page 0x7f657aecb000
signal 7 code 4 addr 0x7f657aecb000
recovered
mlocked page 0x7f657aeca000
signal 7 code 4 addr 0x7f657aeca000
recovered
clean file page 0x7f657aec9000
23
recovered
file dirty page 0x7f657aec8000
signal 7 code 4 addr 0x7f657aec8000
recovered
no error on msync expect error
no error on fsync expect error
hole file dirty page 0x7f657aec7000
signal 7 code 4 addr 0x7f657aec7000
recovered
no error on hole msync expect error
no error on hole fsync expect error
SUCCESS
I'm digging another similar issue for hugetlb pages, which need some more
research and code, so I'll send it separately later.
Thanks,
Naoya Horiguchi
---
Summary:
Naoya Horiguchi (2):
mm: hwpoison: call shake_page() unconditionally
mm: hwpoison: call shake_page() after try_to_unmap() for mlocked page
mm/hwpoison-inject.c | 3 +--
mm/memory-failure.c | 35 +++++++++++++++++++----------------
2 files changed, 20 insertions(+), 18 deletions(-)
--
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 2/2] dax: add regression test for stale mmap reads
From: Xiong Zhou @ 2017-04-26 9:09 UTC (permalink / raw)
To: Ross Zwisler
Cc: fstests, Xiong Zhou, jmoyer, eguan, Christoph Hellwig,
Dan Williams, Darrick J. Wong, Jan Kara, linux-fsdevel, linux-mm,
linux-nvdimm, Andrew Morton
In-Reply-To: <20170425205106.20576-2-ross.zwisler@linux.intel.com>
On Tue, Apr 25, 2017 at 02:51:06PM -0600, Ross Zwisler wrote:
> This adds a regression test for the following kernel patch:
>
> dax: fix data corruption due to stale mmap reads
>
> The above patch fixes an issue where users of DAX can suffer data
> corruption from stale mmap reads via the following sequence:
>
> - open an mmap over a 2MiB hole
>
> - read from a 2MiB hole, faulting in a 2MiB zero page
>
> - write to the hole with write(3p). The write succeeds but we incorrectly
> leave the 2MiB zero page mapping intact.
>
> - via the mmap, read the data that was just written. Since the zero page
> mapping is still intact we read back zeroes instead of the new data.
>
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> ---
> .gitignore | 1 +
> src/Makefile | 2 +-
> src/t_dax_stale_pmd.c | 59 +++++++++++++++++++++++++++++++++++++++++++++
> tests/generic/427 | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++
> tests/generic/427.out | 2 ++
> tests/generic/group | 1 +
> 6 files changed, 131 insertions(+), 1 deletion(-)
> create mode 100644 src/t_dax_stale_pmd.c
> create mode 100755 tests/generic/427
> create mode 100644 tests/generic/427.out
>
> diff --git a/.gitignore b/.gitignore
> index ded4a61..9664dc9 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -134,6 +134,7 @@
> /src/renameat2
> /src/t_rename_overwrite
> /src/t_mmap_dio
> +/src/t_dax_stale_pmd
>
> # dmapi/ binaries
> /dmapi/src/common/cmd/read_invis
> diff --git a/src/Makefile b/src/Makefile
> index abfd873..7e22b50 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -12,7 +12,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
> godown resvtest writemod makeextents itrash rename \
> multi_open_unlink dmiperf unwritten_sync genhashnames t_holes \
> t_mmap_writev t_truncate_cmtime dirhash_collide t_rename_overwrite \
> - holetest t_truncate_self t_mmap_dio af_unix
> + holetest t_truncate_self t_mmap_dio af_unix t_dax_stale_pmd
>
> LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> diff --git a/src/t_dax_stale_pmd.c b/src/t_dax_stale_pmd.c
> new file mode 100644
> index 0000000..59fbbe1
> --- /dev/null
> +++ b/src/t_dax_stale_pmd.c
> @@ -0,0 +1,59 @@
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <libgen.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#define MiB(a) ((a)*1024*1024)
> +
> +void err_exit(char *op)
> +{
> + fprintf(stderr, "%s: %s\n", op, strerror(errno));
> + exit(1);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + volatile int a __attribute__((__unused__));
> + char *buffer = "HELLO WORLD!";
> + char *data;
> + int fd;
> +
> + if (argc < 2) {
> + printf("Usage: %s <pmem file>\n", basename(argv[0]));
> + exit(0);
> + }
> +
> + fd = open(argv[1], O_RDWR);
> + if (fd < 0)
> + err_exit("fd");
> +
> + data = mmap(NULL, MiB(2), PROT_READ, MAP_SHARED, fd, MiB(2));
> +
> + /*
> + * This faults in a 2MiB zero page to satisfy the read.
> + * 'a' is volatile so this read doesn't get optimized out.
> + */
> + a = data[0];
> +
> + pwrite(fd, buffer, strlen(buffer), MiB(2));
> +
> + /*
> + * Try and use the mmap to read back the data we just wrote with
> + * pwrite(). If the kernel bug is present the mapping from the 2MiB
> + * zero page will still be intact, and we'll read back zeros instead.
> + */
> + if (strncmp(buffer, data, strlen(buffer))) {
> + fprintf(stderr, "strncmp mismatch: '%s' vs '%s'\n", buffer,
> + data);
munmap
close(fd);
> + exit(1);
> + }
> +
munmap
Thanks, :)
--
Xiong
> + close(fd);
> + return 0;
> +}
> diff --git a/tests/generic/427 b/tests/generic/427
> new file mode 100755
> index 0000000..6e265a1
> --- /dev/null
> +++ b/tests/generic/427
> @@ -0,0 +1,67 @@
> +#! /bin/bash
> +# FS QA Test 427
> +#
> +# This is a regression test for kernel patch:
> +# dax: fix data corruption due to stale mmap reads
> +# created by Ross Zwisler <ross.zwisler@linux.intel.com>
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 Intel Corporation. All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> + cd /
> + rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_supported_os Linux
> +_require_test_program "t_dax_stale_pmd"
> +_require_xfs_io_command "falloc"
> +_require_user
> +
> +# real QA test starts here
> +
> +# ensure we have no pre-existing block allocations, so we get a hole
> +rm -f $TEST_DIR/testfile
> +$XFS_IO_PROG -f -c "falloc 0 4M" $TEST_DIR/testfile >> $seqres.full 2>&1
> +chmod 0644 $TEST_DIR/testfile
> +
> +src/t_dax_stale_pmd $TEST_DIR/testfile
> +
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/427.out b/tests/generic/427.out
> new file mode 100644
> index 0000000..61295e5
> --- /dev/null
> +++ b/tests/generic/427.out
> @@ -0,0 +1,2 @@
> +QA output created by 427
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index f29009c..06f6e9d 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -429,3 +429,4 @@
> 424 auto quick
> 425 auto quick attr
> 426 auto quick exportfs
> +427 auto quick
> --
> 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: [PATCH v2 1/2] mm: Uncharge poisoned pages
From: Balbir Singh @ 2017-04-26 8:59 UTC (permalink / raw)
To: Naoya Horiguchi
Cc: Laurent Dufour, linux-kernel@vger.kernel.org, linux-mm@kvack.org,
akpm@linux-foundation.org
In-Reply-To: <20170426044608.GA32451@hori1.linux.bs1.fc.nec.co.jp>
On Wed, 2017-04-26 at 04:46 +0000, Naoya Horiguchi wrote:
> On Wed, Apr 26, 2017 at 01:45:00PM +1000, Balbir Singh wrote:
> > > > > static int delete_from_lru_cache(struct page *p)
> > > > > {
> > > > > + if (memcg_kmem_enabled())
> > > > > + memcg_kmem_uncharge(p, 0);
> > > > > +
> > > >
> > > > The changelog is not quite clear, so we are uncharging a page using
> > > > memcg_kmem_uncharge for a page in swap cache/page cache?
> > >
> > > Hi Balbir,
> > >
> > > Yes, in the normal page lifecycle, uncharge is done in page free time.
> > > But in memory error handling case, in-use pages (i.e. swap cache and page
> > > cache) are removed from normal path and they don't pass page freeing code.
> > > So I think that this change is to keep the consistent charging for such a case.
> >
> > I agree we should uncharge, but looking at the API name, it seems to
> > be for kmem pages, why are we not using mem_cgroup_uncharge()? Am I missing
> > something?
>
> Thank you for pointing out.
> Actually I had the same question and this surely looks strange.
> But simply calling mem_cgroup_uncharge() here doesn't work because it
> assumes that page_refcount(p) == 0, which is not true in hwpoison context.
> We need some other clearer way or at least some justifying comment about
> why this is ok.
>
We should call mem_cgroup_uncharge() after isolate_lru_page()/put_page().
We could check if page_count() is 0 or force if required (!MF_RECOVERED &&
!MF_DELAYED). We could even skip the VM_BUG_ON if the page is poisoned.
Balbir Singh.
--
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 2/2] dax: fix data corruption due to stale mmap reads
From: Jan Kara @ 2017-04-26 8:52 UTC (permalink / raw)
To: Ross Zwisler
Cc: Jan Kara, Andrew Morton, linux-kernel, Alexander Viro,
Alexey Kuznetsov, Andrey Ryabinin, Anna Schumaker,
Christoph Hellwig, Dan Williams, Darrick J. Wong,
Eric Van Hensbergen, Jens Axboe, Johannes Weiner,
Konrad Rzeszutek Wilk, Latchesar Ionkov, linux-cifs,
linux-fsdevel, linux-mm, linux-nfs, linux-nvdimm, Matthew Wilcox,
Ron Minnich, samba-technical, Steve French, Trond Myklebust,
v9fs-developer
In-Reply-To: <20170425225936.GA29655@linux.intel.com>
On Tue 25-04-17 16:59:36, Ross Zwisler wrote:
> On Tue, Apr 25, 2017 at 01:10:43PM +0200, Jan Kara wrote:
> <>
> > Hum, but now thinking more about it I have hard time figuring out why write
> > vs fault cannot actually still race:
> >
> > CPU1 - write(2) CPU2 - read fault
> >
> > dax_iomap_pte_fault()
> > ->iomap_begin() - sees hole
> > dax_iomap_rw()
> > iomap_apply()
> > ->iomap_begin - allocates blocks
> > dax_iomap_actor()
> > invalidate_inode_pages2_range()
> > - there's nothing to invalidate
> > grab_mapping_entry()
> > - we add zero page in the radix
> > tree & map it to page tables
> >
> > Similarly read vs write fault may end up racing in a wrong way and try to
> > replace already existing exceptional entry with a hole page?
>
> Yep, this race seems real to me, too. This seems very much like the issues
> that exist when a thread is doing direct I/O. One thread is doing I/O to an
> intermediate buffer (page cache for direct I/O case, zero page for us), and
> the other is going around it directly to media, and they can get out of sync.
>
> IIRC the direct I/O code looked something like:
>
> 1/ invalidate existing mappings
> 2/ do direct I/O to media
> 3/ invalidate mappings again, just in case. Should be cheap if there weren't
> any conflicting faults. This makes sure any new allocations we made are
> faulted in.
Yeah, the problem is people generally expect weird behavior when they mix
direct and buffered IO (or let alone mmap) however everyone expects
standard read(2) and write(2) to be completely coherent with mmap(2).
> I guess one option would be to replicate that logic in the DAX I/O path, or we
> could try and enhance our locking so page faults can't race with I/O since
> both can allocate blocks.
In the abstract way, the problem is that we have radix tree (and page
tables) cache block mapping information and the operation: "read block
mapping information, store it in the radix tree" is not serialized in any
way against other block allocations so the information we store can be out
of date by the time we store it.
One way to solve this would be to move ->iomap_begin call in the fault
paths under entry lock although that would mean I have to redo how ext4
handles DAX faults because with current code it would create lock inversion
wrt transaction start.
Another solution would be to grab i_mmap_sem for write when doing write
fault of a page and similarly have it grabbed for writing when doing
write(2). This would scale rather poorly but if we later replaced it with a
range lock (Davidlohr has already posted a nice implementation of it) it
won't be as bad. But I guess option 1) is better...
Honza
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
--
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 0/5] {ioctl_}userfaultfd.2: initial updates for 4.11
From: Michael Kerrisk (man-pages) @ 2017-04-26 8:43 UTC (permalink / raw)
To: Mike Rapoport
Cc: mtk.manpages, Andrea Arcangeli, linux-kernel, linux-mm, linux-man
In-Reply-To: <20170426074339.GE16837@rapoport-lnx>
Hello Mike,
On 04/26/2017 09:43 AM, Mike Rapoport wrote:
> On Wed, Apr 26, 2017 at 09:23:45AM +0200, Michael Kerrisk (man-pages) wrote:
>> Hello Mike,
>>
>> On 04/25/2017 06:29 PM, Mike Rapoport wrote:
>>> Hello Michael,
>>>
>>> These patches are some kind of brief highlights of the changes to the
>>> userfaultfd pages.
>>
>> Thanks for the patches. All merged. A few tweaks made,
>> and pushed to Git.
>>
>>> The changes to userfaultfd functionality are also described at update to
>>> Documentation/vm/userfaultfd.txt [1].
>>>
>>> In general, there were three major additions:
>>> * hugetlbfs support
>>> * shmem support
>>> * non-page fault events
>>>
>>> I think we should add some details about using userfaultfd with different
>>> memory types, describe meaning of each feature bits and add some text about
>>> the new events.
>>
>> Agreed.
>>
>>> I haven't updated 'struct uffd_msg' yet, and I hesitate whether it's
>>> description belongs to userfaultfd.2 or ioctl_userfaultfd.2
>>
>> My guess is userfaultfd.2. But, maybe I missed something.
>> What suggests to you that it could be ioctl_userfaultfd.2 instead?
>
> I've started to add relatively elaborate descriptions of UFFD_EVENT_* to
> ioctl_userfaultfd.2 and I've found that I write a lot about struct uffd_msg
> fields, but the structure itself is described at userfaultfd.2.
> Now, when I'm thinking about it, maybe it would be better to put the
> detailed descriptions of the events in userfaultfd.2 and only brief notes
> in ioctl_userfaultfd.2.
Okay -- we can tune as we go along.
>>> As for the userfaultfd.7 we've discussed earlier, I believe it would
>>> repeat Documentation/vm/userfaultfd.txt in way, so I'm not really sure it
>>> is required.
>>
>> The thing about kernel Doc files is they are a lot less visible.
>> It would be best I think to have the user-space visible
>> API fully described in man pages...
>
> I agree with the point about the visibility, I just don't know if
> userfaultfd.7 would be required or we'll have all the necessary bits in
> {ioctl_}userfaultfd.2. I'm going to add more content to the man2 pages and
> then we'll see if we need man7 as well.
Okay.
Cheers,
Michael
>> Cheers,
>>
>> Michael
>>
>>
>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5a02026d390ea1bb0c16a0e214e45613a3e3d885
>>>
>>> Mike Rapoport (5):
>>> userfaultfd.2: describe memory types that can be used from 4.11
>>> ioctl_userfaultfd.2: describe memory types that can be used from 4.11
>>> ioctl_userfaultfd.2: update UFFDIO_API description
>>> userfaultfd.2: add Linux container migration use-case to NOTES
>>> usefaultfd.2: add brief description of "non-cooperative" mode
>>>
>>> man2/ioctl_userfaultfd.2 | 46 ++++++++++++++++++++++++++++++++++++++--------
>>> man2/userfaultfd.2 | 25 ++++++++++++++++++++++---
>>> 2 files changed, 60 insertions(+), 11 deletions(-)
>>>
>>
>>
>> --
>> Michael Kerrisk
>> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
>> Linux/UNIX System Programming Training: http://man7.org/training/
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-man" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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 1/6] mm, page_alloc: fix more premature OOM due to race with cpuset update
From: Vlastimil Babka @ 2017-04-26 8:07 UTC (permalink / raw)
To: Christoph Lameter
Cc: linux-mm, linux-kernel, cgroups, Li Zefan, Michal Hocko,
Mel Gorman, David Rientjes, Hugh Dickins, Andrea Arcangeli,
Anshuman Khandual, Kirill A. Shutemov, linux-api
In-Reply-To: <alpine.DEB.2.20.1704141526260.17435@east.gentwo.org>
On 04/14/2017 10:37 PM, Christoph Lameter wrote:
> On Thu, 13 Apr 2017, Vlastimil Babka wrote:
>
>>
>> I doubt we can change that now, because that can break existing
>> programs. It also makes some sense at least to me, because a task can
>> control its own mempolicy (for performance reasons), but cpuset changes
>> are admin decisions that the task cannot even anticipate. I think it's
>> better to continue working with suboptimal performance than start
>> failing allocations?
>
> If the expected semantics (hardwall) are that allocations should fail then
> lets be consistent and do so.
It's not "expected" right now. The documented semantics is that (static,
as the others are rebound) mempolicy is ignored when it's not compatible
with cpuset. I'm just reusing the same existing semantic for race
situations. We can discuss whether we can change the semantics now, but
I don't think it should block this fix.
> Adding more and more exceptions gets this convoluted mess into an even
> worse shape.
Again, it's not a new exception semantics-wise, but I agree that the
code of __alloc_pages_slowpath() is even more subtle. But I don't see
any other easy fix.
> Adding the static binding of nodes was already a screwball
> if used within a cpuset because now one has to anticipate how a user would
> move the nodes of a cpuset and how the static bindings would work in such
> a context.
On the other hand, static mempolicy is the only one that does not need
rebinding, and removing the other modes would allow much simpler
implementation. I thought the outcome of LSF/MM session was that we
should try to go that way.
> The admin basically needs to know how the application has used memory
> policies if one still wants to move the applications within a cpuset with
> the fixed bindings.
>
> Maybe the best way to handle this is to give up on cpuset migration of
> live applications? After all this can be done with a script in the same
> way as the kernel is doing:
>
> 1. Extend the cpuset to include the new nodes.
>
> 2. Loop over the processes and use the migrate_pages() to move the apps
> one by one.
>
> 3. Remove the nodes no longer to be used.
>
> Then forget about translating memory policies. If an application that is
> supposed to run in a cpuset and supposed to be moveable has fixed bindings
> then the application should be aware of that and be equipped with
> some logic to rebind its memory on its own.
>
> Such an application typically already has such logic and executes a
> binding after discovering its numa node configuration on startup. It would
> have to be modified to redo that action when it gets some sort of a signal
> from the script telling it that the node config would be changed.
>
> Having this logic in the application instead of the kernel avoids all the
> kernel messes that we keep on trying to deal with and IMHO is much
> cleaner.
That would be much simpler for us indeed. But we still IMHO can't
abruptly start denying page fault allocations for existing applications
that don't have the necessary awareness.
> --
> 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>
>
--
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 2/2] dax: add regression test for stale mmap reads
From: Eryu Guan @ 2017-04-26 7:47 UTC (permalink / raw)
To: Ross Zwisler
Cc: fstests, Xiong Zhou, jmoyer, Christoph Hellwig, Dan Williams,
Darrick J. Wong, Jan Kara, linux-fsdevel, linux-mm, linux-nvdimm,
Andrew Morton
In-Reply-To: <20170425205106.20576-2-ross.zwisler@linux.intel.com>
On Tue, Apr 25, 2017 at 02:51:06PM -0600, Ross Zwisler wrote:
> This adds a regression test for the following kernel patch:
>
> dax: fix data corruption due to stale mmap reads
>
> The above patch fixes an issue where users of DAX can suffer data
> corruption from stale mmap reads via the following sequence:
>
> - open an mmap over a 2MiB hole
>
> - read from a 2MiB hole, faulting in a 2MiB zero page
>
> - write to the hole with write(3p). The write succeeds but we incorrectly
> leave the 2MiB zero page mapping intact.
>
> - via the mmap, read the data that was just written. Since the zero page
> mapping is still intact we read back zeroes instead of the new data.
>
> Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
I reproduced it now with 4.10 kernel, I think it was because I enalbed
KASAN on my 4.11-rc8 kernel. And the special mkfs options for ext4/xfs
are not needed either.
> ---
> .gitignore | 1 +
> src/Makefile | 2 +-
> src/t_dax_stale_pmd.c | 59 +++++++++++++++++++++++++++++++++++++++++++++
> tests/generic/427 | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++
> tests/generic/427.out | 2 ++
> tests/generic/group | 1 +
> 6 files changed, 131 insertions(+), 1 deletion(-)
> create mode 100644 src/t_dax_stale_pmd.c
> create mode 100755 tests/generic/427
> create mode 100644 tests/generic/427.out
>
> diff --git a/.gitignore b/.gitignore
> index ded4a61..9664dc9 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -134,6 +134,7 @@
> /src/renameat2
> /src/t_rename_overwrite
> /src/t_mmap_dio
> +/src/t_dax_stale_pmd
>
> # dmapi/ binaries
> /dmapi/src/common/cmd/read_invis
> diff --git a/src/Makefile b/src/Makefile
> index abfd873..7e22b50 100644
> --- a/src/Makefile
> +++ b/src/Makefile
> @@ -12,7 +12,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
> godown resvtest writemod makeextents itrash rename \
> multi_open_unlink dmiperf unwritten_sync genhashnames t_holes \
> t_mmap_writev t_truncate_cmtime dirhash_collide t_rename_overwrite \
> - holetest t_truncate_self t_mmap_dio af_unix
> + holetest t_truncate_self t_mmap_dio af_unix t_dax_stale_pmd
>
> LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> diff --git a/src/t_dax_stale_pmd.c b/src/t_dax_stale_pmd.c
> new file mode 100644
> index 0000000..59fbbe1
> --- /dev/null
> +++ b/src/t_dax_stale_pmd.c
> @@ -0,0 +1,59 @@
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <libgen.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#define MiB(a) ((a)*1024*1024)
> +
> +void err_exit(char *op)
> +{
> + fprintf(stderr, "%s: %s\n", op, strerror(errno));
> + exit(1);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> + volatile int a __attribute__((__unused__));
> + char *buffer = "HELLO WORLD!";
> + char *data;
> + int fd;
> +
> + if (argc < 2) {
> + printf("Usage: %s <pmem file>\n", basename(argv[0]));
> + exit(0);
> + }
> +
> + fd = open(argv[1], O_RDWR);
> + if (fd < 0)
> + err_exit("fd");
> +
> + data = mmap(NULL, MiB(2), PROT_READ, MAP_SHARED, fd, MiB(2));
> +
> + /*
> + * This faults in a 2MiB zero page to satisfy the read.
> + * 'a' is volatile so this read doesn't get optimized out.
> + */
> + a = data[0];
> +
> + pwrite(fd, buffer, strlen(buffer), MiB(2));
> +
> + /*
> + * Try and use the mmap to read back the data we just wrote with
> + * pwrite(). If the kernel bug is present the mapping from the 2MiB
> + * zero page will still be intact, and we'll read back zeros instead.
> + */
> + if (strncmp(buffer, data, strlen(buffer))) {
> + fprintf(stderr, "strncmp mismatch: '%s' vs '%s'\n", buffer,
> + data);
> + exit(1);
> + }
> +
> + close(fd);
> + return 0;
> +}
> diff --git a/tests/generic/427 b/tests/generic/427
> new file mode 100755
> index 0000000..6e265a1
> --- /dev/null
> +++ b/tests/generic/427
> @@ -0,0 +1,67 @@
> +#! /bin/bash
> +# FS QA Test 427
> +#
> +# This is a regression test for kernel patch:
> +# dax: fix data corruption due to stale mmap reads
> +# created by Ross Zwisler <ross.zwisler@linux.intel.com>
> +#
> +#-----------------------------------------------------------------------
> +# Copyright (c) 2017 Intel Corporation. All Rights Reserved.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation.
> +#
> +# This program is distributed in the hope that it would be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write the Free Software Foundation,
> +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> +#-----------------------------------------------------------------------
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1 # failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> + cd /
> + rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +. ./common/filter
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +# Modify as appropriate.
> +_supported_fs generic
> +_supported_os Linux
> +_require_test_program "t_dax_stale_pmd"
> +_require_xfs_io_command "falloc"
I'm wondering if falloc is really needed? If not, this test could be run
with ext2/3 too. See below.
> +_require_user
This is not needed anymore.
> +
> +# real QA test starts here
> +
> +# ensure we have no pre-existing block allocations, so we get a hole
> +rm -f $TEST_DIR/testfile
> +$XFS_IO_PROG -f -c "falloc 0 4M" $TEST_DIR/testfile >> $seqres.full 2>&1
I found that 'xfs_io -fc "truncate 4M" $TEST_DIR/testfile' works too,
from the comments in test and kernel patch, if I understand correctly,
we only need to mmap un-allocated blocks, right?
If truncate(2) works too, I think we can move truncate operation to the
t_dax_stale_pmd program too, because the whole truncate && mmap && read
sequence are logically together, this also avoids the confusion on why
testfile is in 4M size.
Thanks,
Eryu
> +chmod 0644 $TEST_DIR/testfile
> +
> +src/t_dax_stale_pmd $TEST_DIR/testfile
> +
> +# success, all done
> +echo "Silence is golden"
> +status=0
> +exit
> diff --git a/tests/generic/427.out b/tests/generic/427.out
> new file mode 100644
> index 0000000..61295e5
> --- /dev/null
> +++ b/tests/generic/427.out
> @@ -0,0 +1,2 @@
> +QA output created by 427
> +Silence is golden
> diff --git a/tests/generic/group b/tests/generic/group
> index f29009c..06f6e9d 100644
> --- a/tests/generic/group
> +++ b/tests/generic/group
> @@ -429,3 +429,4 @@
> 424 auto quick
> 425 auto quick attr
> 426 auto quick exportfs
> +427 auto quick
> --
> 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: [PATCH 0/5] {ioctl_}userfaultfd.2: initial updates for 4.11
From: Mike Rapoport @ 2017-04-26 7:43 UTC (permalink / raw)
To: Michael Kerrisk (man-pages)
Cc: Andrea Arcangeli, linux-kernel, linux-mm, linux-man
In-Reply-To: <428a9209-e712-7067-ab11-9c35cddcd89e@gmail.com>
On Wed, Apr 26, 2017 at 09:23:45AM +0200, Michael Kerrisk (man-pages) wrote:
> Hello Mike,
>
> On 04/25/2017 06:29 PM, Mike Rapoport wrote:
> > Hello Michael,
> >
> > These patches are some kind of brief highlights of the changes to the
> > userfaultfd pages.
>
> Thanks for the patches. All merged. A few tweaks made,
> and pushed to Git.
>
> > The changes to userfaultfd functionality are also described at update to
> > Documentation/vm/userfaultfd.txt [1].
> >
> > In general, there were three major additions:
> > * hugetlbfs support
> > * shmem support
> > * non-page fault events
> >
> > I think we should add some details about using userfaultfd with different
> > memory types, describe meaning of each feature bits and add some text about
> > the new events.
>
> Agreed.
>
> > I haven't updated 'struct uffd_msg' yet, and I hesitate whether it's
> > description belongs to userfaultfd.2 or ioctl_userfaultfd.2
>
> My guess is userfaultfd.2. But, maybe I missed something.
> What suggests to you that it could be ioctl_userfaultfd.2 instead?
I've started to add relatively elaborate descriptions of UFFD_EVENT_* to
ioctl_userfaultfd.2 and I've found that I write a lot about struct uffd_msg
fields, but the structure itself is described at userfaultfd.2.
Now, when I'm thinking about it, maybe it would be better to put the
detailed descriptions of the events in userfaultfd.2 and only brief notes
in ioctl_userfaultfd.2.
> > As for the userfaultfd.7 we've discussed earlier, I believe it would
> > repeat Documentation/vm/userfaultfd.txt in way, so I'm not really sure it
> > is required.
>
> The thing about kernel Doc files is they are a lot less visible.
> It would be best I think to have the user-space visible
> API fully described in man pages...
I agree with the point about the visibility, I just don't know if
userfaultfd.7 would be required or we'll have all the necessary bits in
{ioctl_}userfaultfd.2. I'm going to add more content to the man2 pages and
then we'll see if we need man7 as well.
> Cheers,
>
> Michael
>
>
> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5a02026d390ea1bb0c16a0e214e45613a3e3d885
> >
> > Mike Rapoport (5):
> > userfaultfd.2: describe memory types that can be used from 4.11
> > ioctl_userfaultfd.2: describe memory types that can be used from 4.11
> > ioctl_userfaultfd.2: update UFFDIO_API description
> > userfaultfd.2: add Linux container migration use-case to NOTES
> > usefaultfd.2: add brief description of "non-cooperative" mode
> >
> > man2/ioctl_userfaultfd.2 | 46 ++++++++++++++++++++++++++++++++++++++--------
> > man2/userfaultfd.2 | 25 ++++++++++++++++++++++---
> > 2 files changed, 60 insertions(+), 11 deletions(-)
> >
>
>
> --
> Michael Kerrisk
> Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
> Linux/UNIX System Programming Training: http://man7.org/training/
>
--
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 0/5] {ioctl_}userfaultfd.2: initial updates for 4.11
From: Michael Kerrisk (man-pages) @ 2017-04-26 7:23 UTC (permalink / raw)
To: Mike Rapoport
Cc: mtk.manpages, Andrea Arcangeli, linux-kernel, linux-mm, linux-man
In-Reply-To: <1493137748-32452-1-git-send-email-rppt@linux.vnet.ibm.com>
Hello Mike,
On 04/25/2017 06:29 PM, Mike Rapoport wrote:
> Hello Michael,
>
> These patches are some kind of brief highlights of the changes to the
> userfaultfd pages.
Thanks for the patches. All merged. A few tweaks made,
and pushed to Git.
> The changes to userfaultfd functionality are also described at update to
> Documentation/vm/userfaultfd.txt [1].
>
> In general, there were three major additions:
> * hugetlbfs support
> * shmem support
> * non-page fault events
>
> I think we should add some details about using userfaultfd with different
> memory types, describe meaning of each feature bits and add some text about
> the new events.
Agreed.
> I haven't updated 'struct uffd_msg' yet, and I hesitate whether it's
> description belongs to userfaultfd.2 or ioctl_userfaultfd.2
My guess is userfaultfd.2. But, maybe I missed something.
What suggests to you that it could be ioctl_userfaultfd.2 instead?
> As for the userfaultfd.7 we've discussed earlier, I believe it would
> repeat Documentation/vm/userfaultfd.txt in way, so I'm not really sure it
> is required.
The thing about kernel Doc files is they are a lot less visible.
It would be best I think to have the user-space visible
API fully described in man pages...
Cheers,
Michael
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5a02026d390ea1bb0c16a0e214e45613a3e3d885
>
> Mike Rapoport (5):
> userfaultfd.2: describe memory types that can be used from 4.11
> ioctl_userfaultfd.2: describe memory types that can be used from 4.11
> ioctl_userfaultfd.2: update UFFDIO_API description
> userfaultfd.2: add Linux container migration use-case to NOTES
> usefaultfd.2: add brief description of "non-cooperative" mode
>
> man2/ioctl_userfaultfd.2 | 46 ++++++++++++++++++++++++++++++++++++++--------
> man2/userfaultfd.2 | 25 ++++++++++++++++++++++---
> 2 files changed, 60 insertions(+), 11 deletions(-)
>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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 5/5] usefaultfd.2: add brief description of "non-cooperative" mode
From: Michael Kerrisk (man-pages) @ 2017-04-26 7:18 UTC (permalink / raw)
To: Mike Rapoport
Cc: mtk.manpages, Andrea Arcangeli, linux-kernel, linux-mm, linux-man
In-Reply-To: <1493137748-32452-6-git-send-email-rppt@linux.vnet.ibm.com>
On 04/25/2017 06:29 PM, Mike Rapoport wrote:
> Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
Thanks, Mike. Applied.
Cheers,
Michael
> ---
> man2/userfaultfd.2 | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/man2/userfaultfd.2 b/man2/userfaultfd.2
> index dc37319..291dd10 100644
> --- a/man2/userfaultfd.2
> +++ b/man2/userfaultfd.2
> @@ -89,6 +89,20 @@ them using the operations described in
> .BR ioctl_userfaultfd (2).
> When servicing the page fault events,
> the fault-handling thread can trigger a wake-up for the sleeping thread.
> +
> +It is possible for the faulting threads and the fault-handling threads
> +to run in the context of different processes.
> +In this case, these threads may belong to different programs,
> +and the program that executes the faulting threads
> +will not necessarily cooperate with the program that handles the page faults.
> +In such non-cooperative mode,
> +the process that monitors userfaultfd and handles page faults,
> +needs to be aware of the changes in the virtual memory layout
> +of the faulting process to avoid memory corruption.
> +.\" FIXME elaborate about non-cooperating mode, describe its limitations
> +.\" for kerneles before 4.11, features added in 4.11
> +.\" and limitations remaining in 4.11
> +.\" Maybe it's worth adding a dedicated sub-section...
> .\"
> .SS Userfaultfd operation
> After the userfaultfd object is created with
>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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 4/5] userfaultfd.2: add Linux container migration use-case to NOTES
From: Michael Kerrisk (man-pages) @ 2017-04-26 7:16 UTC (permalink / raw)
To: Mike Rapoport
Cc: mtk.manpages, Andrea Arcangeli, linux-kernel, linux-mm, linux-man
In-Reply-To: <1493137748-32452-5-git-send-email-rppt@linux.vnet.ibm.com>
On 04/25/2017 06:29 PM, Mike Rapoport wrote:
> Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
Thanks, Mike. Applied.
Cheers,
Michael
> ---
> man2/userfaultfd.2 | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/man2/userfaultfd.2 b/man2/userfaultfd.2
> index c89484f..dc37319 100644
> --- a/man2/userfaultfd.2
> +++ b/man2/userfaultfd.2
> @@ -279,7 +279,8 @@ signal and
> It can also be used to implement lazy restore
> for checkpoint/restore mechanisms,
> as well as post-copy migration to allow (nearly) uninterrupted execution
> -when transferring virtual machines from one host to another.
> +when transferring virtual machines and Linux containers
> +from one host to another.
> .SH EXAMPLE
> The program below demonstrates the use of the userfaultfd mechanism.
> The program creates two threads, one of which acts as the
>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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 2/5] ioctl_userfaultfd.2: describe memory types that can be used from 4.11
From: Michael Kerrisk (man-pages) @ 2017-04-26 6:52 UTC (permalink / raw)
To: Mike Rapoport
Cc: mtk.manpages, Andrea Arcangeli, linux-kernel, linux-mm, linux-man
In-Reply-To: <1493137748-32452-3-git-send-email-rppt@linux.vnet.ibm.com>
On 04/25/2017 06:29 PM, Mike Rapoport wrote:
> Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
Thanks, Mike. Applied.
Cheers,
Michael
> ---
> man2/ioctl_userfaultfd.2 | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/man2/ioctl_userfaultfd.2 b/man2/ioctl_userfaultfd.2
> index 66fbfdc..78abc4d 100644
> --- a/man2/ioctl_userfaultfd.2
> +++ b/man2/ioctl_userfaultfd.2
> @@ -169,11 +169,15 @@ field was not zero.
> (Since Linux 4.3.)
> Register a memory address range with the userfaultfd object.
> The pages in the range must be "compatible".
> -In the current implementation,
> -.\" According to Mike Rapoport, this will change in Linux 4.11.
> +
> +Up to Linux kernel 4.11,
> only private anonymous ranges are compatible for registering with
> .BR UFFDIO_REGISTER .
>
> +Since Linux 4.11,
> +hugetlbfs and shared memory ranges are also compatible with
> +.BR UFFDIO_REGISTER .
> +
> The
> .I argp
> argument is a pointer to a
>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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 1/5] userfaultfd.2: describe memory types that can be used from 4.11
From: Michael Kerrisk (man-pages) @ 2017-04-26 6:52 UTC (permalink / raw)
To: Mike Rapoport
Cc: mtk.manpages, Andrea Arcangeli, linux-kernel, linux-mm, linux-man
In-Reply-To: <1493137748-32452-2-git-send-email-rppt@linux.vnet.ibm.com>
On 04/25/2017 06:29 PM, Mike Rapoport wrote:
> Signed-off-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
Thanks, Mike. Applied.
Cheers,
Michael
> ---
> man2/userfaultfd.2 | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/man2/userfaultfd.2 b/man2/userfaultfd.2
> index 1603c20..c89484f 100644
> --- a/man2/userfaultfd.2
> +++ b/man2/userfaultfd.2
> @@ -130,8 +130,12 @@ Details of the various
> operations can be found in
> .BR ioctl_userfaultfd (2).
>
> -Currently, userfaultfd can be used only with anonymous private memory
> -mappings.
> +Up to Linux 4.11,
> +userfaultfd can be used only with anonymous private memory mappings.
> +
> +Since Linux 4.11,
> +userfaultfd can be also used with hugetlbfs and shared memory mappings.
> +
> .\"
> .SS Reading from the userfaultfd structure
> Each
>
--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
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: Naoya Horiguchi @ 2017-04-26 4:46 UTC (permalink / raw)
To: Balbir Singh
Cc: Laurent Dufour, linux-kernel@vger.kernel.org, linux-mm@kvack.org,
akpm@linux-foundation.org
In-Reply-To: <1493178300.4828.5.camel@gmail.com>
On Wed, Apr 26, 2017 at 01:45:00PM +1000, Balbir Singh wrote:
> > > > static int delete_from_lru_cache(struct page *p)
> > > > {
> > > > + if (memcg_kmem_enabled())
> > > > + memcg_kmem_uncharge(p, 0);
> > > > +
> > >
> > > The changelog is not quite clear, so we are uncharging a page using
> > > memcg_kmem_uncharge for a page in swap cache/page cache?
> >
> > Hi Balbir,
> >
> > Yes, in the normal page lifecycle, uncharge is done in page free time.
> > But in memory error handling case, in-use pages (i.e. swap cache and page
> > cache) are removed from normal path and they don't pass page freeing code.
> > So I think that this change is to keep the consistent charging for such a case.
>
> I agree we should uncharge, but looking at the API name, it seems to
> be for kmem pages, why are we not using mem_cgroup_uncharge()? Am I missing
> something?
Thank you for pointing out.
Actually I had the same question and this surely looks strange.
But simply calling mem_cgroup_uncharge() here doesn't work because it
assumes that page_refcount(p) == 0, which is not true in hwpoison context.
We need some other clearer way or at least some justifying comment about
why this is ok.
- Naoya
--
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 V2] mm/madvise: Enable (soft|hard) offline of HugeTLB pages at PGD level
From: Anshuman Khandual @ 2017-04-26 3:57 UTC (permalink / raw)
To: linux-kernel, linux-mm; +Cc: n-horiguchi, akpm, aneesh.kumar
Though migrating gigantic HugeTLB pages does not sound much like real
world use case, they can be affected by memory errors. Hence migration
at the PGD level HugeTLB pages should be supported just to enable soft
and hard offline use cases.
While allocating the new gigantic HugeTLB page, it should not matter
whether new page comes from the same node or not. There would be very
few gigantic pages on the system afterall, we should not be bothered
about node locality when trying to save a big page from crashing.
This introduces a new HugeTLB allocator called alloc_huge_page_nonid()
which will scan over all online nodes on the system and allocate a
single HugeTLB page.
Signed-off-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>
---
Tested on a POWER8 machine with 16GB pages along with Aneesh's
recent HugeTLB enablement patch series on powerpc which can
be found here.
https://lkml.org/lkml/2017/4/17/225
Here, we directly call alloc_huge_page_nonid() which ignores the
node locality. But we can also first call normal alloc_huge_page()
with the node number and if that fails to allocate only then call
alloc_huge_page_nonid() as a fallback option.
Aneesh mentioned about the waste of memory if we just have to
soft offline a single page. The problem persists both on PGD
as well as PMD level HugeTLB pages. Tried solving the problem
with https://patchwork.kernel.org/patch/9690119/ but right now
madvise splits the entire range of HugeTLB pages (if the page
is HugeTLB one) and calls soft_offline_page() on the head page
of each HugeTLB page as soft_offline_page() acts on the entire
HugeTLB range not just the individual pages. Changing the iterator
in madvise() scan over individual pages solves the problem but
then it creates multiple HugeTLB migrations (HUGE_PAGE_SIZE /
PAGE_SIZE times to be precise) if we really have to soft offline
a single HugeTLB page which is not optimal.
Hence for now, lets just enable PGD level HugeTLB soft offline
at par with the PMD level HugeTLB before we can go back and
address the memory wastage problem comprehensively for both
PGD and PMD level HugeTLB page as mentioned above.
Changes in V2:
* Added hstate_is_gigantic() definition when !CONFIG_HUGETLB_PAGE
which takes care of the build failure reported earlier.
mm/hugetlb.c | 17 +++++++++++++++++
mm/memory-failure.c | 8 ++++++--
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
index 04b73a9c8b4b..964d964f22c8 100644
--- a/include/linux/hugetlb.h
+++ b/include/linux/hugetlb.h
@@ -347,6 +347,7 @@ struct huge_bootmem_page {
struct page *alloc_huge_page(struct vm_area_struct *vma,
unsigned long addr, int avoid_reserve);
+struct page *alloc_huge_page_nonid(struct hstate *h);
struct page *alloc_huge_page_node(struct hstate *h, int nid);
struct page *alloc_huge_page_noerr(struct vm_area_struct *vma,
unsigned long addr, int avoid_reserve);
@@ -473,7 +474,11 @@ extern int dissolve_free_huge_pages(unsigned long start_pfn,
static inline bool hugepage_migration_supported(struct hstate *h)
{
#ifdef CONFIG_ARCH_ENABLE_HUGEPAGE_MIGRATION
- return huge_page_shift(h) == PMD_SHIFT;
+ if ((huge_page_shift(h) == PMD_SHIFT) ||
+ (huge_page_shift(h) == PGDIR_SHIFT))
+ return true;
+ else
+ return false;
#else
return false;
#endif
@@ -511,6 +516,7 @@ static inline void hugetlb_count_sub(long l, struct mm_struct *mm)
#else /* CONFIG_HUGETLB_PAGE */
struct hstate {};
#define alloc_huge_page(v, a, r) NULL
+#define alloc_huge_page_nonid(h) NULL
#define alloc_huge_page_node(h, nid) NULL
#define alloc_huge_page_noerr(v, a, r) NULL
#define alloc_bootmem_huge_page(h) NULL
@@ -525,6 +531,7 @@ struct hstate {};
#define vma_mmu_pagesize(v) PAGE_SIZE
#define huge_page_order(h) 0
#define huge_page_shift(h) PAGE_SHIFT
+#define hstate_is_gigantic(h) 0
static inline unsigned int pages_per_huge_page(struct hstate *h)
{
return 1;
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 97a44db06850..bd96fff2bc09 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -1669,6 +1669,23 @@ struct page *__alloc_buddy_huge_page_with_mpol(struct hstate *h,
return __alloc_buddy_huge_page(h, vma, addr, NUMA_NO_NODE);
}
+struct page *alloc_huge_page_nonid(struct hstate *h)
+{
+ struct page *page = NULL;
+ int nid = 0;
+
+ spin_lock(&hugetlb_lock);
+ if (h->free_huge_pages - h->resv_huge_pages > 0) {
+ for_each_online_node(nid) {
+ page = dequeue_huge_page_node(h, nid);
+ if (page)
+ break;
+ }
+ }
+ spin_unlock(&hugetlb_lock);
+ return page;
+}
+
/*
* This allocation function is useful in the context where vma is irrelevant.
* E.g. soft-offlining uses this function because it only cares physical
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index fe64d7729a8e..d4f5710cf3f7 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1481,11 +1481,15 @@ EXPORT_SYMBOL(unpoison_memory);
static struct page *new_page(struct page *p, unsigned long private, int **x)
{
int nid = page_to_nid(p);
- if (PageHuge(p))
+ if (PageHuge(p)) {
+ if (hstate_is_gigantic(page_hstate(compound_head(p))))
+ return alloc_huge_page_nonid(page_hstate(compound_head(p)));
+
return alloc_huge_page_node(page_hstate(compound_head(p)),
nid);
- else
+ } else {
return __alloc_pages_node(nid, GFP_HIGHUSER_MOVABLE, 0);
+ }
}
/*
--
2.12.0
--
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: [PATCH v2 1/2] mm: Uncharge poisoned pages
From: Balbir Singh @ 2017-04-26 3:45 UTC (permalink / raw)
To: Naoya Horiguchi
Cc: Laurent Dufour, linux-kernel@vger.kernel.org, linux-mm@kvack.org,
akpm@linux-foundation.org
In-Reply-To: <20170426023410.GA11619@hori1.linux.bs1.fc.nec.co.jp>
> > > static int delete_from_lru_cache(struct page *p)
> > > {
> > > + if (memcg_kmem_enabled())
> > > + memcg_kmem_uncharge(p, 0);
> > > +
> >
> > The changelog is not quite clear, so we are uncharging a page using
> > memcg_kmem_uncharge for a page in swap cache/page cache?
>
> Hi Balbir,
>
> Yes, in the normal page lifecycle, uncharge is done in page free time.
> But in memory error handling case, in-use pages (i.e. swap cache and page
> cache) are removed from normal path and they don't pass page freeing code.
> So I think that this change is to keep the consistent charging for such a case.
I agree we should uncharge, but looking at the API name, it seems to
be for kmem pages, why are we not using mem_cgroup_uncharge()? Am I missing
something?
Balbir Singh.
--
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 2/2] dax: add regression test for stale mmap reads
From: Eryu Guan @ 2017-04-26 3:42 UTC (permalink / raw)
To: Ross Zwisler
Cc: fstests, Xiong Zhou, jmoyer, Christoph Hellwig, Dan Williams,
Darrick J. Wong, Jan Kara, linux-fsdevel, linux-mm, linux-nvdimm,
Andrew Morton
In-Reply-To: <20170425203911.GC11773@linux.intel.com>
On Tue, Apr 25, 2017 at 02:39:11PM -0600, Ross Zwisler wrote:
> On Tue, Apr 25, 2017 at 07:27:39PM +0800, Eryu Guan wrote:
> > On Mon, Apr 24, 2017 at 11:49:32AM -0600, Ross Zwisler wrote:
> > > This adds a regression test for the following kernel patch:
> > >
> > > dax: fix data corruption due to stale mmap reads
> > >
> >
> > Seems that this patch hasn't been merged into linus tree, thus 4.11-rc8
> > kernel should fail this test, but it passed for me, tested with 4.11-rc8
> > kernel on both ext4 and xfs, with both brd devices and pmem devices
> > created from "memmap=10G!5G memmap=15G!15G" kernel boot command line.
> > Did I miss anything?
> >
> > # ./check -s ext4_pmem_4k generic/427
> > SECTION -- ext4_pmem_4k
>
> Ooh, I didn't add this 'ext4_pmem_4k' section goodness, and it's not present
> in the xfstests/master that I was using. Do you have patches to add that?
That's one of my config sections, it's all user-defined, not committed
to fstests repo :)
You can take a look at README.config-sections for more details. Here is
my local.config file for your reference
[default]
TEST_DEV=/dev/pmem0
SCRATCH_DEV=/dev/pmem1
TEST_DIR=/mnt
SCRATCH_MNT=/scratch
RECREATE_TEST_DEV=true
[xfs_pmem_4k]
FSTYP=xfs
MKFS_OPTIONS="-f -m crc=1 -b size=4k"
[ext4_pmem_4k]
FSTYP=ext4
MKFS_OPTIONS="-b 4096"
>
> > RECREATING -- ext4 on /dev/pmem0
> > FSTYP -- ext4
> > PLATFORM -- Linux/x86_64 hp-dl360g9-15 4.11.0-rc8.kasan
> > MKFS_OPTIONS -- -b 4096 /dev/pmem1
> > MOUNT_OPTIONS -- -o acl,user_xattr -o context=system_u:object_r:root_t:s0 /dev/pmem1 /scratch
> >
> > generic/427 1s ... 1s
> > Ran: generic/427
> > Passed all 1 tests
>
> Your memmap params look fine. I tested with BRD and PMEM, and with EXT4 and
> XFS, and all combinations failed for me as expected with v4.11-rc8.
>
> One issue could have been that the test file already existed when the test was
> run. I wasn't removing it between runs earlier, but I've fixed that for v2.
>
> Another issue I guess could have been that the hole that we got back from the
> filesystem was smaller than 2MiB? Can you try running v2 (which I'll post in
> a second) against a TEST_DEV made with one of the following:
>
> ext4: mkfs.ext4 -b 4096 -E stride=512 -F $TEST_DEV
> xfs: mkfs.xfs -f -d su=2m,sw=1 $TEST_DEV
>
> This helps us get 2MiB sized and aligned allocations so we can fault in PMDs,
> but I'm not sure whether or not it would matter for holes.
I guess that's the point to reproduce the failure, I'll confirm with v2
patches. If these non-default & not widely tested mkfs options are
required to reproduce it, I think we can specify them in the test, as
extra mkfs options to _scratch_mkfs, as what generic/413 does.
>
> > Some comments inline.
> >
> > > The above patch fixes an issue where users of DAX can suffer data
> > > corruption from stale mmap reads via the following sequence:
> > >
> > > - open an mmap over a 2MiB hole
> > >
> > > - read from a 2MiB hole, faulting in a 2MiB zero page
> > >
> > > - write to the hole with write(3p). The write succeeds but we incorrectly
> > > leave the 2MiB zero page mapping intact.
> > >
> > > - via the mmap, read the data that was just written. Since the zero page
> > > mapping is still intact we read back zeroes instead of the new data.
> > >
> > > Signed-off-by: Ross Zwisler <ross.zwisler@linux.intel.com>
> > > ---
> > > .gitignore | 1 +
> > > src/Makefile | 2 +-
> > > src/t_dax_stale_pmd.c | 56 ++++++++++++++++++++++++++++++++++++++++++
> > > tests/generic/427 | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++
> > > tests/generic/427.out | 2 ++
> > > tests/generic/group | 1 +
> > > 6 files changed, 129 insertions(+), 1 deletion(-)
> > > create mode 100644 src/t_dax_stale_pmd.c
> > > create mode 100755 tests/generic/427
> > > create mode 100644 tests/generic/427.out
> > >
> > > diff --git a/.gitignore b/.gitignore
> > > index ded4a61..9664dc9 100644
> > > --- a/.gitignore
> > > +++ b/.gitignore
> > > @@ -134,6 +134,7 @@
> > > /src/renameat2
> > > /src/t_rename_overwrite
> > > /src/t_mmap_dio
> > > +/src/t_dax_stale_pmd
> > >
> > > # dmapi/ binaries
> > > /dmapi/src/common/cmd/read_invis
> > > diff --git a/src/Makefile b/src/Makefile
> > > index abfd873..7e22b50 100644
> > > --- a/src/Makefile
> > > +++ b/src/Makefile
> > > @@ -12,7 +12,7 @@ TARGETS = dirstress fill fill2 getpagesize holes lstat64 \
> > > godown resvtest writemod makeextents itrash rename \
> > > multi_open_unlink dmiperf unwritten_sync genhashnames t_holes \
> > > t_mmap_writev t_truncate_cmtime dirhash_collide t_rename_overwrite \
> > > - holetest t_truncate_self t_mmap_dio af_unix
> > > + holetest t_truncate_self t_mmap_dio af_unix t_dax_stale_pmd
> > >
> > > LINUX_TARGETS = xfsctl bstat t_mtab getdevicesize preallo_rw_pattern_reader \
> > > preallo_rw_pattern_writer ftrunc trunc fs_perms testx looptest \
> > > diff --git a/src/t_dax_stale_pmd.c b/src/t_dax_stale_pmd.c
> > > new file mode 100644
> > > index 0000000..d0016eb
> > > --- /dev/null
> > > +++ b/src/t_dax_stale_pmd.c
> > > @@ -0,0 +1,56 @@
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +#include <libgen.h>
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > > +#include <string.h>
> > > +#include <sys/mman.h>
> > > +#include <sys/stat.h>
> > > +#include <sys/types.h>
> > > +#include <unistd.h>
> > > +
> > > +#define MiB(a) ((a)*1024*1024)
> > > +
> > > +void err_exit(char *op)
> > > +{
> > > + fprintf(stderr, "%s: %s\n", op, strerror(errno));
> > > + exit(1);
> > > +}
> > > +
> > > +int main(int argc, char *argv[])
> > > +{
> > > + volatile int a __attribute__((__unused__));
> > > + char *buffer = "HELLO WORLD!";
> > > + char *data;
> > > + int fd;
> > > +
> > > + if (argc < 2) {
> > > + printf("Usage: %s <pmem file>\n", basename(argv[0]));
> > > + exit(0);
> > > + }
> > > +
> > > + fd = open(argv[1], O_RDWR);
> > > + if (fd < 0)
> > > + err_exit("fd");
> > ^^^^ Nitpick, the "op" should be "open"?
> > > +
> > > + data = mmap(NULL, MiB(2), PROT_READ, MAP_SHARED, fd, MiB(2));
> > > +
> > > + /*
> > > + * This faults in a 2MiB zero page to satisfy the read.
> > > + * 'a' is volatile so this read doesn't get optimized out.
> > > + */
> > > + a = data[0];
> > > +
> > > + pwrite(fd, buffer, strlen(buffer), MiB(2));
> > > +
> > > + /*
> > > + * Try and use the mmap to read back the data we just wrote with
> > > + * pwrite(). If the kernel bug is present the mapping from the 2MiB
> > > + * zero page will still be intact, and we'll read back zeros instead.
> > > + */
> > > + if (strncmp(buffer, data, strlen(buffer)))
> > > + err_exit("strncmp mismatch!");
> >
> > strncmp doesn't set errno, this err_exit message might be confusing:
> > "strncmp mismatch!: Success"
>
> Ah, thanks, fixed in v2.
>
> > > +
> > > + close(fd);
> > > + return 0;
> > > +}
> > > diff --git a/tests/generic/427 b/tests/generic/427
> > > new file mode 100755
> > > index 0000000..baf1099
> > > --- /dev/null
> > > +++ b/tests/generic/427
> > > @@ -0,0 +1,68 @@
> > > +#! /bin/bash
> > > +# FS QA Test 427
> > > +#
> > > +# This is a regression test for kernel patch:
> > > +# dax: fix data corruption due to stale mmap reads
> > > +# created by Ross Zwisler <ross.zwisler@linux.intel.com>
> > > +#
> > > +#-----------------------------------------------------------------------
> > > +# Copyright (c) 2017 Intel Corporation. All Rights Reserved.
> > > +#
> > > +# This program is free software; you can redistribute it and/or
> > > +# modify it under the terms of the GNU General Public License as
> > > +# published by the Free Software Foundation.
> > > +#
> > > +# This program is distributed in the hope that it would be useful,
> > > +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> > > +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > > +# GNU General Public License for more details.
> > > +#
> > > +# You should have received a copy of the GNU General Public License
> > > +# along with this program; if not, write the Free Software Foundation,
> > > +# Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
> > > +#-----------------------------------------------------------------------
> > > +#
> > > +
> > > +seq=`basename $0`
> > > +seqres=$RESULT_DIR/$seq
> > > +echo "QA output created by $seq"
> > > +
> > > +here=`pwd`
> > > +tmp=/tmp/$$
> > > +status=1 # failure is the default!
> > > +trap "_cleanup; exit \$status" 0 1 2 3 15
> > > +
> > > +_cleanup()
> > > +{
> > > + cd /
> > > + rm -f $tmp.*
> > > +}
> > > +
> > > +# get standard environment, filters and checks
> > > +. ./common/rc
> > > +. ./common/filter
> > > +
> > > +# remove previous $seqres.full before test
> > > +rm -f $seqres.full
> > > +
> > > +# Modify as appropriate.
> > > +_supported_fs generic
> > > +_supported_os Linux
> > > +_require_scratch_dax
> >
> > I don't think dax is a requirement here, this test could run on normal
> > block device without "-o dax" option too. It won't hurt to run with more
> > test configurations. And test on nvdimm device with dax mount option
> > could be one of the test configs, e.g.
> >
> > TEST_DEV=/dev/pmem0
> > SCRATCH_DEV=/dev/pmem1
> > MOUNT_OPTIONS="-o dax"
> > ...
>
> Yep, agreed, fixed in v2.
Then perhaps the test program should be renamed? As no dax is required.
How about t_mmap_stale_pmd?
>
> > > +_require_test_program "t_dax_stale_pmd"
> > > +_require_user
> >
> > _require_xfs_io_command "falloc"
> >
> > So test _notrun on ext2/3.
>
> Fixed in v2.
>
> > > +
> > > +# real QA test starts here
> > > +_scratch_mkfs >>$seqres.full 2>&1
> > > +_scratch_mount "-o dax"
> >
> > Same here, dax is not required.
>
> Fixed in v2.
>
> >
> > > +
> > > +$XFS_IO_PROG -f -c "falloc 0 4M" $SCRATCH_MNT/testfile >> $seqres.full 2>&1
> > > +chmod 0644 $SCRATCH_MNT/testfile
> > > +chown $qa_user $SCRATCH_MNT/testfile
> >
> > Any specific reason to use $qa_user to run this test? Comments would be
> > great.
>
> Nope, just cargo-culting my way through my first xfstest. :) I've removed
> this for v2.
I think it's in a pretty good shape for "first fstests test" :)
>
> > Thanks,
> > Eryu
>
> Thanks for the review!
Thanks for adding new test!
Eryu
>
> > > +
> > > +_user_do "src/t_dax_stale_pmd $SCRATCH_MNT/testfile"
> > > +
> > > +# success, all done
> > > +echo "Silence is golden"
> > > +status=0
> > > +exit
> > > diff --git a/tests/generic/427.out b/tests/generic/427.out
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" 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: [PATCH v2 2/2] mm: skip HWPoisoned pages when onlining pages
From: Naoya Horiguchi @ 2017-04-26 3:13 UTC (permalink / raw)
To: Balbir Singh
Cc: Laurent Dufour, linux-kernel@vger.kernel.org, linux-mm@kvack.org,
akpm@linux-foundation.org
In-Reply-To: <1493172615.4828.3.camel@gmail.com>
On Wed, Apr 26, 2017 at 12:10:15PM +1000, Balbir Singh wrote:
> On Tue, 2017-04-25 at 16:27 +0200, Laurent Dufour wrote:
> > The commit b023f46813cd ("memory-hotplug: skip HWPoisoned page when
> > offlining pages") skip the HWPoisoned pages when offlining pages, but
> > this should be skipped when onlining the pages too.
> >
> > Signed-off-by: Laurent Dufour <ldufour@linux.vnet.ibm.com>
> > ---
> > mm/memory_hotplug.c | 4 ++++
> > 1 file changed, 4 insertions(+)
> >
> > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> > index 6fa7208bcd56..741ddb50e7d2 100644
> > --- a/mm/memory_hotplug.c
> > +++ b/mm/memory_hotplug.c
> > @@ -942,6 +942,10 @@ static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages,
> > if (PageReserved(pfn_to_page(start_pfn)))
> > for (i = 0; i < nr_pages; i++) {
> > page = pfn_to_page(start_pfn + i);
> > + if (PageHWPoison(page)) {
> > + ClearPageReserved(page);
>
> Why do we clear page reserved? Also if the page is marked PageHWPoison, it
> was never offlined to begin with? Or do you expect this to be set on newly
> hotplugged memory? Also don't we need to skip the entire pageblock?
If I read correctly, to "skip HWPoiosned page" in commit b023f46813cd means
that we skip the page status check for hwpoisoned pages *not* to prevent
memory offlining for memblocks with hwpoisoned pages. That means that
hwpoisoned pages can be offlined.
And another reason to clear PageReserved is that we could reuse the
hwpoisoned page after onlining back with replacing the broken DIMM.
In this usecase, we first do unpoisoning to clear PageHWPoison,
but it doesn't work if PageReserved is set. My simple testing shows
the BUG below in unpoisoning (without the ClearPageReserved):
Unpoison: Software-unpoisoned page 0x18000
BUG: Bad page state in process page-types pfn:18000
page:ffffda5440600000 count:0 mapcount:0 mapping: (null) index:0x70006b599
flags: 0x1fffc00004081a(error|uptodate|dirty|reserved|swapbacked)
raw: 001fffc00004081a 0000000000000000 000000070006b599 00000000ffffffff
raw: dead000000000100 dead000000000200 0000000000000000 0000000000000000
page dumped because: PAGE_FLAGS_CHECK_AT_FREE flag(s) set
bad because of flags: 0x800(reserved)
Thanks,
Naoya Horiguchi
--
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