* Re: [PATCH v4 5/6] driver core: Rename deferred_probe_timeout and make it global
From: Bjorn Andersson @ 2020-02-20 16:00 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Rob Herring, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson,
Pavel Machek, Len Brown, Todd Kjos, Liam Girdwood, Mark Brown,
Thierry Reding, Linus Walleij, Greg Kroah-Hartman, linux-pm
In-Reply-To: <20200220050440.45878-6-john.stultz@linaro.org>
On Wed 19 Feb 21:04 PST 2020, John Stultz wrote:
> Since other subsystems (like regulator) have similar arbitrary
> timeouts for how long they try to resolve driver dependencies,
> rename deferred_probe_timeout to driver_deferred_probe_timeout
> and set it as global, so it can be shared.
>
> Cc: Rob Herring <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: I92ee3b392004ecc9217c5337b54eda48c2d7f3ee
Change-Id...
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> v4:
> * Split out into its own patch as suggested by Mark
> * Renamed deferred_probe_timeout as suggested by Greg
> ---
> drivers/base/dd.c | 18 ++++++++++--------
> include/linux/device/driver.h | 1 +
> 2 files changed, 11 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 408e4da081da..39f1ce6d4f1c 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -229,17 +229,19 @@ DEFINE_SHOW_ATTRIBUTE(deferred_devs);
> * In the case of modules, set the default probe timeout to
> * 30 seconds to give userland some time to load needed modules
> */
> -static int deferred_probe_timeout = 30;
> +int driver_deferred_probe_timeout = 30;
> #else
> /* In the case of !modules, no probe timeout needed */
> -static int deferred_probe_timeout = -1;
> +int driver_deferred_probe_timeout = -1;
> #endif
> +EXPORT_SYMBOL_GPL(driver_deferred_probe_timeout);
> +
> static int __init deferred_probe_timeout_setup(char *str)
> {
> int timeout;
>
> if (!kstrtoint(str, 10, &timeout))
> - deferred_probe_timeout = timeout;
> + driver_deferred_probe_timeout = timeout;
> return 1;
> }
> __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
> @@ -259,10 +261,10 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
> */
> int driver_deferred_probe_check_state(struct device *dev)
> {
> - if (!initcalls_done || deferred_probe_timeout > 0)
> + if (!initcalls_done || driver_deferred_probe_timeout > 0)
> return -EPROBE_DEFER;
>
> - if (!deferred_probe_timeout) {
> + if (!driver_deferred_probe_timeout) {
> dev_WARN(dev, "deferred probe timeout, ignoring dependency");
> return -ETIMEDOUT;
> }
> @@ -276,7 +278,7 @@ static void deferred_probe_timeout_work_func(struct work_struct *work)
> {
> struct device_private *private, *p;
>
> - deferred_probe_timeout = 0;
> + driver_deferred_probe_timeout = 0;
> driver_deferred_probe_trigger();
> flush_work(&deferred_probe_work);
>
> @@ -310,9 +312,9 @@ static int deferred_probe_initcall(void)
> driver_deferred_probe_trigger();
> flush_work(&deferred_probe_work);
>
> - if (deferred_probe_timeout > 0) {
> + if (driver_deferred_probe_timeout > 0) {
> schedule_delayed_work(&deferred_probe_timeout_work,
> - deferred_probe_timeout * HZ);
> + driver_deferred_probe_timeout * HZ);
> }
> return 0;
> }
> diff --git a/include/linux/device/driver.h b/include/linux/device/driver.h
> index 5242afabfaba..ee7ba5b5417e 100644
> --- a/include/linux/device/driver.h
> +++ b/include/linux/device/driver.h
> @@ -236,6 +236,7 @@ driver_find_device_by_acpi_dev(struct device_driver *drv, const void *adev)
> }
> #endif
>
> +extern int driver_deferred_probe_timeout;
> void driver_deferred_probe_add(struct device *dev);
> int driver_deferred_probe_check_state(struct device *dev);
> void driver_init(void);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [dpdk-dev] [PATCH 4/4] ci: reorganise Travis jobs
From: David Marchand @ 2020-02-20 16:01 UTC (permalink / raw)
To: ci; +Cc: Thomas Monjalon, dev, Michael Santana, Aaron Conole,
Bruce Richardson
In-Reply-To: <f7tv9o19viw.fsf@dhcp-25.97.bos.redhat.com>
On Thu, Feb 20, 2020 at 3:35 PM Aaron Conole <aconole@redhat.com> wrote:
>
> David Marchand <david.marchand@redhat.com> writes:
>
> > On Thu, Feb 20, 2020 at 11:42 AM Thomas Monjalon <thomas@monjalon.net> wrote:
> >>
> >> 19/02/2020 22:39, Aaron Conole:
> >> > David Marchand <david.marchand@redhat.com> writes:
> >> >
> >> > > Let's prune the jobs list to limit the amount of time spent by the robot
> >> > > in Travis.
> >> > >
> >> > > Since meson enables automatically the relevant components, there is not
> >> > > much gain in testing with extra_packages vs required_packages only.
> >> > >
> >> > > For a given arch/compiler/env combination, compilation is first tested
> >> > > in all jobs that run tests or build the docs or run the ABI checks.
> >> > > In the same context, for jobs that accumulates running tests, building
> >> > > the docs etc..., those steps are independent and can be split to save
> >> > > some cpu on Travis.
> >> > >
> >> > > With this, we go down from 21 to 15 jobs.
> >> > >
> >> > > Note: this patch requires a flush of the existing caches in Travis.
> >> > >
> >> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> >> > > ---
> >> >
> >> > In general, I think the idea with required vs. extra was to have a build
> >> > that did the minimum required, and one that did all the packages (to
> >> > allow a minimum vs. full DPDK).
> >> >
> >> > At least, that's from
> >> > http://mails.dpdk.org/archives/dev/2019-January/124007.html
> >>
> >> I think the benefit of a minimum build is to have a quick report,
> >> and easy to setup.
> >
> > Yes, Travis serves as a first gate when submitting patches.
> > But since Travis is best effort/free, we can't have a full coverage.
> >
> >
> >> > Not sure if that's still something anyone cares about.
> >>
> >> Given that Travis knows how to satisfy the dependencies,
> >> and that we must wait for all jobs to finish,
> >> I don't see any benefit of a minimal setup.
> >
> > This minimal setup also tests that dpdk dependencies are correct.
> > If a change makes something rely on libX and libX is in the packages
> > always installed in Travis, the missing dependency would not get
> > caught.
> >
> > But here, this adds too many jobs.
> >
> > UNH, Intel and other CIs should step in and fill this kind of gap.
>
> Okay, makes sense to me. Are one of these CI providers offering to
> cover this?
Maybe it is already covered, the best is to ask, so sending to ci@dpdk.org.
For the CI guys, which packages are installed on the systems/vms that
do compilation tests?
Is it possible to have a summary of the different setups?
Thanks.
--
David Marchand
^ permalink raw reply
* ioctl seems to change errno behaviour in 5.6.0rc2
From: Antonio Larrosa @ 2020-02-20 16:02 UTC (permalink / raw)
To: Doug Gilbert, Jens Axboe; +Cc: linux-scsi
[-- Attachment #1: Type: text/plain, Size: 1110 bytes --]
Hello,
I noticed cdparanoia stopped working with kernel 5.6.0rc2 while it worked fine
with 5.5.2 .
Running as root `cdparanoia -v -d /dev/sr0 [0]` with 5.6.0rc2, gives the
following errors:
Testing /dev/sr0 for SCSI/MMC interface
no SG_IO support for device: /dev/sr0
Error trying to open /dev/sga exclusively (No such file or directory).
I checked that the sg module is loaded with both kernels and also did a diff of
the lsmod output with both kernels and didn't find anything suspicious.
After some tests, I did a small c application using code from cdparanoia where
it can be seen that the ioctl(fd, SG_IO, &hdr) call returns EINVAL in errno with
the 5.5.2 kernel but returns EFAULT with 5.6.0rc2 .
The code is attached and can be built just with `gcc test.c -o test` (note it's
hardcoded in main to use /dev/sr0, so it doesn't have any parameter).
Note that I'm not a cdparanoia developer (in fact, it seems to have been
unmaintained for many years), but I thought it might be interesting to report an
ioctl that changes the behaviour in different kernels.
Greetings,
--
Antonio Larrosa
[-- Attachment #2: test.c --]
[-- Type: text/x-csrc, Size: 1939 bytes --]
#include <stdio.h>
#include <unistd.h>
#include <errno.h>
#include <fcntl.h>
#include <string.h>
#include <sys/ioctl.h>
#include <scsi/sg.h>
int check_sgio(const char *device){
int fd;
struct sg_io_hdr hdr;
if (!device) return 0;
/* we don't really care what type of device it is -- if it can do
* SG_IO, then we'll put it through the normal mmc/atapi/etc tests
* later, but it's good enough for now. */
fd = open(device, O_RDWR|O_NONBLOCK);
if (fd < 0){
printf("Could not access device %s to test for SG_IO support\n",device);
return 0;
}
memset(&hdr, 0, sizeof (struct sg_io_hdr));
/* First try with interface_id = 'A'; for all but the sg case,
* that'll get us a -EINVAL if it supports SG_IO, and some other
* error for all other cases. */
hdr.interface_id = 'A';
if (ioctl(fd, SG_IO, &hdr)) {
int err=errno;
printf("errno: %d\n", err);
switch (err) {
case EINVAL: /* sr and ata give us EINVAL when SG_IO is
* supported but interface_id is bad. */
case ENOSYS: /* sg gives us ENOSYS when SG_IO is supported but
* interface_id is bad. IMHO, this is wrong and
* needs fixing in the kernel. */
close(fd);
return 1;
default: /* everything else gives ENOTTY, I think. I'm just
* going to be paranoid and reject everything else. */
close(fd);
return 0;
}
}
/* if we get here, something is dreadfuly wrong. ioctl(fd,SG_IO,&hdr)
* handled SG_IO, but took hdr.interface_id = 'A' as valid, and an empty
* command as good. Don't trust it. */
printf("closing\n");
close(fd);
return 0;
}
int main(int argc, char**argv)
{
const char *specialized_device = "/dev/sr0";
if(check_sgio(specialized_device)){
printf("SG_IO device: %s\n",specialized_device);
}else{
printf("no SG_IO support for device: %s\n",specialized_device);
}
return 0;
};
^ permalink raw reply
* Re: [PATCH v2 1/2] s390x: fix memleaks in cpu_finalize
From: Cornelia Huck @ 2020-02-20 15:59 UTC (permalink / raw)
To: pannengyuan
Cc: peter.maydell, zhang.zhanghailiang, alistair, qemu-devel,
mav2-rk.cave-ayland, qemu-arm, qemu-ppc, euler.robot,
edgar.iglesias, Richard Henderson, david
In-Reply-To: <20200217032127.46508-2-pannengyuan@huawei.com>
On Mon, 17 Feb 2020 11:21:26 +0800
<pannengyuan@huawei.com> wrote:
> From: Pan Nengyuan <pannengyuan@huawei.com>
>
> This patch fix memleaks when we call tests/qtest/cpu-plug-test on s390x. The leak stack is as follow:
>
> Direct leak of 48 byte(s) in 1 object(s) allocated from:
> #0 0x7fb43c7cd970 in __interceptor_calloc (/lib64/libasan.so.5+0xef970)
> #1 0x7fb43be2149d in g_malloc0 (/lib64/libglib-2.0.so.0+0x5249d)
> #2 0x558ba96da716 in timer_new_full /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:530
> #3 0x558ba96da716 in timer_new /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:551
> #4 0x558ba96da716 in timer_new_ns /mnt/sdb/qemu-new/qemu/include/qemu/timer.h:569
> #5 0x558ba96da716 in s390_cpu_initfn /mnt/sdb/qemu-new/qemu/target/s390x/cpu.c:285
> #6 0x558ba9c969ab in object_init_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:372
> #7 0x558ba9c9eb5f in object_initialize_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:516
> #8 0x558ba9c9f053 in object_new_with_type /mnt/sdb/qemu-new/qemu/qom/object.c:684
> #9 0x558ba967ede6 in s390x_new_cpu /mnt/sdb/qemu-new/qemu/hw/s390x/s390-virtio-ccw.c:64
> #10 0x558ba99764b3 in hmp_cpu_add /mnt/sdb/qemu-new/qemu/hw/core/machine-hmp-cmds.c:57
> #11 0x558ba9b1c27f in handle_hmp_command /mnt/sdb/qemu-new/qemu/monitor/hmp.c:1082
> #12 0x558ba96c1b02 in qmp_human_monitor_command /mnt/sdb/qemu-new/qemu/monitor/misc.c:142
>
> Reported-by: Euler Robot <euler.robot@huawei.com>
> Signed-off-by: Pan Nengyuan <pannengyuan@huawei.com>
> Cc: Richard Henderson <rth@twiddle.net>
> Cc: Cornelia Huck <cohuck@redhat.com>
> ---
> Changes v2 to v1:
> - Similarly to other cleanups, move timer_new into realize, then do
> timer_del in unrealize.
> ---
> target/s390x/cpu.c | 22 ++++++++++++++++++----
> 1 file changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/target/s390x/cpu.c b/target/s390x/cpu.c
> index cf84d307c6..f18dbc6fe4 100644
> --- a/target/s390x/cpu.c
> +++ b/target/s390x/cpu.c
> @@ -170,7 +170,12 @@ static void s390_cpu_realizefn(DeviceState *dev, Error **errp)
> S390CPUClass *scc = S390_CPU_GET_CLASS(dev);
> #if !defined(CONFIG_USER_ONLY)
> S390CPU *cpu = S390_CPU(dev);
> + cpu->env.tod_timer =
> + timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_tod_timer, cpu);
> + cpu->env.cpu_timer =
> + timer_new_ns(QEMU_CLOCK_VIRTUAL, s390x_cpu_timer, cpu);
I may be missing something, but what cleans up the timers if we fail
further down in this function? I don't think the unrealize callback is
invoked by the core in case of error?
Also, as a matter of personal preference, I think it would be better to
initialize the timers in the !CONFIG_USER_ONLY section further below,
rather than in the variable declaration section.
> #endif
> +
> Error *err = NULL;
>
> /* the model has to be realized before qemu_init_vcpu() due to kvm */
^ permalink raw reply
* [PATCH RESEND v6 10/16] userfaultfd: Don't retake mmap_sem to emulate NOPAGE
From: Peter Xu @ 2020-02-20 16:02 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Peter Xu, Martin Cracauer, Mike Rapoport, Hugh Dickins,
Jerome Glisse, Kirill A . Shutemov, Matthew Wilcox,
Pavel Emelyanov, Brian Geffon, Maya Gokhale, Denis Plotnikov,
Andrea Arcangeli, Johannes Weiner, Dr . David Alan Gilbert,
Linus Torvalds, Mike Kravetz, Marty McFadden, David Hildenbrand,
Bobby Powers, Mel Gorman
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
This patch removes the risk path in handle_userfault() then we will be
sure that the callers of handle_mm_fault() will know that the VMAs
might have changed. Meanwhile with previous patch we don't lose
responsiveness as well since the core mm code now can handle the
nonfatal userspace signals even if we return VM_FAULT_RETRY.
Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Reviewed-by: Jerome Glisse <jglisse@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
fs/userfaultfd.c | 24 ------------------------
1 file changed, 24 deletions(-)
diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c
index 37df7c9eedb1..888272621f38 100644
--- a/fs/userfaultfd.c
+++ b/fs/userfaultfd.c
@@ -524,30 +524,6 @@ vm_fault_t handle_userfault(struct vm_fault *vmf, unsigned long reason)
__set_current_state(TASK_RUNNING);
- if (return_to_userland) {
- if (signal_pending(current) &&
- !fatal_signal_pending(current)) {
- /*
- * If we got a SIGSTOP or SIGCONT and this is
- * a normal userland page fault, just let
- * userland return so the signal will be
- * handled and gdb debugging works. The page
- * fault code immediately after we return from
- * this function is going to release the
- * mmap_sem and it's not depending on it
- * (unlike gup would if we were not to return
- * VM_FAULT_RETRY).
- *
- * If a fatal signal is pending we still take
- * the streamlined VM_FAULT_RETRY failure path
- * and there's no need to retake the mmap_sem
- * in such case.
- */
- down_read(&mm->mmap_sem);
- ret = VM_FAULT_NOPAGE;
- }
- }
-
/*
* Here we race with the list_del; list_add in
* userfaultfd_ctx_read(), however because we don't ever run
--
2.24.1
^ permalink raw reply related
* Re: [PATCH] console: make QMP screendump use coroutine
From: Markus Armbruster @ 2020-02-20 16:01 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Kevin Wolf, QEMU, David Alan Gilbert, Gerd Hoffmann
In-Reply-To: <CAJ+F1C+M3yPreBLOHXkt16b5aghesT7qYkEPbS_3Dm7vGTaMKA@mail.gmail.com>
Cc: David for questions regarding the HMP core. David, please look for
"Is HMP blocking the main loop a problem?"
Marc-André Lureau <marcandre.lureau@gmail.com> writes:
> Hi
>
> On Thu, Feb 20, 2020 at 8:49 AM Markus Armbruster <armbru@redhat.com> wrote:
>>
>> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>>
>> > Thanks to the QMP coroutine support, the screendump handler can
>> > trigger a graphic_hw_update(), yield and let the main loop run until
>> > update is done. Then the handler is resumed, and the ppm_save() will
>> > write the screen image to disk in the coroutine context (thus
>> > non-blocking).
>> >
>> > For now, HMP doesn't have coroutine support, so it remains potentially
>> > outdated or glitched.
>> >
>> > Fixes:
>> > https://bugzilla.redhat.com/show_bug.cgi?id=1230527
>> >
>> > Based-on: <20200109183545.27452-2-kwolf@redhat.com>
>> >
>> > Cc: Kevin Wolf <kwolf@redhat.com>
>> > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > ---
>> > qapi/ui.json | 3 ++-
>> > ui/console.c | 35 +++++++++++++++++++++++++++--------
>> > ui/trace-events | 2 +-
>> > 3 files changed, 30 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/qapi/ui.json b/qapi/ui.json
>> > index e04525d8b4..d941202f34 100644
>> > --- a/qapi/ui.json
>> > +++ b/qapi/ui.json
>> > @@ -96,7 +96,8 @@
>> > #
>> > ##
>> > { 'command': 'screendump',
>> > - 'data': {'filename': 'str', '*device': 'str', '*head': 'int'} }
>> > + 'data': {'filename': 'str', '*device': 'str', '*head': 'int'},
>> > + 'coroutine': true }
>> >
>> > ##
>> > # == Spice
>> > diff --git a/ui/console.c b/ui/console.c
>> > index ac79d679f5..db184b473f 100644
>> > --- a/ui/console.c
>> > +++ b/ui/console.c
>> > @@ -167,6 +167,7 @@ struct QemuConsole {
>> > QEMUFIFO out_fifo;
>> > uint8_t out_fifo_buf[16];
>> > QEMUTimer *kbd_timer;
>> > + Coroutine *screendump_co;
>> >
>> > QTAILQ_ENTRY(QemuConsole) next;
>> > };
>> > @@ -194,7 +195,6 @@ static void dpy_refresh(DisplayState *s);
>> > static DisplayState *get_alloc_displaystate(void);
>> > static void text_console_update_cursor_timer(void);
>> > static void text_console_update_cursor(void *opaque);
>> > -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp);
>> >
>> > static void gui_update(void *opaque)
>> > {
>> > @@ -263,6 +263,9 @@ static void gui_setup_refresh(DisplayState *ds)
>> >
>> > void graphic_hw_update_done(QemuConsole *con)
>> > {
>> > + if (con && con->screendump_co) {
>>
>> How can !con happen?
>
> I don't think it can happen anymore (the patch evolved over several
> years, this is probably a left-over). In any case, it doesn't hurt.
I hate such dead checks, because they make me assume they can actually
happen. Incorrect assumptions breed bugs.
But I'm willing to defer to the maintainer here. Gerd?
>> > + aio_co_wake(con->screendump_co);
>> > + }
>> > }
>> >
>> > void graphic_hw_update(QemuConsole *con)
>> > @@ -310,16 +313,16 @@ void graphic_hw_invalidate(QemuConsole *con)
>> > }
>> > }
>> >
>> > -static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
>> > +static bool ppm_save(int fd, pixman_image_t *image, Error **errp)
>> > {
>> > - int width = pixman_image_get_width(ds->image);
>> > - int height = pixman_image_get_height(ds->image);
>> > + int width = pixman_image_get_width(image);
>> > + int height = pixman_image_get_height(image);
>> > g_autoptr(Object) ioc = OBJECT(qio_channel_file_new_fd(fd));
>> > g_autofree char *header = NULL;
>> > g_autoptr(pixman_image_t) linebuf = NULL;
>> > int y;
>> >
>> > - trace_ppm_save(fd, ds);
>> > + trace_ppm_save(fd, image);
>> >
>> > header = g_strdup_printf("P6\n%d %d\n%d\n", width, height, 255);
>> > if (qio_channel_write_all(QIO_CHANNEL(ioc),
>> > @@ -329,7 +332,7 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
>> >
>> > linebuf = qemu_pixman_linebuf_create(PIXMAN_BE_r8g8b8, width);
>> > for (y = 0; y < height; y++) {
>> > - qemu_pixman_linebuf_fill(linebuf, ds->image, width, 0, y);
>> > + qemu_pixman_linebuf_fill(linebuf, image, width, 0, y);
>> > if (qio_channel_write_all(QIO_CHANNEL(ioc),
>> > (char *)pixman_image_get_data(linebuf),
>> > pixman_image_get_stride(linebuf), errp) < 0) {
>>
>> Looks like an unrelated optimization / simplification. If I was
>> maintainer, I'd ask for a separate patch.
>
> I can be split, but it's related. We should pass a reference to
> pixman_image_t, rather than a pointer to DisplaySurface, as the
> underlying image may change over time, and would result in corrupted
> coroutine save or worse.
Work that into your commit message, please. Might be easier if you
split, but that's between you and the maintainer :)
>> > @@ -340,11 +343,18 @@ static bool ppm_save(int fd, DisplaySurface *ds, Error **errp)
>> > return true;
>> > }
>> >
>> > +static void graphic_hw_update_bh(void *con)
>> > +{
>> > + graphic_hw_update(con);
>> > +}
>> > +
>> > +/* may be called in coroutine context or not */
>>
>> Hmm.
>>
>> Even though the QMP core always calls in coroutine context, the comment
>> is correct: hmp_screendump() calls it outside coroutine context.
>> Because of that...
>>
>> > void qmp_screendump(const char *filename, bool has_device, const char *device,
>> > bool has_head, int64_t head, Error **errp)
>> > {
>> > QemuConsole *con;
>> > DisplaySurface *surface;
>> > + g_autoptr(pixman_image_t) image = NULL;
>> > int fd;
>> >
>> > if (has_device) {
>> > @@ -365,7 +375,15 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
>> > }
>> > }
>> >
>> > - graphic_hw_update(con);
>> > + if (qemu_in_coroutine()) {
>> > + assert(!con->screendump_co);
>>
>> What if multiple QMP monitors simultaneously screendump? Hmm, it works
>> because all execute one after another in the same coroutine
>> qmp_dispatcher_co. Implicit mutual exclusion.
>>
>> Executing them one after another is bad, because it lets an ill-behaved
>> QMP command starve *all* QMP monitors. We do it only out of
>> (reasonable!) fear of implicit mutual exclusion requirements like the
>> one you add.
>>
>> Let's not add more if we can help it.
>
> The situation is not worse than the current blocking handling.
Really?
What makes executing multiple qmp_screendump() concurrently (in separate
threads) or interleaved (in separate coroutines in the same thread)
unsafe before this patch?
>> Your screendump_co is per QemuConsole instead of per QMP monitor only
>> because you need to find the coroutine in graphic_hw_update_done(). Can
>> we somehow pass it via function arguments?
>
> I think it could be done later, so I suggest a TODO.
We should avoid making our dependence on implicit mutual exclusion
worse. When we do it anyway, a big, fat, ugly comment is definitely
called for.
>> In case avoiding the mutual exclusion is impractical: please explain it
>> in a comment to make it somewhat less implicit.
It is anything but: see appended patch.
>> > + con->screendump_co = qemu_coroutine_self();
>> > + aio_bh_schedule_oneshot(qemu_get_aio_context(),
>> > + graphic_hw_update_bh, con);
>> > + qemu_coroutine_yield();
>> > + con->screendump_co = NULL;
>> > + }
>> > +
>>
>> ... the command handler needs extra code to cope with either. Is this
>> really what we want for coroutine QMP command handlers? We'll acquire
>> more of them, and I'd hate to make each one run both in and outside
>> coroutine context. Shouldn't we let the HMP core take care of this? Or
>> at least have some common infrastructure these handlers can use?
>
> We have several functions that have this dual support, for ex QIO.
>
> Changing both QMP & HMP commands to run in coroutine is likely
> additional work that we may not care at this point.
If it wasn't for non-QMP calls (typically HMP, but also CLI), then
handlers for QMP commands with 'coroutine': true could be coroutine_fn.
So far, coroutine_fn is merely documentation. Perhaps it can guide a
checker for "do coroutine stuff only in coroutines" some day. Would be
nice, because the coroutine calls are often buried deep, and far away
from the code that ensures they run in a coroutine.
My point is: marking functions coroutine_fn is good. We should do it
more. We should try to avoid stuff that hinders doing it more.
> I propose to leave a TODO, once we have several similar QMP & HMP mix
> cases we can try to find a common HMP solution to make the code
> simpler in QMP handler.
Collecting several users before building infrastructure makes sense when
the design of the infrastructure isn't obvious, or when the need for it
is in doubt.
Neither is the case for running QMP handlers in a coroutine: QMP
commands blocking the main loop is without doubt a problem we need to
solve, and the way to solve it was obvious enough for Kevin to do it
with one user: block_resize. A second one quickly followed: screendump.
The only part that's different for HMP, I think, is "need".
Is HMP blocking the main loop a problem?
If yes, is it serious enough to justify solving it?
If yes, then putting workarounds into QMP handlers now so we can put off
solving it some more is taking on technical debt.
> I don't know if this is going to be a common pattern, we may end up
> with conversions that can run both without explicit handling (like the
> ppm_save() function, thanks to QIO).
Yes, such handlers may exist. Running them out of coroutine context
would throw away their capability not to block the event loop, though,
wouldn't it?
>> Why is it okay not to call graphic_hw_update() anymore when
>> !qemu_in_coroutine()?
>
> You could call it, but then you should wait for completion by
> reentering the main loop (that was the point of my earlier qapi-async
> series)
Possibly stupid question: why is it necessary before this patch
(assuming it is, since we call it), and why is it no longer necessary
after?
Oh, see below.
>>
>> If qemu_in_coroutine(), we now run graphic_hw_update() in a bottom half,
>> then yield until the update completes (see graphic_hw_update_done()
>> above). Can you explain the need for the bottom half?
>
> At least spice rendering is done in a separate thread, completion is async.
When I ask a stupid question like this one, I'm really after the "for
dummies" explanation. I may be able to figure it out myself with some
effort, but having to put that kind of effort into patch review makes me
grumpy, and once I'm sufficiently grumpy, I don't want to review patches
anymore, let alone merge them.
Oh well, let me try. We're in the main loop. We want to trigger a
"graphics update" (whatever that is, doesn't matter) and wait for it to
complete without blocking the main loop.
"Without blocking the main loop" means the QMP coroutine yields. I'd
naively expect
QMP coroutine: schedule the job; yield
whatever gets scheduled: complete the job; wake up QMP coroutine
Now let's examine the "graphics update" interface.
GraphicHwOps callback gfx_update() comes in two flavours:
* synchronous: complete the job, return
* asynchronous: start the job, return immediately,
graphic_hw_update_done() will get called on job completion
graphic_hw_update() partly hides the difference:
* synchronous: complete the job, call graphic_hw_update_done()
* asynchronous: start the job, return immediately,
graphic_hw_update_done() will get called on job completion
This lets you treat the synchronous case more like the asynchronous
case.
You use graphic_hw_update_done() to wake up the QMP coroutine.
I think I can now answer my question "why is it okay not to call
graphic_hw_update() anymore when !qemu_in_coroutine()?"
Before the patch, both QMP and HMP:
* with synchronous gfx_update(): we update before we write out the
screendump. The screendump is up-to-date. Both update and write out
block the main loop.
* with asynchronous gfx_update(): we start updating, but don't wait for
it to complete before we write out. This is wrong. Write out blocks
the main loop, but update does not.
After the patch:
* QMP with either gfx_update(): we update before we write out the
screendump. The screendump is up-to-date. Neither update nor write
out block the main loop. Improvement.
* HMP with either gfx_update(): we don't update before we write out.
Similarly wrong for asynchronous gfx_update(), regression for
synchronous gfx_update(). Write out blocks the main loop as before.
Why is the regression okay?
Back to the bottom half. The way graphic_hw_update() works, the QMP
coroutine can't schedule then yield. It *has* to yield before
graphic_hw_update() runs. That means we need a bottom half.
Alright, I'm officially grumpy now.
Please explain the need for a bottom half in a comment.
>> > surface = qemu_console_surface(con);
>> > if (!surface) {
>> > error_setg(errp, "no surface");
>> > @@ -379,7 +397,8 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
>> > return;
>> > }
>> >
>> > - if (!ppm_save(fd, surface, errp)) {
>> > + image = pixman_image_ref(surface->image);
>> > + if (!ppm_save(fd, image, errp)) {
>> > qemu_unlink(filename);
>> > }
>> > }
>> > diff --git a/ui/trace-events b/ui/trace-events
>> > index 0dcda393c1..e8726fc969 100644
>> > --- a/ui/trace-events
>> > +++ b/ui/trace-events
>> > @@ -15,7 +15,7 @@ displaysurface_create_pixman(void *display_surface) "surface=%p"
>> > displaysurface_free(void *display_surface) "surface=%p"
>> > displaychangelistener_register(void *dcl, const char *name) "%p [ %s ]"
>> > displaychangelistener_unregister(void *dcl, const char *name) "%p [ %s ]"
>> > -ppm_save(int fd, void *display_surface) "fd=%d surface=%p"
>> > +ppm_save(int fd, void *image) "fd=%d image=%p"
>> >
>> > # gtk.c
>> > # gtk-gl-area.c
diff --git a/ui/console.c b/ui/console.c
index 57df3a5439..c5aabf5a5f 100644
--- a/ui/console.c
+++ b/ui/console.c
@@ -167,7 +167,6 @@ struct QemuConsole {
QEMUFIFO out_fifo;
uint8_t out_fifo_buf[16];
QEMUTimer *kbd_timer;
- Coroutine *screendump_co;
QTAILQ_ENTRY(QemuConsole) next;
};
@@ -261,14 +260,20 @@ static void gui_setup_refresh(DisplayState *ds)
ds->have_text = have_text;
}
-void graphic_hw_update_done(QemuConsole *con)
+static void graphic_hw_update_done_with_kick(QemuConsole *con,
+ Coroutine *kick_me)
{
- if (con && con->screendump_co) {
- aio_co_wake(con->screendump_co);
+ if (kick_me) {
+ aio_co_wake(kick_me);
}
}
-void graphic_hw_update(QemuConsole *con)
+void graphic_hw_update_done(QemuConsole *con)
+{
+ graphic_hw_update_done_with_kick(con, NULL);
+}
+
+static void graphic_hw_update_with_kick(QemuConsole *con, Coroutine *kick_me)
{
bool async = false;
if (!con) {
@@ -279,10 +284,15 @@ void graphic_hw_update(QemuConsole *con)
async = con->hw_ops->gfx_update_async;
}
if (!async) {
- graphic_hw_update_done(con);
+ graphic_hw_update_done_with_kick(con, kick_me);
}
}
+void graphic_hw_update(QemuConsole *con)
+{
+ graphic_hw_update_with_kick(con, NULL);
+}
+
void graphic_hw_gl_block(QemuConsole *con, bool block)
{
assert(con != NULL);
@@ -343,9 +353,16 @@ static bool ppm_save(int fd, pixman_image_t *image, Error **errp)
return true;
}
-static void graphic_hw_update_bh(void *con)
+typedef struct {
+ QemuConsole *con;
+ Coroutine *kick_me;
+} UpdateBHargs;
+
+static void graphic_hw_update_bh(void *p)
{
- graphic_hw_update(con);
+ UpdateBHargs *args = p;
+
+ graphic_hw_update_with_kick(args->con, args->kick_me);
}
/* may be called in coroutine context or not */
@@ -376,12 +393,13 @@ void qmp_screendump(const char *filename, bool has_device, const char *device,
}
if (qemu_in_coroutine()) {
- assert(!con->screendump_co);
- con->screendump_co = qemu_coroutine_self();
+ UpdateBHargs args = {
+ .con = con,
+ .kick_me = qemu_coroutine_self(),
+ };
aio_bh_schedule_oneshot(qemu_get_aio_context(),
- graphic_hw_update_bh, con);
+ graphic_hw_update_bh, &args);
qemu_coroutine_yield();
- con->screendump_co = NULL;
}
surface = qemu_console_surface(con);
^ permalink raw reply related
* [PATCH RESEND v6 11/16] mm: Introduce FAULT_FLAG_DEFAULT
From: Peter Xu @ 2020-02-20 16:02 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Peter Xu, Martin Cracauer, Mike Rapoport, Hugh Dickins,
Jerome Glisse, Kirill A . Shutemov, Matthew Wilcox,
Pavel Emelyanov, Brian Geffon, Maya Gokhale, Denis Plotnikov,
Andrea Arcangeli, Johannes Weiner, Dr . David Alan Gilbert,
Linus Torvalds, Mike Kravetz, Marty McFadden, David Hildenbrand,
Bobby Powers, Mel Gorman
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
Although there're tons of arch-specific page fault handlers, most of
them are still sharing the same initial value of the page fault flags.
Say, merely all of the page fault handlers would allow the fault to be
retried, and they also allow the fault to respond to SIGKILL.
Let's define a default value for the fault flags to replace those
initial page fault flags that were copied over. With this, it'll be
far easier to introduce new fault flag that can be used by all the
architectures instead of touching all the archs.
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/alpha/mm/fault.c | 2 +-
arch/arc/mm/fault.c | 2 +-
arch/arm/mm/fault.c | 2 +-
arch/arm64/mm/fault.c | 2 +-
arch/hexagon/mm/vm_fault.c | 2 +-
arch/ia64/mm/fault.c | 2 +-
arch/m68k/mm/fault.c | 2 +-
arch/microblaze/mm/fault.c | 2 +-
arch/mips/mm/fault.c | 2 +-
arch/nds32/mm/fault.c | 2 +-
arch/nios2/mm/fault.c | 2 +-
arch/openrisc/mm/fault.c | 2 +-
arch/parisc/mm/fault.c | 2 +-
arch/powerpc/mm/fault.c | 2 +-
arch/riscv/mm/fault.c | 2 +-
arch/s390/mm/fault.c | 2 +-
arch/sh/mm/fault.c | 2 +-
arch/sparc/mm/fault_32.c | 2 +-
arch/sparc/mm/fault_64.c | 2 +-
arch/um/kernel/trap.c | 2 +-
arch/unicore32/mm/fault.c | 2 +-
arch/x86/mm/fault.c | 2 +-
arch/xtensa/mm/fault.c | 2 +-
include/linux/mm.h | 7 +++++++
24 files changed, 30 insertions(+), 23 deletions(-)
diff --git a/arch/alpha/mm/fault.c b/arch/alpha/mm/fault.c
index aea33b599037..fcfa229cc1e7 100644
--- a/arch/alpha/mm/fault.c
+++ b/arch/alpha/mm/fault.c
@@ -89,7 +89,7 @@ do_page_fault(unsigned long address, unsigned long mmcsr,
const struct exception_table_entry *fixup;
int si_code = SEGV_MAPERR;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
/* As of EV6, a load into $31/$f31 is a prefetch, and never faults
(or is suppressed by the PALcode). Support that for older CPUs
diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c
index 6eb821a59b49..643fad774071 100644
--- a/arch/arc/mm/fault.c
+++ b/arch/arc/mm/fault.c
@@ -100,7 +100,7 @@ void do_page_fault(unsigned long address, struct pt_regs *regs)
(regs->ecr_cause == ECR_C_PROTV_INST_FETCH))
exec = 1;
- flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ flags = FAULT_FLAG_DEFAULT;
if (user_mode(regs))
flags |= FAULT_FLAG_USER;
if (write)
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 937b81ff8649..18ef0b143ac2 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -241,7 +241,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
struct mm_struct *mm;
int sig, code;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
if (kprobe_page_fault(regs, fsr))
return 0;
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 6f4b69d712b1..cbb29a43aa7f 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -446,7 +446,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
struct mm_struct *mm = current->mm;
vm_fault_t fault, major = 0;
unsigned long vm_flags = VM_READ | VM_WRITE | VM_EXEC;
- unsigned int mm_flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int mm_flags = FAULT_FLAG_DEFAULT;
if (kprobe_page_fault(regs, esr))
return 0;
diff --git a/arch/hexagon/mm/vm_fault.c b/arch/hexagon/mm/vm_fault.c
index d19beaf11b4c..d9e15d941bdb 100644
--- a/arch/hexagon/mm/vm_fault.c
+++ b/arch/hexagon/mm/vm_fault.c
@@ -41,7 +41,7 @@ void do_page_fault(unsigned long address, long cause, struct pt_regs *regs)
int si_code = SEGV_MAPERR;
vm_fault_t fault;
const struct exception_table_entry *fixup;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
/*
* If we're in an interrupt or have no user context,
diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index 211b4f439384..b5aa4e80c762 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -65,7 +65,7 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
struct mm_struct *mm = current->mm;
unsigned long mask;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
mask = ((((isr >> IA64_ISR_X_BIT) & 1UL) << VM_EXEC_BIT)
| (((isr >> IA64_ISR_W_BIT) & 1UL) << VM_WRITE_BIT));
diff --git a/arch/m68k/mm/fault.c b/arch/m68k/mm/fault.c
index a455e202691b..182799fd9987 100644
--- a/arch/m68k/mm/fault.c
+++ b/arch/m68k/mm/fault.c
@@ -71,7 +71,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
struct mm_struct *mm = current->mm;
struct vm_area_struct * vma;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
pr_debug("do page fault:\nregs->sr=%#x, regs->pc=%#lx, address=%#lx, %ld, %p\n",
regs->sr, regs->pc, address, error_code, mm ? mm->pgd : NULL);
diff --git a/arch/microblaze/mm/fault.c b/arch/microblaze/mm/fault.c
index cdde01dcdfc3..32da02778a63 100644
--- a/arch/microblaze/mm/fault.c
+++ b/arch/microblaze/mm/fault.c
@@ -91,7 +91,7 @@ void do_page_fault(struct pt_regs *regs, unsigned long address,
int code = SEGV_MAPERR;
int is_write = error_code & ESR_S;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
regs->ear = address;
regs->esr = error_code;
diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c
index 0ee6fafc57bc..f177da67d940 100644
--- a/arch/mips/mm/fault.c
+++ b/arch/mips/mm/fault.c
@@ -44,7 +44,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write,
const int field = sizeof(unsigned long) * 2;
int si_code;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
static DEFINE_RATELIMIT_STATE(ratelimit_state, 5 * HZ, 10);
diff --git a/arch/nds32/mm/fault.c b/arch/nds32/mm/fault.c
index 0e63f81eff5b..2810a4e5ab27 100644
--- a/arch/nds32/mm/fault.c
+++ b/arch/nds32/mm/fault.c
@@ -80,7 +80,7 @@ void do_page_fault(unsigned long entry, unsigned long addr,
int si_code;
vm_fault_t fault;
unsigned int mask = VM_READ | VM_WRITE | VM_EXEC;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
error_code = error_code & (ITYPE_mskINST | ITYPE_mskETYPE);
tsk = current;
diff --git a/arch/nios2/mm/fault.c b/arch/nios2/mm/fault.c
index 704ace8ca0ee..c38bea4220fb 100644
--- a/arch/nios2/mm/fault.c
+++ b/arch/nios2/mm/fault.c
@@ -47,7 +47,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long cause,
struct mm_struct *mm = tsk->mm;
int code = SEGV_MAPERR;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
cause >>= 2;
diff --git a/arch/openrisc/mm/fault.c b/arch/openrisc/mm/fault.c
index 85c7eb0c0186..30d5c51e9d40 100644
--- a/arch/openrisc/mm/fault.c
+++ b/arch/openrisc/mm/fault.c
@@ -50,7 +50,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long address,
struct vm_area_struct *vma;
int si_code;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
tsk = current;
diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
index f9be1d1cb43f..8e88e5c5f26a 100644
--- a/arch/parisc/mm/fault.c
+++ b/arch/parisc/mm/fault.c
@@ -274,7 +274,7 @@ void do_page_fault(struct pt_regs *regs, unsigned long code,
if (!mm)
goto no_context;
- flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ flags = FAULT_FLAG_DEFAULT;
if (user_mode(regs))
flags |= FAULT_FLAG_USER;
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 0868172ce4e3..d7e1f8dc7e4c 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -434,7 +434,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
{
struct vm_area_struct * vma;
struct mm_struct *mm = current->mm;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
int is_exec = TRAP(regs) == 0x400;
int is_user = user_mode(regs);
int is_write = page_fault_is_write(error_code);
diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c
index 1d3869e9ddef..a252d9e38561 100644
--- a/arch/riscv/mm/fault.c
+++ b/arch/riscv/mm/fault.c
@@ -30,7 +30,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs)
struct vm_area_struct *vma;
struct mm_struct *mm;
unsigned long addr, cause;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
int code = SEGV_MAPERR;
vm_fault_t fault;
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 179cf92a56e5..551ac311bd35 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -429,7 +429,7 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access)
address = trans_exc_code & __FAIL_ADDR_MASK;
perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
- flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ flags = FAULT_FLAG_DEFAULT;
if (user_mode(regs))
flags |= FAULT_FLAG_USER;
if (access == VM_WRITE || (trans_exc_code & store_indication) == 0x400)
diff --git a/arch/sh/mm/fault.c b/arch/sh/mm/fault.c
index eb4048ad0b38..d9c8f2d00a54 100644
--- a/arch/sh/mm/fault.c
+++ b/arch/sh/mm/fault.c
@@ -380,7 +380,7 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
struct mm_struct *mm;
struct vm_area_struct * vma;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
tsk = current;
mm = tsk->mm;
diff --git a/arch/sparc/mm/fault_32.c b/arch/sparc/mm/fault_32.c
index 6efbeb227644..a91b0c2d84f8 100644
--- a/arch/sparc/mm/fault_32.c
+++ b/arch/sparc/mm/fault_32.c
@@ -168,7 +168,7 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
int from_user = !(regs->psr & PSR_PS);
int code;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
if (text_fault)
address = regs->pc;
diff --git a/arch/sparc/mm/fault_64.c b/arch/sparc/mm/fault_64.c
index dd1ed6555831..30653418a672 100644
--- a/arch/sparc/mm/fault_64.c
+++ b/arch/sparc/mm/fault_64.c
@@ -271,7 +271,7 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
int si_code, fault_code;
vm_fault_t fault;
unsigned long address, mm_rss;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
fault_code = get_thread_fault_code();
diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
index 818553064f04..c59ad37eacda 100644
--- a/arch/um/kernel/trap.c
+++ b/arch/um/kernel/trap.c
@@ -33,7 +33,7 @@ int handle_page_fault(unsigned long address, unsigned long ip,
pmd_t *pmd;
pte_t *pte;
int err = -EFAULT;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
*code_out = SEGV_MAPERR;
diff --git a/arch/unicore32/mm/fault.c b/arch/unicore32/mm/fault.c
index 59d0e6ec2cfc..34a90453ca18 100644
--- a/arch/unicore32/mm/fault.c
+++ b/arch/unicore32/mm/fault.c
@@ -202,7 +202,7 @@ static int do_pf(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
struct mm_struct *mm;
int sig, code;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
tsk = current;
mm = tsk->mm;
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 6a00bc8d047f..7b6f65333355 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1288,7 +1288,7 @@ void do_user_addr_fault(struct pt_regs *regs,
struct task_struct *tsk;
struct mm_struct *mm;
vm_fault_t fault, major = 0;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
tsk = current;
mm = tsk->mm;
diff --git a/arch/xtensa/mm/fault.c b/arch/xtensa/mm/fault.c
index 59515905d4ad..7d196dc951e8 100644
--- a/arch/xtensa/mm/fault.c
+++ b/arch/xtensa/mm/fault.c
@@ -43,7 +43,7 @@ void do_page_fault(struct pt_regs *regs)
int is_write, is_exec;
vm_fault_t fault;
- unsigned int flags = FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_KILLABLE;
+ unsigned int flags = FAULT_FLAG_DEFAULT;
code = SEGV_MAPERR;
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 52269e56c514..08ca35d3c341 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -388,6 +388,13 @@ extern pgprot_t protection_map[16];
#define FAULT_FLAG_REMOTE 0x80 /* faulting for non current tsk/mm */
#define FAULT_FLAG_INSTRUCTION 0x100 /* The fault was during an instruction fetch */
+/*
+ * The default fault flags that should be used by most of the
+ * arch-specific page fault handlers.
+ */
+#define FAULT_FLAG_DEFAULT (FAULT_FLAG_ALLOW_RETRY | \
+ FAULT_FLAG_KILLABLE)
+
#define FAULT_FLAG_TRACE \
{ FAULT_FLAG_WRITE, "WRITE" }, \
{ FAULT_FLAG_MKWRITE, "MKWRITE" }, \
--
2.24.1
^ permalink raw reply related
* [yocto] Lots of basehash related errors
From: Dmitri Toubelis @ 2020-02-20 16:02 UTC (permalink / raw)
To: yocto
[-- Attachment #1: Type: text/plain, Size: 2916 bytes --]
Hi,
I'm migrating from yocto morty to zeus and I'm receiving a whole lot of errors like this:
>
> ERROR: When reparsing
> /srv/yocto/poky/meta-loopedge/meta-loopedge-dist/recipes-core/images/loopedge-std.bb:do_image_wic,
> the basehash value changed from
> 8dd96e09d0b7defa552e586e626933ca37ace5180918ea65addbfcb6c1247b1c to
> e38fae3f400b3e3de114fcd668d46a1f7c3eec436ec72962e78df906714a6fb0. The
> metadata is not deterministic and this needs to be fixed.
> ERROR: The following commands may help:
> ERROR: $ bitbake loopedge-std -cdo_image_wic -Snone
> ERROR: Then:
> ERROR: $ bitbake loopedge-std -cdo_image_wic -Sprintdiff
>
> ERROR: When reparsing
> /srv/yocto/poky/meta-loopedge/meta-loopedge-dist/recipes-core/images/loopedge-std.bb:do_image_ext4,
> the basehash value changed from
> a8209ab35324ce59bb193b80871c12c492f69f42fd97b03801165bd4a12670f6 to
> 1ab2d25ef217fe87b4cce1106d122acd4286043b04dcd74d98df30a01aa6a0b9. The
> metadata is not deterministic and this needs to be fixed.
> ERROR: The following commands may help:
> ERROR: $ bitbake loopedge-std -cdo_image_ext4 -Snone
> ERROR: Then:
> ERROR: $ bitbake loopedge-std -cdo_image_ext4 -Sprintdiff
>
> ERROR: When reparsing
> /srv/yocto/poky/meta-loopedge/meta-loopedge-dist/recipes-core/images/loopedge-std.bb:do_image_tar,
> the basehash value changed from
> c5ab62cac832e502a338d59124efc690e66560a4e877bc4ba3487c3a734c2497 to
> bb7ca72863614cb5c9915eb502259b1ffa8b98992f7ad3280d1e049a1824b930. The
> metadata is not deterministic and this needs to be fixed.
> ERROR: The following commands may help:
> ERROR: $ bitbake loopedge-std -cdo_image_tar -Snone
> ERROR: Then:
> ERROR: $ bitbake loopedge-std -cdo_image_tar -Sprintdiff
>
I search around for answers and there are here are reasons and solutions for this that I found:
- to make sure any date related variables are excluded from basehash via `do_task_name[vardepsexclude] = "DATE DATETIME"`
- clears state cache with `bitbake image -c cleansstate`
- delete tmp directory and build from scratch
Here is my observation and interpretation:
- this messages occur when running with pristine build directory, i.e. it only contains 2 files in `conf` dir - `local.conf` and `bblatyers.conf`, so I can rule out contamination from a previous run.
- same messages reapeat over and over totalling ~900 errors at the end of the run
- I have few custom classes and I removed them from the image to rule out contamination from my own code.
- Tasks that give this error are coming from image.bbclass from poky and none of them have been altered in any way.
- The image build runs through the end but because bitbake exits with non-zero exit code it breaks lots of our tools, so just ignoring them is a bad option.
So, am I doing something obviously wrong? It is a known issue in zeus? And if so is there a known workaround?
Thanks in advance.
[-- Attachment #2: Type: text/html, Size: 3152 bytes --]
^ permalink raw reply
* Re: [PATCH v2] xfs: fix iclog release error check race with shutdown
From: Brian Foster @ 2020-02-20 16:02 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: Darrick J. Wong, Dave Chinner, linux-xfs
In-Reply-To: <20200220154317.GB6870@infradead.org>
On Thu, Feb 20, 2020 at 07:43:17AM -0800, Christoph Hellwig wrote:
> On Thu, Feb 20, 2020 at 07:41:44AM -0500, Brian Foster wrote:
> > I wasn't planning on a v3. The discussion to this point has been
> > centered around the xfs_force_shutdown() call in the associated function
> > (which is orthogonal to the bug). v1 is technically correct, but
> > Christoph suggested to restore historical behavior wrt to the shutdown
> > call. v2 does that, but is a bit superfluous in that the iclog error
> > state with the lock held implies shutdown has already occurred. This is
> > harmless (unless we're worried about shutdown performance or
> > something..), but I think Dave indicated he preferred v1 based on that
> > reasoning.
> >
> > Functionally I don't think it matters either way and at this point I
> > have no preference between v1 or v2. They fix the same problem. Do note
> > that v2 does have the Fixed: tag I missed with v1 (as well as a R-b)...
>
> I'm fine with v1 after all this discussion, and volunteer to clean up
> all the ioerr handling for the log code after this fix goes in.
>
Ok.
> That being said as noted in one of my replies I think we also need to
> add the same check in the other caller of __xlog_state_release_iclog.
>
That seems reasonable as a cleanup, but I'm not sure how critical it is
otherwise. We've already handled the iclog at that point, so we're
basically just changing the function to return an error code regardless
of the fact that we ran xlog_sync() (which doesn't care about iclog
state until we attempt to write, where it checks state again before bio
submission) and that most callers have their own IOERROR state checks up
the chain anyways because there is no other indication of I/O submission
failure..
Brian
^ permalink raw reply
* [PATCH RESEND v6 13/16] mm: Allow VM_FAULT_RETRY for multiple times
From: Peter Xu @ 2020-02-20 16:02 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Peter Xu, Martin Cracauer, Mike Rapoport, Hugh Dickins,
Jerome Glisse, Kirill A . Shutemov, Matthew Wilcox,
Pavel Emelyanov, Brian Geffon, Maya Gokhale, Denis Plotnikov,
Andrea Arcangeli, Johannes Weiner, Dr . David Alan Gilbert,
Linus Torvalds, Mike Kravetz, Marty McFadden, David Hildenbrand,
Bobby Powers, Mel Gorman
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
The idea comes from a discussion between Linus and Andrea [1].
Before this patch we only allow a page fault to retry once. We
achieved this by clearing the FAULT_FLAG_ALLOW_RETRY flag when doing
handle_mm_fault() the second time. This was majorly used to avoid
unexpected starvation of the system by looping over forever to handle
the page fault on a single page. However that should hardly happen,
and after all for each code path to return a VM_FAULT_RETRY we'll
first wait for a condition (during which time we should possibly yield
the cpu) to happen before VM_FAULT_RETRY is really returned.
This patch removes the restriction by keeping the
FAULT_FLAG_ALLOW_RETRY flag when we receive VM_FAULT_RETRY. It means
that the page fault handler now can retry the page fault for multiple
times if necessary without the need to generate another page fault
event. Meanwhile we still keep the FAULT_FLAG_TRIED flag so page
fault handler can still identify whether a page fault is the first
attempt or not.
Then we'll have these combinations of fault flags (only considering
ALLOW_RETRY flag and TRIED flag):
- ALLOW_RETRY and !TRIED: this means the page fault allows to
retry, and this is the first try
- ALLOW_RETRY and TRIED: this means the page fault allows to
retry, and this is not the first try
- !ALLOW_RETRY and !TRIED: this means the page fault does not allow
to retry at all
- !ALLOW_RETRY and TRIED: this is forbidden and should never be used
In existing code we have multiple places that has taken special care
of the first condition above by checking against (fault_flags &
FAULT_FLAG_ALLOW_RETRY). This patch introduces a simple helper to
detect the first retry of a page fault by checking against
both (fault_flags & FAULT_FLAG_ALLOW_RETRY) and !(fault_flag &
FAULT_FLAG_TRIED) because now even the 2nd try will have the
ALLOW_RETRY set, then use that helper in all existing special paths.
One example is in __lock_page_or_retry(), now we'll drop the mmap_sem
only in the first attempt of page fault and we'll keep it in follow up
retries, so old locking behavior will be retained.
This will be a nice enhancement for current code [2] at the same time
a supporting material for the future userfaultfd-writeprotect work,
since in that work there will always be an explicit userfault
writeprotect retry for protected pages, and if that cannot resolve the
page fault (e.g., when userfaultfd-writeprotect is used in conjunction
with swapped pages) then we'll possibly need a 3rd retry of the page
fault. It might also benefit other potential users who will have
similar requirement like userfault write-protection.
GUP code is not touched yet and will be covered in follow up patch.
Please read the thread below for more information.
[1] https://lore.kernel.org/lkml/20171102193644.GB22686@redhat.com/
[2] https://lore.kernel.org/lkml/20181230154648.GB9832@redhat.com/
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/alpha/mm/fault.c | 2 +-
arch/arc/mm/fault.c | 1 -
arch/arm/mm/fault.c | 3 ---
arch/arm64/mm/fault.c | 5 -----
arch/hexagon/mm/vm_fault.c | 1 -
arch/ia64/mm/fault.c | 1 -
arch/m68k/mm/fault.c | 3 ---
arch/microblaze/mm/fault.c | 1 -
arch/mips/mm/fault.c | 1 -
arch/nds32/mm/fault.c | 1 -
arch/nios2/mm/fault.c | 3 ---
arch/openrisc/mm/fault.c | 1 -
arch/parisc/mm/fault.c | 4 +---
arch/powerpc/mm/fault.c | 6 ------
arch/riscv/mm/fault.c | 5 -----
arch/s390/mm/fault.c | 5 +----
arch/sh/mm/fault.c | 1 -
arch/sparc/mm/fault_32.c | 1 -
arch/sparc/mm/fault_64.c | 1 -
arch/um/kernel/trap.c | 1 -
arch/unicore32/mm/fault.c | 4 +---
arch/x86/mm/fault.c | 2 --
arch/xtensa/mm/fault.c | 1 -
drivers/gpu/drm/ttm/ttm_bo_vm.c | 12 ++++++++---
include/linux/mm.h | 37 +++++++++++++++++++++++++++++++++
mm/filemap.c | 2 +-
mm/internal.h | 6 +++---
27 files changed, 54 insertions(+), 57 deletions(-)
diff --git a/arch/alpha/mm/fault.c b/arch/alpha/mm/fault.c
index fcfa229cc1e7..c2d7b6d7bac7 100644
--- a/arch/alpha/mm/fault.c
+++ b/arch/alpha/mm/fault.c
@@ -169,7 +169,7 @@ do_page_fault(unsigned long address, unsigned long mmcsr,
else
current->min_flt++;
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
+ flags |= FAULT_FLAG_TRIED;
/* No need to up_read(&mm->mmap_sem) as we would
* have already released it in __lock_page_or_retry
diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c
index 643fad774071..92b339c7adba 100644
--- a/arch/arc/mm/fault.c
+++ b/arch/arc/mm/fault.c
@@ -145,7 +145,6 @@ void do_page_fault(unsigned long address, struct pt_regs *regs)
*/
if (unlikely((fault & VM_FAULT_RETRY) &&
(flags & FAULT_FLAG_ALLOW_RETRY))) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
goto retry;
}
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index 18ef0b143ac2..b598e6978b29 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -319,9 +319,6 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
regs, addr);
}
if (fault & VM_FAULT_RETRY) {
- /* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
- * of starvation. */
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
goto retry;
}
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index cbb29a43aa7f..1027851d469a 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -521,12 +521,7 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
}
if (fault & VM_FAULT_RETRY) {
- /*
- * Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk of
- * starvation.
- */
if (mm_flags & FAULT_FLAG_ALLOW_RETRY) {
- mm_flags &= ~FAULT_FLAG_ALLOW_RETRY;
mm_flags |= FAULT_FLAG_TRIED;
goto retry;
}
diff --git a/arch/hexagon/mm/vm_fault.c b/arch/hexagon/mm/vm_fault.c
index d9e15d941bdb..72334b26317a 100644
--- a/arch/hexagon/mm/vm_fault.c
+++ b/arch/hexagon/mm/vm_fault.c
@@ -102,7 +102,6 @@ void do_page_fault(unsigned long address, long cause, struct pt_regs *regs)
else
current->min_flt++;
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
goto retry;
}
diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index b5aa4e80c762..30d0c1fca99e 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -167,7 +167,6 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
else
current->min_flt++;
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/* No need to up_read(&mm->mmap_sem) as we would
diff --git a/arch/m68k/mm/fault.c b/arch/m68k/mm/fault.c
index 182799fd9987..f7afb9897966 100644
--- a/arch/m68k/mm/fault.c
+++ b/arch/m68k/mm/fault.c
@@ -162,9 +162,6 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
else
current->min_flt++;
if (fault & VM_FAULT_RETRY) {
- /* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
- * of starvation. */
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/*
diff --git a/arch/microblaze/mm/fault.c b/arch/microblaze/mm/fault.c
index 32da02778a63..3248141f8ed5 100644
--- a/arch/microblaze/mm/fault.c
+++ b/arch/microblaze/mm/fault.c
@@ -236,7 +236,6 @@ void do_page_fault(struct pt_regs *regs, unsigned long address,
else
current->min_flt++;
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/*
diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c
index f177da67d940..fd64b135fd7b 100644
--- a/arch/mips/mm/fault.c
+++ b/arch/mips/mm/fault.c
@@ -178,7 +178,6 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write,
tsk->min_flt++;
}
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/*
diff --git a/arch/nds32/mm/fault.c b/arch/nds32/mm/fault.c
index 2810a4e5ab27..0cf0c08c7da2 100644
--- a/arch/nds32/mm/fault.c
+++ b/arch/nds32/mm/fault.c
@@ -246,7 +246,6 @@ void do_page_fault(unsigned long entry, unsigned long addr,
1, regs, addr);
}
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/* No need to up_read(&mm->mmap_sem) as we would
diff --git a/arch/nios2/mm/fault.c b/arch/nios2/mm/fault.c
index c38bea4220fb..ec9d8a9c426f 100644
--- a/arch/nios2/mm/fault.c
+++ b/arch/nios2/mm/fault.c
@@ -157,9 +157,6 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long cause,
else
current->min_flt++;
if (fault & VM_FAULT_RETRY) {
- /* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
- * of starvation. */
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/*
diff --git a/arch/openrisc/mm/fault.c b/arch/openrisc/mm/fault.c
index 30d5c51e9d40..8af1cc78c4fb 100644
--- a/arch/openrisc/mm/fault.c
+++ b/arch/openrisc/mm/fault.c
@@ -181,7 +181,6 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long address,
else
tsk->min_flt++;
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/* No need to up_read(&mm->mmap_sem) as we would
diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
index 8e88e5c5f26a..86e8c848f3d7 100644
--- a/arch/parisc/mm/fault.c
+++ b/arch/parisc/mm/fault.c
@@ -328,14 +328,12 @@ void do_page_fault(struct pt_regs *regs, unsigned long code,
else
current->min_flt++;
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
-
/*
* No need to up_read(&mm->mmap_sem) as we would
* have already released it in __lock_page_or_retry
* in mm/filemap.c.
*/
-
+ flags |= FAULT_FLAG_TRIED;
goto retry;
}
}
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index d7e1f8dc7e4c..d15f0f0ee806 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -590,13 +590,7 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
* case.
*/
if (unlikely(fault & VM_FAULT_RETRY)) {
- /* We retry only once */
if (flags & FAULT_FLAG_ALLOW_RETRY) {
- /*
- * Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
- * of starvation.
- */
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
goto retry;
}
diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c
index a252d9e38561..be84e32adc4c 100644
--- a/arch/riscv/mm/fault.c
+++ b/arch/riscv/mm/fault.c
@@ -144,11 +144,6 @@ asmlinkage void do_page_fault(struct pt_regs *regs)
1, regs, addr);
}
if (fault & VM_FAULT_RETRY) {
- /*
- * Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
- * of starvation.
- */
- flags &= ~(FAULT_FLAG_ALLOW_RETRY);
flags |= FAULT_FLAG_TRIED;
/*
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 551ac311bd35..aeccdb30899a 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -513,10 +513,7 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access)
fault = VM_FAULT_PFAULT;
goto out_up;
}
- /* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
- * of starvation. */
- flags &= ~(FAULT_FLAG_ALLOW_RETRY |
- FAULT_FLAG_RETRY_NOWAIT);
+ flags &= ~FAULT_FLAG_RETRY_NOWAIT;
flags |= FAULT_FLAG_TRIED;
down_read(&mm->mmap_sem);
goto retry;
diff --git a/arch/sh/mm/fault.c b/arch/sh/mm/fault.c
index d9c8f2d00a54..13ee4d20e622 100644
--- a/arch/sh/mm/fault.c
+++ b/arch/sh/mm/fault.c
@@ -481,7 +481,6 @@ asmlinkage void __kprobes do_page_fault(struct pt_regs *regs,
regs, address);
}
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/*
diff --git a/arch/sparc/mm/fault_32.c b/arch/sparc/mm/fault_32.c
index a91b0c2d84f8..f6e0e601f857 100644
--- a/arch/sparc/mm/fault_32.c
+++ b/arch/sparc/mm/fault_32.c
@@ -261,7 +261,6 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
1, regs, address);
}
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/* No need to up_read(&mm->mmap_sem) as we would
diff --git a/arch/sparc/mm/fault_64.c b/arch/sparc/mm/fault_64.c
index 30653418a672..c0c0dd471b6b 100644
--- a/arch/sparc/mm/fault_64.c
+++ b/arch/sparc/mm/fault_64.c
@@ -449,7 +449,6 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
1, regs, address);
}
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/* No need to up_read(&mm->mmap_sem) as we would
diff --git a/arch/um/kernel/trap.c b/arch/um/kernel/trap.c
index c59ad37eacda..8f18cf56b3dd 100644
--- a/arch/um/kernel/trap.c
+++ b/arch/um/kernel/trap.c
@@ -97,7 +97,6 @@ int handle_page_fault(unsigned long address, unsigned long ip,
else
current->min_flt++;
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
goto retry;
diff --git a/arch/unicore32/mm/fault.c b/arch/unicore32/mm/fault.c
index 34a90453ca18..a9bd08fbe588 100644
--- a/arch/unicore32/mm/fault.c
+++ b/arch/unicore32/mm/fault.c
@@ -259,9 +259,7 @@ static int do_pf(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
else
tsk->min_flt++;
if (fault & VM_FAULT_RETRY) {
- /* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk
- * of starvation. */
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
+ flags |= FAULT_FLAG_TRIED;
goto retry;
}
}
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index 7b6f65333355..4ce647bbe546 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1457,8 +1457,6 @@ void do_user_addr_fault(struct pt_regs *regs,
*/
if (unlikely((fault & VM_FAULT_RETRY) &&
(flags & FAULT_FLAG_ALLOW_RETRY))) {
- /* Retry at most once */
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
goto retry;
}
diff --git a/arch/xtensa/mm/fault.c b/arch/xtensa/mm/fault.c
index 7d196dc951e8..e7172bd53ced 100644
--- a/arch/xtensa/mm/fault.c
+++ b/arch/xtensa/mm/fault.c
@@ -128,7 +128,6 @@ void do_page_fault(struct pt_regs *regs)
else
current->min_flt++;
if (fault & VM_FAULT_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
/* No need to up_read(&mm->mmap_sem) as we would
diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c
index 389128b8c4dd..cb8829ca6c7f 100644
--- a/drivers/gpu/drm/ttm/ttm_bo_vm.c
+++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c
@@ -59,9 +59,10 @@ static vm_fault_t ttm_bo_vm_fault_idle(struct ttm_buffer_object *bo,
/*
* If possible, avoid waiting for GPU with mmap_sem
- * held.
+ * held. We only do this if the fault allows retry and this
+ * is the first attempt.
*/
- if (vmf->flags & FAULT_FLAG_ALLOW_RETRY) {
+ if (fault_flag_allow_retry_first(vmf->flags)) {
ret = VM_FAULT_RETRY;
if (vmf->flags & FAULT_FLAG_RETRY_NOWAIT)
goto out_unlock;
@@ -135,7 +136,12 @@ vm_fault_t ttm_bo_vm_reserve(struct ttm_buffer_object *bo,
* for the buffer to become unreserved.
*/
if (unlikely(!dma_resv_trylock(bo->base.resv))) {
- if (vmf->flags & FAULT_FLAG_ALLOW_RETRY) {
+ /*
+ * If the fault allows retry and this is the first
+ * fault attempt, we try to release the mmap_sem
+ * before waiting
+ */
+ if (fault_flag_allow_retry_first(vmf->flags)) {
if (!(vmf->flags & FAULT_FLAG_RETRY_NOWAIT)) {
ttm_bo_get(bo);
up_read(&vmf->vma->vm_mm->mmap_sem);
diff --git a/include/linux/mm.h b/include/linux/mm.h
index ff653f9136dd..51a886d50758 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -391,6 +391,25 @@ extern pgprot_t protection_map[16];
* @FAULT_FLAG_REMOTE: The fault is not for current task/mm.
* @FAULT_FLAG_INSTRUCTION: The fault was during an instruction fetch.
* @FAULT_FLAG_INTERRUPTIBLE: The fault can be interrupted by non-fatal signals.
+ *
+ * About @FAULT_FLAG_ALLOW_RETRY and @FAULT_FLAG_TRIED: we can specify
+ * whether we would allow page faults to retry by specifying these two
+ * fault flags correctly. Currently there can be three legal combinations:
+ *
+ * (a) ALLOW_RETRY and !TRIED: this means the page fault allows retry, and
+ * this is the first try
+ *
+ * (b) ALLOW_RETRY and TRIED: this means the page fault allows retry, and
+ * we've already tried at least once
+ *
+ * (c) !ALLOW_RETRY and !TRIED: this means the page fault does not allow retry
+ *
+ * The unlisted combination (!ALLOW_RETRY && TRIED) is illegal and should never
+ * be used. Note that page faults can be allowed to retry for multiple times,
+ * in which case we'll have an initial fault with flags (a) then later on
+ * continuous faults with flags (b). We should always try to detect pending
+ * signals before a retry to make sure the continuous page faults can still be
+ * interrupted if necessary.
*/
#define FAULT_FLAG_WRITE 0x01
#define FAULT_FLAG_MKWRITE 0x02
@@ -411,6 +430,24 @@ extern pgprot_t protection_map[16];
FAULT_FLAG_KILLABLE | \
FAULT_FLAG_INTERRUPTIBLE)
+/**
+ * fault_flag_allow_retry_first - check ALLOW_RETRY the first time
+ *
+ * This is mostly used for places where we want to try to avoid taking
+ * the mmap_sem for too long a time when waiting for another condition
+ * to change, in which case we can try to be polite to release the
+ * mmap_sem in the first round to avoid potential starvation of other
+ * processes that would also want the mmap_sem.
+ *
+ * Return: true if the page fault allows retry and this is the first
+ * attempt of the fault handling; false otherwise.
+ */
+static inline bool fault_flag_allow_retry_first(unsigned int flags)
+{
+ return (flags & FAULT_FLAG_ALLOW_RETRY) &&
+ (!(flags & FAULT_FLAG_TRIED));
+}
+
#define FAULT_FLAG_TRACE \
{ FAULT_FLAG_WRITE, "WRITE" }, \
{ FAULT_FLAG_MKWRITE, "MKWRITE" }, \
diff --git a/mm/filemap.c b/mm/filemap.c
index 1784478270e1..590ec3a9f5da 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1386,7 +1386,7 @@ EXPORT_SYMBOL_GPL(__lock_page_killable);
int __lock_page_or_retry(struct page *page, struct mm_struct *mm,
unsigned int flags)
{
- if (flags & FAULT_FLAG_ALLOW_RETRY) {
+ if (fault_flag_allow_retry_first(flags)) {
/*
* CAUTION! In this case, mmap_sem is not released
* even though return 0.
diff --git a/mm/internal.h b/mm/internal.h
index 3cf20ab3ca01..5958cfe50a0c 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -377,10 +377,10 @@ static inline struct file *maybe_unlock_mmap_for_io(struct vm_fault *vmf,
/*
* FAULT_FLAG_RETRY_NOWAIT means we don't want to wait on page locks or
* anything, so we only pin the file and drop the mmap_sem if only
- * FAULT_FLAG_ALLOW_RETRY is set.
+ * FAULT_FLAG_ALLOW_RETRY is set, while this is the first attempt.
*/
- if ((flags & (FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_RETRY_NOWAIT)) ==
- FAULT_FLAG_ALLOW_RETRY) {
+ if (fault_flag_allow_retry_first(flags) &&
+ !(flags & FAULT_FLAG_RETRY_NOWAIT)) {
fpin = get_file(vmf->vma->vm_file);
up_read(&vmf->vma->vm_mm->mmap_sem);
}
--
2.24.1
^ permalink raw reply related
* [PATCH 1/2] drm/i915/gt: Detect if we miss WaIdleLiteRestore
From: Chris Wilson @ 2020-02-20 16:03 UTC (permalink / raw)
To: stable
Upstream: 82c69bf58650 ("drm/i915/gt: Detect if we miss WaIdleLiteRestore")
In order to avoid confusing the HW, we must never submit an empty ring
during lite-restore, that is we should always advance the RING_TAIL
before submitting to stay ahead of the RING_HEAD.
Normally this is prevented by keeping a couple of spare NOPs in the
request->wa_tail so that on resubmission we can advance the tail. This
relies on the request only being resubmitted once, which is the normal
condition as it is seen once for ELSP[1] and then later in ELSP[0]. On
preemption, the requests are unwound and the tail reset back to the
normal end point (as we know the request is incomplete and therefore its
RING_HEAD is even earlier).
However, if this w/a should fail we would try and resubmit the request
with the RING_TAIL already set to the location of this request's wa_tail
potentially causing a GPU hang. We can spot when we do try and
incorrectly resubmit without advancing the RING_TAIL and spare any
embarrassment by forcing the context restore.
In the case of preempt-to-busy, we leave the requests running on the HW
while we unwind. As the ring is still live, we cannot rewind our
rq->tail without forcing a reload so leave it set to rq->wa_tail and
only force a reload if we resubmit after a lite-restore. (Normally, the
forced reload will be a part of the preemption event.)
Fixes: 22b7a426bbe1 ("drm/i915/execlists: Preempt-to-busy")
Closes: https://gitlab.freedesktop.org/drm/intel/issues/673
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: stable@kernel.vger.org #5.3+
Link: https://patchwork.freedesktop.org/patch/msgid/20191209023215.3519970-1-chris@chris-wilson.co.uk
---
drivers/gpu/drm/i915/gt/intel_lrc.c | 42 ++++++++++++++---------------
1 file changed, 20 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index 4949b5ad860f..e0e4f3deb2da 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -471,12 +471,6 @@ lrc_descriptor(struct intel_context *ce, struct intel_engine_cs *engine)
return desc;
}
-static void unwind_wa_tail(struct i915_request *rq)
-{
- rq->tail = intel_ring_wrap(rq->ring, rq->wa_tail - WA_TAIL_BYTES);
- assert_ring_tail_valid(rq->ring, rq->tail);
-}
-
static struct i915_request *
__unwind_incomplete_requests(struct intel_engine_cs *engine)
{
@@ -495,7 +489,6 @@ __unwind_incomplete_requests(struct intel_engine_cs *engine)
continue; /* XXX */
__i915_request_unsubmit(rq);
- unwind_wa_tail(rq);
/*
* Push the request back into the queue for later resubmission.
@@ -650,13 +643,29 @@ execlists_schedule_out(struct i915_request *rq)
i915_request_put(rq);
}
-static u64 execlists_update_context(const struct i915_request *rq)
+static u64 execlists_update_context(struct i915_request *rq)
{
struct intel_context *ce = rq->hw_context;
- u64 desc;
+ u64 desc = ce->lrc_desc;
+ u32 tail;
- ce->lrc_reg_state[CTX_RING_TAIL + 1] =
- intel_ring_set_tail(rq->ring, rq->tail);
+ /*
+ * WaIdleLiteRestore:bdw,skl
+ *
+ * We should never submit the context with the same RING_TAIL twice
+ * just in case we submit an empty ring, which confuses the HW.
+ *
+ * We append a couple of NOOPs (gen8_emit_wa_tail) after the end of
+ * the normal request to be able to always advance the RING_TAIL on
+ * subsequent resubmissions (for lite restore). Should that fail us,
+ * and we try and submit the same tail again, force the context
+ * reload.
+ */
+ tail = intel_ring_set_tail(rq->ring, rq->tail);
+ if (unlikely(ce->lrc_reg_state[CTX_RING_TAIL + 1] == tail))
+ desc |= CTX_DESC_FORCE_RESTORE;
+ ce->lrc_reg_state[CTX_RING_TAIL + 1] = tail;
+ rq->tail = rq->wa_tail;
/*
* Make sure the context image is complete before we submit it to HW.
@@ -675,7 +684,6 @@ static u64 execlists_update_context(const struct i915_request *rq)
*/
mb();
- desc = ce->lrc_desc;
ce->lrc_desc &= ~CTX_DESC_FORCE_RESTORE;
return desc;
@@ -1150,16 +1158,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
if (!list_is_last(&last->sched.link,
&engine->active.requests))
return;
-
- /*
- * WaIdleLiteRestore:bdw,skl
- * Apply the wa NOOPs to prevent
- * ring:HEAD == rq:TAIL as we resubmit the
- * request. See gen8_emit_fini_breadcrumb() for
- * where we prepare the padding after the
- * end of the request.
- */
- last->tail = last->wa_tail;
}
}
--
2.25.1
^ permalink raw reply related
* [PATCH 2/2] drm/i915/execlists: Always force a context reload when rewinding RING_TAIL
From: Chris Wilson @ 2020-02-20 16:03 UTC (permalink / raw)
To: stable
In-Reply-To: <20200220160319.1919629-1-chris@chris-wilson.co.uk>
Upstream: b1339ecac661 ("drm/i915/execlists: Always force a context reload when rewinding RING_TAIL")
If we rewind the RING_TAIL on a context, due to a preemption event, we
must force the context restore for the RING_TAIL update to be properly
handled. Rather than note which preemption events may cause us to rewind
the tail, compare the new request's tail with the previously submitted
RING_TAIL, as it turns out that timeslicing was causing unexpected
rewinds.
<idle>-0 0d.s2 1280851190us : __execlists_submission_tasklet: 0000:00:02.0 rcs0: expired last=130:4698, prio=3, hint=3
<idle>-0 0d.s2 1280851192us : __i915_request_unsubmit: 0000:00:02.0 rcs0: fence 66:119966, current 119964
<idle>-0 0d.s2 1280851195us : __i915_request_unsubmit: 0000:00:02.0 rcs0: fence 130:4698, current 4695
<idle>-0 0d.s2 1280851198us : __i915_request_unsubmit: 0000:00:02.0 rcs0: fence 130:4696, current 4695
^---- Note we unwind 2 requests from the same context
<idle>-0 0d.s2 1280851208us : __i915_request_submit: 0000:00:02.0 rcs0: fence 130:4696, current 4695
<idle>-0 0d.s2 1280851213us : __i915_request_submit: 0000:00:02.0 rcs0: fence 134:1508, current 1506
^---- But to apply the new timeslice, we have to replay the first request
before the new client can start -- the unexpected RING_TAIL rewind
<idle>-0 0d.s2 1280851219us : trace_ports: 0000:00:02.0 rcs0: submit { 130:4696*, 134:1508 }
synmark2-5425 2..s. 1280851239us : process_csb: 0000:00:02.0 rcs0: cs-irq head=5, tail=0
synmark2-5425 2..s. 1280851240us : process_csb: 0000:00:02.0 rcs0: csb[0]: status=0x00008002:0x00000000
^---- Preemption event for the ELSP update; note the lite-restore
synmark2-5425 2..s. 1280851243us : trace_ports: 0000:00:02.0 rcs0: preempted { 130:4698, 66:119966 }
synmark2-5425 2..s. 1280851246us : trace_ports: 0000:00:02.0 rcs0: promote { 130:4696*, 134:1508 }
synmark2-5425 2.... 1280851462us : __i915_request_commit: 0000:00:02.0 rcs0: fence 130:4700, current 4695
synmark2-5425 2.... 1280852111us : __i915_request_commit: 0000:00:02.0 rcs0: fence 130:4702, current 4695
synmark2-5425 2.Ns1 1280852296us : process_csb: 0000:00:02.0 rcs0: cs-irq head=0, tail=2
synmark2-5425 2.Ns1 1280852297us : process_csb: 0000:00:02.0 rcs0: csb[1]: status=0x00000814:0x00000000
synmark2-5425 2.Ns1 1280852299us : trace_ports: 0000:00:02.0 rcs0: completed { 130:4696!, 134:1508 }
synmark2-5425 2.Ns1 1280852301us : process_csb: 0000:00:02.0 rcs0: csb[2]: status=0x00000818:0x00000040
synmark2-5425 2.Ns1 1280852302us : trace_ports: 0000:00:02.0 rcs0: completed { 134:1508, 0:0 }
synmark2-5425 2.Ns1 1280852313us : process_csb: process_csb:2336 GEM_BUG_ON(!i915_request_completed(*execlists->active) && !reset_in_progress(execlists))
Fixes: 8ee36e048c98 ("drm/i915/execlists: Minimalistic timeslicing")
Referenecs: 82c69bf58650 ("drm/i915/gt: Detect if we miss WaIdleLiteRestore")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Cc: <stable@vger.kernel.org> # v5.4+
Link: https://patchwork.freedesktop.org/patch/msgid/20200207211452.2860634-1-chris@chris-wilson.co.uk
(cherry picked from commit 5ba32c7be81e53ea8a27190b0f6be98e6c6779af)
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/gt/intel_engine.h | 8 ++++++++
drivers/gpu/drm/i915/gt/intel_engine_types.h | 1 +
drivers/gpu/drm/i915/gt/intel_lrc.c | 18 ++++++++----------
drivers/gpu/drm/i915/gt/intel_ringbuffer.c | 2 ++
4 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
index 22aab8593abf..926272b5a0ca 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -250,6 +250,14 @@ static inline u32 intel_ring_wrap(const struct intel_ring *ring, u32 pos)
return pos & (ring->size - 1);
}
+static inline int intel_ring_direction(const struct intel_ring *ring,
+ u32 next, u32 prev)
+{
+ typecheck(typeof(ring->size), next);
+ typecheck(typeof(ring->size), prev);
+ return (next - prev) << ring->wrap;
+}
+
static inline bool
intel_ring_offset_valid(const struct intel_ring *ring,
unsigned int pos)
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
index 798e1b024406..c77c9518c58b 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
@@ -107,6 +107,7 @@ struct intel_ring {
u32 space;
u32 size;
+ u32 wrap;
u32 effective_size;
};
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
index e0e4f3deb2da..bf6addece25b 100644
--- a/drivers/gpu/drm/i915/gt/intel_lrc.c
+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
@@ -647,7 +647,7 @@ static u64 execlists_update_context(struct i915_request *rq)
{
struct intel_context *ce = rq->hw_context;
u64 desc = ce->lrc_desc;
- u32 tail;
+ u32 tail, prev;
/*
* WaIdleLiteRestore:bdw,skl
@@ -660,9 +660,15 @@ static u64 execlists_update_context(struct i915_request *rq)
* subsequent resubmissions (for lite restore). Should that fail us,
* and we try and submit the same tail again, force the context
* reload.
+ *
+ * If we need to return to a preempted context, we need to skip the
+ * lite-restore and force it to reload the RING_TAIL. Otherwise, the
+ * HW has a tendency to ignore us rewinding the TAIL to the end of
+ * an earlier request.
*/
tail = intel_ring_set_tail(rq->ring, rq->tail);
- if (unlikely(ce->lrc_reg_state[CTX_RING_TAIL + 1] == tail))
+ prev = ce->lrc_reg_state[CTX_RING_TAIL + 1];
+ if (unlikely(intel_ring_direction(rq->ring, tail, prev) <= 0))
desc |= CTX_DESC_FORCE_RESTORE;
ce->lrc_reg_state[CTX_RING_TAIL + 1] = tail;
rq->tail = rq->wa_tail;
@@ -1110,14 +1116,6 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
*/
__unwind_incomplete_requests(engine);
- /*
- * If we need to return to the preempted context, we
- * need to skip the lite-restore and force it to
- * reload the RING_TAIL. Otherwise, the HW has a
- * tendency to ignore us rewinding the TAIL to the
- * end of an earlier request.
- */
- last->hw_context->lrc_desc |= CTX_DESC_FORCE_RESTORE;
last = NULL;
} else if (need_timeslice(engine, last) &&
!timer_pending(&engine->execlists.timer)) {
diff --git a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
index bacaa7bb8c9a..eee9fcbe0434 100644
--- a/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/gt/intel_ringbuffer.c
@@ -1312,6 +1312,8 @@ intel_engine_create_ring(struct intel_engine_cs *engine, int size)
kref_init(&ring->ref);
ring->size = size;
+ ring->wrap = BITS_PER_TYPE(ring->size) - ilog2(size);
+
/* Workaround an erratum on the i830 which causes a hang if
* the TAIL pointer points to within the last 2 cachelines
* of the buffer.
--
2.25.1
^ permalink raw reply related
* Re: [PATCH 0/5] Support metric group constraint
From: Liang, Kan @ 2020-02-20 16:03 UTC (permalink / raw)
To: Jiri Olsa
Cc: acme, mingo, peterz, linux-kernel, mark.rutland, namhyung,
ravi.bangoria, yao.jin, ak
In-Reply-To: <20200220113924.GB565976@krava>
On 2/20/2020 6:39 AM, Jiri Olsa wrote:
> On Wed, Feb 19, 2020 at 11:08:35AM -0800, kan.liang@linux.intel.com wrote:
>> From: Kan Liang <kan.liang@linux.intel.com>
>>
>> Some metric groups, e.g. Page_Walks_Utilization, will never count when
>> NMI watchdog is enabled.
>>
>> $echo 1 > /proc/sys/kernel/nmi_watchdog
>> $perf stat -M Page_Walks_Utilization
>>
>> Performance counter stats for 'system wide':
>>
>> <not counted> itlb_misses.walk_pending (0.00%)
>> <not counted> dtlb_load_misses.walk_pending (0.00%)
>> <not counted> dtlb_store_misses.walk_pending (0.00%)
>> <not counted> ept.walk_pending (0.00%)
>> <not counted> cycles (0.00%)
>>
>> 2.343460588 seconds time elapsed
>>
>> Some events weren't counted. Try disabling the NMI watchdog:
>> echo 0 > /proc/sys/kernel/nmi_watchdog
>> perf stat ...
>> echo 1 > /proc/sys/kernel/nmi_watchdog
>> The events in group usually have to be from the same PMU. Try
>> reorganizing the group.
>>
>> A metric group is a weak group, which relies on group validation
>> code in the kernel to determine whether to be opened as a group or
>> a non-group. However, group validation code may return false-positives,
>> especially when NMI watchdog is enabled. (The metric group is allowed
>> as a group but will never be scheduled.)
>>
>> The attempt to fix the group validation code has been rejected.
>> https://lore.kernel.org/lkml/20200117091341.GX2827@hirez.programming.kicks-ass.net/
>> Because we cannot accurately predict whether the group can be scheduled
>> as a group, only by checking current status.
>>
>> This patch set provides another solution to mitigate the issue.
>> Add "MetricConstraint" in event list, which provides a hint for perf tool,
>> e.g. "MetricConstraint": "NO_NMI_WATCHDOG". Perf tool can change the
>> metric group to non-group (standalone metrics) if NMI watchdog is enabled.
>
> the problem is in the missing counter, that's taken by NMI watchdog, right?
>
> and it's problem for any metric that won't fit to the available
> counters.. shouldn't we rather do this workaround for any metric
> that wouldn't fit in available counters?
I think current perf already did this.
All metric groups are weak group. Kernel (validate_group()) tells perf
tool whether a metric group fit to available counters.
If yes, the metric group will be scheduled as a group.
If no, perf tool will not using a group and re-try. The code is as below.
try_again:
if (create_perf_stat_counter(counter, &stat_config, &target) < 0) {
/* Weak group failed. Reset the group. */
if ((errno == EINVAL || errno == EBADF) &&
counter->leader != counter &&
counter->weak_group) {
counter = perf_evlist__reset_weak_group(evsel_list, counter);
goto try_again;
}
This patch set is to workaroud the false-positives from the kernel.
Kernel only validate the group itself. It assumes that all counters are
available. So, when any counter is permanently occupied by others, e.g.
watchdog, the validate_group() may return false-positives.
? not just for chosen
> ones..?
For now, I think we only need to workaround the Page_Walks_Utilization
metric group. Because it has 5 events, and one of the events is cycles.
The cycles has to be scheduled on fixed counter 2. But it's already
occupied by watchdog.
The false-positives of validate_group() will trigger the issue (metric
group never be scheduled.).
For other metric groups, even they have cycles, the issue should not be
triggered.
For example, if they have 4 or less events, the cycles can be scheduled
to GP counter instead.
If they have 6 or more events, the weak group will be reject anyway.
Perf tool will open it as non-group (standalone metrics).
I think we only need to apply the constraint for the
Page_Walks_Utilization metric group for now.
Thanks,
Kan
>
> thanks,
> jirka
>
^ permalink raw reply
* Re: [RFC PATCH v3 05/27] qcow2: Document the Extended L2 Entries feature
From: Eric Blake @ 2020-02-20 16:02 UTC (permalink / raw)
To: Max Reitz, Alberto Garcia, qemu-devel
Cc: Kevin Wolf, Denis V . Lunev, Anton Nefedov,
Vladimir Sementsov-Ogievskiy, qemu-block
In-Reply-To: <7ff19f65-5148-a40a-9b7a-6a330cf7272e@redhat.com>
On 2/20/20 9:54 AM, Max Reitz wrote:
>> +Subcluster Allocation Bitmap (for standard clusters):
>> +
>> + Bit 0 - 31: Allocation status (one bit per subcluster)
>> +
>> + 1: the subcluster is allocated. In this case the
>> + host cluster offset field must contain a valid
>> + offset.
>> + 0: the subcluster is not allocated. In this case
>> + read requests shall go to the backing file or
>> + return zeros if there is no backing file data.
>> +
>> + Bits are assigned starting from the most significant one.
>> + (i.e. bit x is used for subcluster 31 - x)
>
> I still prefer it the other way round, both personally (e.g. it’s the C
> ordering), and because other places in qcow2 use LSb for bit ordering
> (the refcount order).
Internal consistency with refcount order using LSb ordering is the
strongest reason to flip things, and have bit x be subcluster x.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply
* Re: omap-secure.c:undefined reference to `__arm_smccc_smc'
From: Andrew F. Davis @ 2020-02-20 16:03 UTC (permalink / raw)
To: kbuild-all
In-Reply-To: <20200220155429.GH37466@atomide.com>
[-- Attachment #1: Type: text/plain, Size: 1528 bytes --]
On 2/20/20 10:54 AM, Tony Lindgren wrote:
> Andrew,
>
> * kbuild test robot <lkp@intel.com> [200213 10:27]:
>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>> head: 0bf999f9c5e74c7ecf9dafb527146601e5c848b9
>> commit: c37baa06f8a970e4a533d41f7d33e5e57de5ad25 ARM: OMAP2+: Fix undefined reference to omap_secure_init
>> date: 3 weeks ago
>> config: arm-randconfig-a001-20200213 (attached as .config)
>> compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
>> reproduce:
>> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> git checkout c37baa06f8a970e4a533d41f7d33e5e57de5ad25
>> # save the attached .config to linux build tree
>> GCC_VERSION=7.5.0 make.cross ARCH=arm
>>
>> If you fix the issue, kindly add following tag
>> Reported-by: kbuild test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>):
>>
>> arch/arm/mach-omap2/omap-secure.o: In function `omap_smccc_smc':
>>>> omap-secure.c:(.text+0x94): undefined reference to `__arm_smccc_smc'
>
> Have you looked at this one? Looks like there's still an unhandled
> randconfig build case.
>
I've had a quick look, all the ARM config does:
select HAVE_ARM_SMCCC if CPU_V7
so I don't think this will happen in any real config, but if we want to
prevent randconfig issue this we could force ARCH_OMAP2PLUS to "depend"
on it.
Andrew
> Regards,
>
> Tony
>
^ permalink raw reply
* Re: omap-secure.c:undefined reference to `__arm_smccc_smc'
From: Andrew F. Davis @ 2020-02-20 16:03 UTC (permalink / raw)
To: Tony Lindgren; +Cc: kbuild-all, linux-kernel, kbuild test robot
In-Reply-To: <20200220155429.GH37466@atomide.com>
On 2/20/20 10:54 AM, Tony Lindgren wrote:
> Andrew,
>
> * kbuild test robot <lkp@intel.com> [200213 10:27]:
>> tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>> head: 0bf999f9c5e74c7ecf9dafb527146601e5c848b9
>> commit: c37baa06f8a970e4a533d41f7d33e5e57de5ad25 ARM: OMAP2+: Fix undefined reference to omap_secure_init
>> date: 3 weeks ago
>> config: arm-randconfig-a001-20200213 (attached as .config)
>> compiler: arm-linux-gnueabi-gcc (GCC) 7.5.0
>> reproduce:
>> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>> chmod +x ~/bin/make.cross
>> git checkout c37baa06f8a970e4a533d41f7d33e5e57de5ad25
>> # save the attached .config to linux build tree
>> GCC_VERSION=7.5.0 make.cross ARCH=arm
>>
>> If you fix the issue, kindly add following tag
>> Reported-by: kbuild test robot <lkp@intel.com>
>>
>> All errors (new ones prefixed by >>):
>>
>> arch/arm/mach-omap2/omap-secure.o: In function `omap_smccc_smc':
>>>> omap-secure.c:(.text+0x94): undefined reference to `__arm_smccc_smc'
>
> Have you looked at this one? Looks like there's still an unhandled
> randconfig build case.
>
I've had a quick look, all the ARM config does:
select HAVE_ARM_SMCCC if CPU_V7
so I don't think this will happen in any real config, but if we want to
prevent randconfig issue this we could force ARCH_OMAP2PLUS to "depend"
on it.
Andrew
> Regards,
>
> Tony
>
^ permalink raw reply
* [PATCH RESEND v6 09/16] mm: Return faster for non-fatal signals in user mode faults
From: Peter Xu @ 2020-02-20 16:02 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Peter Xu, Martin Cracauer, Mike Rapoport, Hugh Dickins,
Jerome Glisse, Kirill A . Shutemov, Matthew Wilcox,
Pavel Emelyanov, Brian Geffon, Maya Gokhale, Denis Plotnikov,
Andrea Arcangeli, Johannes Weiner, Dr . David Alan Gilbert,
Linus Torvalds, Mike Kravetz, Marty McFadden, David Hildenbrand,
Bobby Powers, Mel Gorman
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
The idea comes from the upstream discussion between Linus and Andrea:
https://lore.kernel.org/lkml/20171102193644.GB22686@redhat.com/
A summary to the issue: there was a special path in handle_userfault()
in the past that we'll return a VM_FAULT_NOPAGE when we detected
non-fatal signals when waiting for userfault handling. We did that by
reacquiring the mmap_sem before returning. However that brings a risk
in that the vmas might have changed when we retake the mmap_sem and
even we could be holding an invalid vma structure.
This patch is a preparation of removing that special path by allowing
the page fault to return even faster if we were interrupted by a
non-fatal signal during a user-mode page fault handling routine.
Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Suggested-by: Andrea Arcangeli <aarcange@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
include/linux/sched/signal.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 4c87ffce64d1..09d40ce6a162 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -379,7 +379,8 @@ static inline bool fault_signal_pending(unsigned int fault_flags,
struct pt_regs *regs)
{
return unlikely((fault_flags & VM_FAULT_RETRY) &&
- fatal_signal_pending(current));
+ (fatal_signal_pending(current) ||
+ (user_mode(regs) && signal_pending(current))));
}
/*
--
2.24.1
^ permalink raw reply related
* Re: [RFC PATCH v3 06/27] qcow2: Add dummy has_subclusters() function
From: Max Reitz @ 2020-02-20 16:03 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel
Cc: Kevin Wolf, Anton Nefedov, qemu-block,
Vladimir Sementsov-Ogievskiy, Denis V . Lunev
In-Reply-To: <bebe4058df5210ac3293e917ad6b61abac398f60.1577014346.git.berto@igalia.com>
[-- Attachment #1.1: Type: text/plain, Size: 542 bytes --]
On 22.12.19 12:36, Alberto Garcia wrote:
> This function will be used by the qcow2 code to check if an image has
> subclusters or not.
>
> At the moment this simply returns false. Once all patches needed for
> subcluster support are ready then QEMU will be able to create and
> read images with subclusters and this function will return the actual
> value.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> block/qcow2.h | 6 ++++++
> 1 file changed, 6 insertions(+)
Reviewed-by: Max Reitz <mreitz@redhat.com>
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [igt-dev] ✓ Fi.CI.BAT: success for lib/i915: Restrict mmap types to GTT if no MMAP_OFFSET support
From: Patchwork @ 2020-02-20 16:04 UTC (permalink / raw)
To: Janusz Krzysztofik; +Cc: igt-dev
In-Reply-To: <20200220153203.29571-1-janusz.krzysztofik@linux.intel.com>
== Series Details ==
Series: lib/i915: Restrict mmap types to GTT if no MMAP_OFFSET support
URL : https://patchwork.freedesktop.org/series/73719/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_7973 -> IGTPW_4198
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/index.html
New tests
---------
New tests have been introduced between CI_DRM_7973 and IGTPW_4198:
### New IGT tests (4) ###
* igt@i915_pm_backlight@basic-brightness:
- Statuses : 1 dmesg-warn(s) 17 pass(s) 23 skip(s)
- Exec time: [0.0, 0.23] s
* igt@i915_pm_rpm@basic-pci-d3-state:
- Statuses : 1 dmesg-warn(s) 29 pass(s) 11 skip(s)
- Exec time: [0.0, 10.49] s
* igt@i915_pm_rpm@basic-rte:
- Statuses : 1 dmesg-warn(s) 29 pass(s) 11 skip(s)
- Exec time: [0.44, 11.29] s
* igt@i915_pm_rps@basic-api:
- Statuses : 36 pass(s) 5 skip(s)
- Exec time: [0.0, 0.01] s
Known issues
------------
Here are the changes found in IGTPW_4198 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_close_race@basic-threads:
- fi-byt-n2820: [PASS][1] -> [INCOMPLETE][2] ([i915#45])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7973/fi-byt-n2820/igt@gem_close_race@basic-threads.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/fi-byt-n2820/igt@gem_close_race@basic-threads.html
* igt@gem_mmap_gtt@basic:
- fi-tgl-y: [PASS][3] -> [DMESG-WARN][4] ([CI#94] / [i915#402]) +1 similar issue
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7973/fi-tgl-y/igt@gem_mmap_gtt@basic.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/fi-tgl-y/igt@gem_mmap_gtt@basic.html
* igt@i915_selftest@live_execlists:
- fi-tgl-y: [PASS][5] -> [INCOMPLETE][6] ([CI#94] / [i915#647])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7973/fi-tgl-y/igt@i915_selftest@live_execlists.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/fi-tgl-y/igt@i915_selftest@live_execlists.html
#### Possible fixes ####
* igt@i915_selftest@live_gem_contexts:
- fi-cml-s: [DMESG-FAIL][7] ([i915#877]) -> [PASS][8]
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7973/fi-cml-s/igt@i915_selftest@live_gem_contexts.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/fi-cml-s/igt@i915_selftest@live_gem_contexts.html
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-icl-u2: [FAIL][9] ([i915#217]) -> [PASS][10]
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7973/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/fi-icl-u2/igt@kms_chamelium@hdmi-hpd-fast.html
* igt@prime_self_import@basic-llseek-bad:
- fi-tgl-y: [DMESG-WARN][11] ([CI#94] / [i915#402]) -> [PASS][12] +1 similar issue
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7973/fi-tgl-y/igt@prime_self_import@basic-llseek-bad.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/fi-tgl-y/igt@prime_self_import@basic-llseek-bad.html
#### Warnings ####
* igt@amdgpu/amd_prime@amd-to-i915:
- fi-icl-u3: [SKIP][13] ([fdo#109315]) -> [SKIP][14] ([fdo#109315] / [i915#585])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7973/fi-icl-u3/igt@amdgpu/amd_prime@amd-to-i915.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/fi-icl-u3/igt@amdgpu/amd_prime@amd-to-i915.html
* igt@kms_chamelium@hdmi-hpd-fast:
- fi-kbl-7500u: [FAIL][15] ([fdo#111407]) -> [FAIL][16] ([fdo#111096] / [i915#323])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_7973/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/fi-kbl-7500u/igt@kms_chamelium@hdmi-hpd-fast.html
[CI#94]: https://gitlab.freedesktop.org/gfx-ci/i915-infra/issues/94
[fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
[fdo#111096]: https://bugs.freedesktop.org/show_bug.cgi?id=111096
[fdo#111407]: https://bugs.freedesktop.org/show_bug.cgi?id=111407
[i915#217]: https://gitlab.freedesktop.org/drm/intel/issues/217
[i915#323]: https://gitlab.freedesktop.org/drm/intel/issues/323
[i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
[i915#45]: https://gitlab.freedesktop.org/drm/intel/issues/45
[i915#585]: https://gitlab.freedesktop.org/drm/intel/issues/585
[i915#647]: https://gitlab.freedesktop.org/drm/intel/issues/647
[i915#877]: https://gitlab.freedesktop.org/drm/intel/issues/877
Participating hosts (49 -> 46)
------------------------------
Additional (4): fi-kbl-soraka fi-skl-lmem fi-ivb-3770 fi-pnv-d510
Missing (7): fi-ilk-m540 fi-hsw-4200u fi-skl-6770hq fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-bdw-samus
Build changes
-------------
* CI: CI-20190529 -> None
* IGT: IGT_5453 -> IGTPW_4198
CI-20190529: 20190529
CI_DRM_7973: 07350317e4b2be54b1de7f1e73f77875df5e43f3 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_4198: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/index.html
IGT_5453: cae9a5881ed2c5be2c2518a255740b612a927f9a @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4198/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply
* Re: MCE handler gets NIP wrong on MPC8378
From: Radu Rendec @ 2020-02-20 16:02 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linuxppc-dev
In-Reply-To: <09e9a042-766c-d2e6-2300-cebc372cabde@c-s.fr>
On 02/20/2020 at 3:38 AM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> On 02/19/2020 10:39 PM, Radu Rendec wrote:
> > On 02/19/2020 at 4:21 PM Christophe Leroy <christophe.leroy@c-s.fr> wrote:
> >>> Interesting.
> >>>
> >>> 0x900 is the adress of the timer interrupt.
> >>>
> >>> Would the MCE occur just after the timer interrupt ?
> >
> > I doubt that. I'm using a small test module to artificially trigger the
> > MCE. Basically it's just this (the full code is in my original post):
> >
> > bad_addr_base = ioremap(0xf0000000, 0x100);
> > x = ioread32(bad_addr_base);
> >
> > I find it hard to believe that every time I load the module the lwbrx
> > instruction that triggers the MCE is executed exactly after the timer
> > interrupt (or that the timer interrupt always occurs close to the lwbrx
> > instruction).
>
> Can you try to see how much time there is between your read and the MCE ?
> The below should allow it, you'll see first value in r13 and the other
> in r14 (mce.c is your test code)
>
> Also provide the timebase frequency as reported in /proc/cpuinfo
I just ran a test: r13 is 0xda8e0f91 and r14 is 0xdaae0f9c.
# cat /proc/cpuinfo
processor : 0
cpu : e300c4
clock : 800.000004MHz
revision : 1.1 (pvr 8086 1011)
bogomips : 200.00
timebase : 100000000
The difference between r14 and r13 is 0x20000b. Assuming TB is
incremented with 'timebase' frequency, that means 20.97 milliseconds
(although the e300 manual says TB is "incremented once every four core
input clock cycles").
I repeated the test twice and the absolute values were of course very
different, but r14-r13 was 0x20000c and 0x200011, so it seems to be
quite consistent (within just a few clock cycles).
Just for the fun of it, I repeated the test once more, but with
interrupts disabled. The difference was 0x200014. FWIW, I disabled
interrupts before sampling TB in r13.
> And what's the reason given in the Oops message for the machine check ?
> Is that "Caused by (from SRR1=49030): Transfer error ack signal" or
> something else ?
When interrupts are enabled:
Caused by (from SRR1=41000): Transfer error ack signal
When interrupts are disabled:
Caused by (from SRR1=41030): Transfer error ack signal
> >
> >> Do you use the local bus monitoring driver ?
> >
> > I don't. In fact, I'm not even aware of it. What driver is that?
>
> CONFIG_FSL_LBC
OK, it seems I'm actually using it. I haven't enabled it explicitly, but
it's automatically pulled by CONFIG_MTD_NAND_FSL_ELBC as a prerequisite.
I looked at the code in arch/powerpc/sysdev/fsl_lbc.c and it's quite
small. Most of the code is in fsl_lbc_ctrl_irq, which I guess is
supposed to print a message if/when the LBC catches an error. I've never
seen any of those messages being printed.
Best regards,
Radu
^ permalink raw reply
* Re: [Lsf-pc] [LSF/MM TOPIC] Memory cgroups, whether you like it or not
From: Kirill A. Shutemov @ 2020-02-20 16:06 UTC (permalink / raw)
To: Michal Hocko
Cc: Tim Chen, lsf-pc, linux-mm, Dave Hansen, Dan Williams, Huang Ying
In-Reply-To: <20200214104541.GT31689@dhcp22.suse.cz>
On Fri, Feb 14, 2020 at 11:45:41AM +0100, Michal Hocko wrote:
> On Wed 05-02-20 10:34:57, Tim Chen wrote:
> > There is existing infrastructure for memory soft limit per cgroup we
> > can leverage to implement such a scheme. We'll like to find out if this
> > approach makes sense for people working on systems with multiple memory tiers.
>
> Soft limit is dead.
Michal, could you remind what the deal with soft limit? Why is it dead?
--
Kirill A. Shutemov
^ permalink raw reply
* [PATCH 2/2] virtio: let virtio use DMA API when guest RAM is protected
From: Halil Pasic @ 2020-02-20 16:06 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang, Marek Szyprowski, Robin Murphy,
Christoph Hellwig
Cc: Halil Pasic, linux-s390, virtualization, linux-kernel, iommu,
Christian Borntraeger, Janosch Frank, Viktor Mihajlovski,
Cornelia Huck, Ram Pai, Thiago Jung Bauermann, David Gibson,
Lendacky, Thomas, Michael Mueller
In-Reply-To: <20200220160606.53156-1-pasic@linux.ibm.com>
Currently the advanced guest memory protection technologies (AMD SEV,
powerpc secure guest technology and s390 Protected VMs) abuse the
VIRTIO_F_IOMMU_PLATFORM flag to make virtio core use the DMA API, which
is in turn necessary, to make IO work with guest memory protection.
But VIRTIO_F_IOMMU_PLATFORM a.k.a. VIRTIO_F_ACCESS_PLATFORM is really a
different beast: with virtio devices whose implementation runs on an SMP
CPU we are still fine with doing all the usual optimizations, it is just
that we need to make sure that the memory protection mechanism does not
get in the way. The VIRTIO_F_ACCESS_PLATFORM mandates more work on the
side of the guest (and possibly he host side as well) than we actually
need.
An additional benefit of teaching the guest to make the right decision
(and use DMA API) on it's own is: removing the need, to mandate special
VM configuration for guests that may run with protection. This is
especially interesting for s390 as VIRTIO_F_IOMMU_PLATFORM pushes all
the virtio control structures into the first 2G of guest memory:
something we don't necessarily want to do per-default.
Signed-off-by: Halil Pasic <pasic@linux.ibm.com>
Tested-by: Ram Pai <linuxram@us.ibm.com>
Tested-by: Michael Mueller <mimu@linux.ibm.com>
---
drivers/virtio/virtio_ring.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 867c7ebd3f10..fafc8f924955 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -243,6 +243,9 @@ static bool vring_use_dma_api(struct virtio_device *vdev)
if (!virtio_has_iommu_quirk(vdev))
return true;
+ if (force_dma_unencrypted(&vdev->dev))
+ return true;
+
/* Otherwise, we are left to guess. */
/*
* In theory, it's possible to have a buggy QEMU-supposed
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v4 6/6] regulator: Use driver_deferred_probe_timeout for regulator_init_complete_work
From: Bjorn Andersson @ 2020-02-20 16:05 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Rob Herring, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson,
Pavel Machek, Len Brown, Todd Kjos, Liam Girdwood, Mark Brown,
Thierry Reding, Linus Walleij, Greg Kroah-Hartman, linux-pm
In-Reply-To: <20200220050440.45878-7-john.stultz@linaro.org>
On Wed 19 Feb 21:04 PST 2020, John Stultz wrote:
> The regulator_init_complete_work logic defers the cleanup for an
> arbitrary 30 seconds of time to allow modules loaded by userland
> to start.
>
> This arbitrary timeout is similar to the
> driver_deferred_probe_timeout value, and its been suggested we
> align these so users have a method to extend the timeouts as
> needed.
>
> So this patch changes the logic to use the
> driver_deferred_probe_timeout value if it is set, otherwise we
> directly call the regulator_init_complete_work_function().
>
> Cc: Rob Herring <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: I9fa2411abbb91ed4dd0edc41e8cc8583577c005b
Change-Id...
> ---
> v4:
> * Split out into its own patch, as suggested by Mark
> ---
> drivers/regulator/core.c | 25 ++++++++++++++-----------
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index d015d99cb59d..394e7b11576a 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -5767,18 +5767,21 @@ static int __init regulator_init_complete(void)
> has_full_constraints = true;
>
> /*
> - * We punt completion for an arbitrary amount of time since
> - * systems like distros will load many drivers from userspace
> - * so consumers might not always be ready yet, this is
> - * particularly an issue with laptops where this might bounce
> - * the display off then on. Ideally we'd get a notification
> - * from userspace when this happens but we don't so just wait
> - * a bit and hope we waited long enough. It'd be better if
> - * we'd only do this on systems that need it, and a kernel
> - * command line option might be useful.
> + * If driver_deferred_probe_timeout is set, we punt
> + * completion for that many seconds since systems like
> + * distros will load many drivers from userspace so consumers
> + * might not always be ready yet, this is particularly an
> + * issue with laptops where this might bounce the display off
> + * then on. Ideally we'd get a notification from userspace
> + * when this happens but we don't so just wait a bit and hope
> + * we waited long enough. It'd be better if we'd only do
> + * this on systems that need it.
> */
> - schedule_delayed_work(®ulator_init_complete_work,
> - msecs_to_jiffies(30000));
> + if (driver_deferred_probe_timeout >= 0)
> + schedule_delayed_work(®ulator_init_complete_work,
> + driver_deferred_probe_timeout * HZ);
> + else
> + regulator_init_complete_work_function(NULL);
Why not schedule_delayed_work(..., 0) in this case, to get it off the
initcall context and to avoid the difference in execution paths?
Regards,
Bjorn
>
> return 0;
> }
> --
> 2.17.1
>
^ permalink raw reply
* Re: [RFC PATCH v3 05/27] qcow2: Document the Extended L2 Entries feature
From: Alberto Garcia @ 2020-02-20 16:04 UTC (permalink / raw)
To: Eric Blake, Max Reitz, qemu-devel
Cc: Kevin Wolf, Denis V . Lunev, Anton Nefedov,
Vladimir Sementsov-Ogievskiy, qemu-block
In-Reply-To: <fcaace04-17be-66b2-e0aa-6b1c68b11989@redhat.com>
On Thu 20 Feb 2020 05:02:22 PM CET, Eric Blake wrote:
>>> + Bits are assigned starting from the most significant one.
>>> + (i.e. bit x is used for subcluster 31 - x)
>>
>> I still prefer it the other way round, both personally (e.g. it’s the
>> C ordering), and because other places in qcow2 use LSb for bit
>> ordering (the refcount order).
>
> Internal consistency with refcount order using LSb ordering is the
> strongest reason to flip things, and have bit x be subcluster x.
Ok, I think you're both right, I'll change that.
Berto
^ permalink raw reply
* [PATCH RESEND v6 07/16] powerpc/mm: Use helper fault_signal_pending()
From: Peter Xu @ 2020-02-20 16:02 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Peter Xu, Martin Cracauer, Mike Rapoport, Hugh Dickins,
Jerome Glisse, Kirill A . Shutemov, Matthew Wilcox,
Pavel Emelyanov, Brian Geffon, Maya Gokhale, Denis Plotnikov,
Andrea Arcangeli, Johannes Weiner, Dr . David Alan Gilbert,
Linus Torvalds, Mike Kravetz, Marty McFadden, David Hildenbrand,
Bobby Powers, Mel Gorman
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
Let powerpc code to use the new helper, by moving the signal handling
earlier before the retry logic.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/powerpc/mm/fault.c | 12 ++++--------
1 file changed, 4 insertions(+), 8 deletions(-)
diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 8db0507619e2..0868172ce4e3 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c
@@ -582,6 +582,9 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
major |= fault & VM_FAULT_MAJOR;
+ if (fault_signal_pending(fault, regs))
+ return user_mode(regs) ? 0 : SIGBUS;
+
/*
* Handle the retry right now, the mmap_sem has been released in that
* case.
@@ -595,15 +598,8 @@ static int __do_page_fault(struct pt_regs *regs, unsigned long address,
*/
flags &= ~FAULT_FLAG_ALLOW_RETRY;
flags |= FAULT_FLAG_TRIED;
- if (!fatal_signal_pending(current))
- goto retry;
+ goto retry;
}
-
- /*
- * User mode? Just return to handle the fatal exception otherwise
- * return to bad_page_fault
- */
- return is_user ? 0 : SIGBUS;
}
up_read(¤t->mm->mmap_sem);
--
2.24.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.