* Re: [PATCH 2/2] watchdog: Add new arm_smc_wdt watchdog driver
From: Guenter Roeck @ 2020-02-20 15:52 UTC (permalink / raw)
To: Evan Benn
Cc: Rob Herring, Wim Van Sebroeck, linux-watchdog, Anson Huang,
Dinh Nguyen, Catalin Marinas, LKML, Shawn Guo, Bjorn Andersson,
Marcin Juszkiewicz, Olof Johansson, Clément Péron,
Greg Kroah-Hartman, Jonathan Cameron, Mauro Carvalho Chehab,
Julius Werner, Leonard Crestez, Will Deacon, David S. Miller,
linux-arm-kernel
In-Reply-To: <CAKz_xw0fHgVBLdEoEoQ7OSAgBcvYBAowV0obWLsDUGNPotP55Q@mail.gmail.com>
On Thu, Feb 20, 2020 at 05:50:03PM +1100, Evan Benn wrote:
> > > + if ((int)res->a0 == PSCI_RET_NOT_SUPPORTED)
> > > + return -ENOTSUPP;
> >
> > -ENODEV would be better here.
> >
> > > + if ((int)res->a0 == PSCI_RET_INVALID_PARAMS)
> > > + return -EINVAL;
> > > + if ((int)res->a0 < 0)
> > > + return -EIO;
>
> In fixing this I found drivers/firmware/psci/psci.c:145
> Which also translates psci codes to errno codes, but uses EOPNOTSUPP:
>
> switch (errno) {
> case PSCI_RET_SUCCESS:
> return 0;
> case PSCI_RET_NOT_SUPPORTED:
> return -EOPNOTSUPP;
> case PSCI_RET_INVALID_PARAMS:
> case PSCI_RET_INVALID_ADDRESS:
> return -EINVAL;
> case PSCI_RET_DENIED:
> return -EPERM;
> };
>
> return -EINVAL;
>
> Are these more appropriate?
>
It is customary for driver probe functions to return -ENODEV if the
device is not supported. I don't see a reason why this driver should
behave any different. "but this other driver does it" is never a good
argument.
Having said that, yet, with the exception of -EOPNOTSUPP the other
return values would be more appropriate.
Guenter
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: w83627ehf crash in 5.6.0-rc2-00055-gca7e1fd1026c
From: Dr. David Alan Gilbert @ 2020-02-20 15:52 UTC (permalink / raw)
To: Guenter Roeck; +Cc: Meelis Roos, linux-hwmon, LKML, Chen Zhou
In-Reply-To: <ad023dc1-1878-dcd9-a183-06003ed698af@roeck-us.net>
* Guenter Roeck (linux@roeck-us.net) wrote:
> On 2/20/20 5:57 AM, Dr. David Alan Gilbert wrote:
> > * Meelis Roos (mroos@linux.ee) wrote:
> > > > It looks like not all chips have temp_label, so I think we need to change w83627ehf_is_visible
> > > > which has:
> > > >
> > > > if (attr == hwmon_temp_input || attr == hwmon_temp_label)
> > > > return 0444;
> > > >
> > > > to
> > > > if (attr == hwmon_temp_input)
> > > > return 0444;
> > > > if (attr == hwmon_temp_label) {
> > > > if (data->temp_label)
> > > > return 0444;
> > > > else
> > > > return 0;
>
> Nitpick: else after return isn't necessary. Too bad I didn't notice that before;
> static analyzers will have a field day :-)
>
> > > > }
> > > >
> > > > Does that work for you?
> > > Yes, it works - sensors are displayed as they should be, with nothing in dmesg.
> > >
> > > Thank you for so quick response!
> >
> > Great, I need to turn that into a proper patch; (I might need to wait till
> > Saturday for that, although if someone needs it before then please shout).
> >
>
> We'll want this fixed in the next stable release candidate, so I wrote one up
> and submitted it.
Thanks!
Dave
> Thanks,
> Guenter
--
-----Open up your eyes, open up your mind, open up your code -------
/ Dr. David Alan Gilbert | Running GNU/Linux | Happy \
\ dave @ treblig.org | | In Hex /
\ _________________________|_____ http://www.treblig.org |_______/
^ permalink raw reply
* Re: [PATCH RFC] Toggle fifo write clk after ungating sdcc clk
From: Bjorn Andersson @ 2020-02-20 15:51 UTC (permalink / raw)
To: Sayali Lokhande
Cc: adrian.hunter, robh+dt, ulf.hansson, asutoshd, stummala, ppvk,
rampraka, vbadigan, sboyd, georgi.djakov, mka, linux-mmc,
linux-kernel, linux-arm-msm, devicetree, agross, linux-mmc-owner
In-Reply-To: <1582190446-4778-1-git-send-email-sayalil@codeaurora.org>
On Thu 20 Feb 01:20 PST 2020, Sayali Lokhande wrote:
> During GCC level clock gating of MCLK, the async FIFO
> gets into some hang condition, such that for the next
> transfer after MCLK ungating, first bit of CMD response
> doesn't get written in to the FIFO. This cause the CPSM
> to hang eventually leading to SW timeout.
>
> To fix the issue, toggle the FIFO write clock after
> MCLK ungated to get the FIFO pointers and flags to
> valid states.
>
Please don't provide cover letters for a single patch.
Regards,
Bjorn
> Ram Prakash Gupta (1):
> mmc: sdhci-msm: Toggle fifo write clk after ungating sdcc clk
>
> drivers/mmc/host/sdhci-msm.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
^ permalink raw reply
* Re: [Intel-gfx] [PATCH v1] drm/i915: Use intel_plane_data_rate for min_cdclk calculation
From: Lisovskiy, Stanislav @ 2020-02-20 15:52 UTC (permalink / raw)
To: ville.syrjala@linux.intel.com
Cc: Nikula, Jani, intel-gfx@lists.freedesktop.org
In-Reply-To: <20200220154311.GD13686@intel.com>
On Thu, 2020-02-20 at 17:43 +0200, Ville Syrjälä wrote:
> On Thu, Feb 20, 2020 at 05:23:47PM +0200, Stanislav Lisovskiy wrote:
> > There seems to be a bit of confusing redundancy in a way, how
> > plane data rate/min cdclk are calculated.
> > In fact both min cdclk, pixel rate and plane data rate are all
> > part of the same formula as per BSpec.
> >
> > However currently we have intel_plane_data_rate, which is used
> > to calculate plane data rate and which is also used in bandwidth
> > calculations. However for calculating min_cdclk we have another
> > piece of code, doing almost same calculation, but a bit differently
> > and in a different place. However as both are actually part of same
> > formula, probably would be wise to use plane data rate calculations
> > as a basis anyway, thus avoiding code duplication and possible bugs
> > related to this.
> >
> > Another thing is that I've noticed that during min_cdclk
> > calculations
> > we account for plane scaling, while for plane data rate, we don't.
> > crtc->pixel_rate seems to account only for pipe ratio, however it
> > is
> > clearly stated in BSpec that plane data rate also need to account
> > plane ratio as well.
> >
> > So what this commit does is:
> > - Adds a plane ratio calculation to intel_plane_data_rate
> > - Removes redundant calculations from skl_plane_min_cdclk which is
> > used for gen9+ and now uses intel_plane_data_rate as a basis from
> > there as well.
> >
> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> > .../gpu/drm/i915/display/intel_atomic_plane.c | 16 ++++++-
> > drivers/gpu/drm/i915/display/intel_sprite.c | 46 +++++++++++--
> > ------
> > 2 files changed, 41 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index c86d7a35c816..702dfa14d112 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -133,15 +133,27 @@ intel_plane_destroy_state(struct drm_plane
> > *plane,
> > kfree(plane_state);
> > }
> >
> > +
> > +
> > unsigned int intel_plane_data_rate(const struct intel_crtc_state
> > *crtc_state,
> > const struct intel_plane_state
> > *plane_state)
> > {
> > const struct drm_framebuffer *fb = plane_state->hw.fb;
> > unsigned int cpp;
> > + unsigned int src_w, src_h, dst_w, dst_h;
> >
> > if (!plane_state->uapi.visible)
> > return 0;
> >
> > + src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
> > + src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
> > + dst_w = drm_rect_width(&plane_state->uapi.dst);
> > + dst_h = drm_rect_height(&plane_state->uapi.dst);
> > +
> > + /* Downscaling limits the maximum pixel rate */
> > + dst_w = min(src_w, dst_w);
> > + dst_h = min(src_h, dst_h);
> > +
> > cpp = fb->format->cpp[0];
> >
> > /*
> > @@ -153,7 +165,9 @@ unsigned int intel_plane_data_rate(const struct
> > intel_crtc_state *crtc_state,
> > if (fb->format->is_yuv && fb->format->num_planes > 1)
> > cpp *= 4;
> >
> > - return cpp * crtc_state->pixel_rate;
> > + return DIV64_U64_ROUND_UP(mul_u32_u32(cpp * crtc_state-
> > >pixel_rate,
> > + src_w * src_h),
> > + mul_u32_u32(dst_w, dst_h));
>
> You don't need a 64bit divisor for this.
>
> > }
> >
> > int intel_plane_calc_min_cdclk(struct intel_atomic_state *state,
> > diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c
> > b/drivers/gpu/drm/i915/display/intel_sprite.c
> > index 7abeefe8dce5..75afb78ff1b0 100644
> > --- a/drivers/gpu/drm/i915/display/intel_sprite.c
> > +++ b/drivers/gpu/drm/i915/display/intel_sprite.c
> > @@ -330,24 +330,34 @@ bool icl_is_hdr_plane(struct drm_i915_private
> > *dev_priv, enum plane_id plane_id)
> > }
> >
> > static void
> > -skl_plane_ratio(const struct intel_crtc_state *crtc_state,
> > - const struct intel_plane_state *plane_state,
> > - unsigned int *num, unsigned int *den)
> > +skl_plane_bpp_constraints(const struct intel_crtc_state
> > *crtc_state,
> > + const struct intel_plane_state *plane_state,
> > + unsigned int *num, unsigned int *den)
> > {
> > struct drm_i915_private *dev_priv = to_i915(plane_state-
> > >uapi.plane->dev);
> > const struct drm_framebuffer *fb = plane_state->hw.fb;
> > + unsigned int cpp = fb->format->cpp[0];
> > +
> > + /*
> > + * Based on HSD#:1408715493
> > + * NV12 cpp == 4, P010 cpp == 8
> > + *
> > + * FIXME what is the logic behind this?
> > + */
> > + if (fb->format->is_yuv && fb->format->num_planes > 1)
> > + cpp *= 4;
>
> This is ugly. I think we need a plane pixel rate instead of
> abusing the data rate as the pixel rate like this.
Yeah, agree, but that is all because of this HSD#-something workaround,
which is preventing to use plane_data_rate in a formula "as is".
Was almost sure that this part is questionable. Probably the best
way would be to extract some function like you say to calculate
per-plane pixel rate, which would be used in both intel_plane_data_rate
and skl_calc_min_cdclk.
The thing what I just aim to eliminate is a duplication on this matter.
I mean if we really calculate it as per BSpec it should be
Plane data rate = Pixel rate * bpp * plane scale ratio * pipe scale
ratio
For recent Gens Pixel rate = Min CDCLK / 2, so all of those are
obviously part of same formula, same should be in the code.
Stan
>
> >
> > if (fb->format->cpp[0] == 8) {
> > if (INTEL_GEN(dev_priv) >= 10 ||
> > IS_GEMINILAKE(dev_priv)) {
> > *num = 10;
> > - *den = 8;
> > + *den = 8 * cpp;
> > } else {
> > *num = 9;
> > - *den = 8;
> > + *den = 8 * cpp;
> > }
> > } else {
> > *num = 1;
> > - *den = 1;
> > + *den = cpp;
> > }
> > }
> >
> > @@ -355,27 +365,23 @@ static int skl_plane_min_cdclk(const struct
> > intel_crtc_state *crtc_state,
> > const struct intel_plane_state
> > *plane_state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(plane_state-
> > >uapi.plane->dev);
> > - unsigned int pixel_rate = crtc_state->pixel_rate;
> > - unsigned int src_w, src_h, dst_w, dst_h;
> > unsigned int num, den;
> > + struct intel_plane *plane = to_intel_plane(plane_state-
> > >uapi.plane);
> >
> > - skl_plane_ratio(crtc_state, plane_state, &num, &den);
> > + skl_plane_bpp_constraints(crtc_state, plane_state, &num, &den);
> >
> > /* two pixels per clock on glk+ */
> > if (INTEL_GEN(dev_priv) >= 10 || IS_GEMINILAKE(dev_priv))
> > den *= 2;
> >
> > - src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
> > - src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
> > - dst_w = drm_rect_width(&plane_state->uapi.dst);
> > - dst_h = drm_rect_height(&plane_state->uapi.dst);
> > -
> > - /* Downscaling limits the maximum pixel rate */
> > - dst_w = min(src_w, dst_w);
> > - dst_h = min(src_h, dst_h);
> > -
> > - return DIV64_U64_ROUND_UP(mul_u32_u32(pixel_rate * num, src_w *
> > src_h),
> > - mul_u32_u32(den, dst_w * dst_h));
> > + /*
> > + * intel_atomic_check_planes has already been called by this
> > + * time in intel_atomic_check, so use calculated plane
> > + * data rate as a basis, in order not to have duplicate code.
> > + * According to BSpec, plane data rate is anyway used as
> > + * a basis for this calculation.
> > + */
> > + return DIV64_U64_ROUND_UP(crtc_state->data_rate[plane->id] *
> > num, den);
> > }
> >
> > static unsigned int
> > --
> > 2.24.1.485.gad05a3d8e5
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [igt-dev] [PATCH i-g-t] intel-ci: add a pre-merge blacklist to reduce the testing queue
From: Chris Wilson @ 2020-02-20 15:52 UTC (permalink / raw)
To: Martin Peres, igt-dev
In-Reply-To: <20200220153209.210767-1-martin.peres@linux.intel.com>
Quoting Martin Peres (2020-02-20 15:32:09)
> +###############################################################################
> +# This test virtually never failed, yet is responsible for a relatively big
> +# execution time on some platforms:
> +#
> +# - shard-skl: 1.3% (~3 minutes)
> +# - shard-kbl: 0.3% (~0.3 minutes)
> +# - shard-apl: 0.6% (~1 minute)
> +# - shard-glk: 0.4% (50 seconds)
> +# - shard-icl: 0.1% (15 seconds)
> +# - shard-tgl: 0.1% (15 seconds)
> +#
> +# Data acquired on 2020-02-20 by Martin Peres
> +###############################################################################
> +igt@sw_sync@sync_expired_merge
Change from fixed workload to fixed runtime.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply
* Re: [Xen-devel] [PATCH] rwlock: allow recursive read locking when already locked in write mode
From: Jan Beulich @ 2020-02-20 15:52 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Jürgen Groß, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
xen-devel
In-Reply-To: <20200220152004.GN4679@Air-de-Roger>
On 20.02.2020 16:20, Roger Pau Monné wrote:
> On Thu, Feb 20, 2020 at 04:11:08PM +0100, Jan Beulich wrote:
>> On 20.02.2020 15:38, Roger Pau Monné wrote:
>>> On Thu, Feb 20, 2020 at 03:23:38PM +0100, Jürgen Groß wrote:
>>>> On 20.02.20 15:11, Roger Pau Monné wrote:
>>>>> On Thu, Feb 20, 2020 at 01:48:54PM +0100, Jan Beulich wrote:
>>>>>> On 20.02.2020 13:02, Roger Pau Monne wrote:
>>>>>>> @@ -166,7 +180,8 @@ static inline void _write_unlock(rwlock_t *lock)
>>>>>>> * If the writer field is atomic, it can be cleared directly.
>>>>>>> * Otherwise, an atomic subtraction will be used to clear it.
>>>>>>> */
>>>>>>> - atomic_sub(_QW_LOCKED, &lock->cnts);
>>>>>>> + ASSERT(_is_write_locked_by_me(atomic_read(&lock->cnts)));
>>>>>>> + atomic_sub(_write_lock_val(), &lock->cnts);
>>>>>>
>>>>>> I think this would be more efficient with atomic_and(), not
>>>>>> the least because of the then avoided smp_processor_id().
>>>>>> Whether to mask off just _QW_WMASK or also the CPU number of
>>>>>> the last write owner would need to be determined. But with
>>>>>> using subtraction, in case of problems it'll likely be
>>>>>> harder to understand what actually went on, from looking at
>>>>>> the resulting state of the lock (this is in part a pre-
>>>>>> existing problem, but gets worse with subtraction of CPU
>>>>>> numbers).
>>>>>
>>>>> Right, a mask would be better. Right now both need to be cleared (the
>>>>> LOCKED and the CPU fields) as there's code that relies on !lock->cnts
>>>>> as a way to determine that the lock is not read or write locked. If we
>>>>> left the CPU lying around those checks would need to be adjusted.
>>>>
>>>> In case you make _QR_SHIFT 16 it is possible to just write a 2-byte zero
>>>> value for write_unlock() (like its possible to do so today using a
>>>> single byte write).
>>>
>>> That would limit the readers count to 65536, what do you think Jan?
>>
>> If the recurse_cpu approach is considered bad, I think this would
>> be acceptable. But of course you'll need to consult with the Arm
>> guys regarding the correctness of such a "half" store in their
>> memory model; I would hope this to be universally okay, but I'm
>> not entirely certain.
>
> I would like to get confirmation from the Arm folks, but I see Arm has
> write_atomic and supports such operation against a uint16_t, so I
> don't see why it wouldn't work.
The operations individually being available doesn't mean that
a combination of 32-bit locked accesses and a 16-bit plain
store are going to produce a consistent result. Perhaps I'm
over-cautious here, but I've been bitten by a vaguely similar
issue on x86 back in the ticket lock (in Linux) days.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* Re: Question about pressure sensor driver data processing
From: Jean-Baptiste Maneyrol @ 2020-02-20 15:51 UTC (permalink / raw)
To: Jonathan Cameron; +Cc: linux-iio
In-Reply-To: <20200219183837.00002aac@Huawei.com>
Hi Jonathan,
I will send first a generic driver returning processed pressure data and raw temperature data and without any buffer/trigger support.
I think this is the most valuable features for this driver.
Afterward, I am interested to add buffer support using hrtimer trigger.
Thanks for your feedback.
JB
From: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Sent: Wednesday, February 19, 2020 19:38
To: Jean-Baptiste Maneyrol <JManeyrol@invensense.com>
Cc: linux-iio <linux-iio@vger.kernel.org>
Subject: Re: Question about pressure sensor driver data processing
CAUTION: This email originated from outside of the organization. Please make sure the sender is who they say they are and do not click links or open attachments unless you recognize the sender and know the content is safe.
On Tue, 18 Feb 2020 15:59:26 +0000
Jean-Baptiste Maneyrol <JManeyrol@invensense.com> wrote:
> Hello,
>
> I have a question concerning a pressure sensor driver I am currently
> writing.
>
> The formula to get the real pressure in Pa for this sensor is quite
> complex. It depends on the measured temperature and would be far
> better done in floats rather than in integers.
>
> The formula is too complex to be expressed with only scale and offset
> factors. And the factors are not fixed since it depends on the
> temperature.
>
> Would it be acceptable to have a driver than returns the raw data
> without processing? Meaning data that have no unit and require a
> processing done in userspace? That would be much more efficient for
> sure, but it would not output really useful data without the
> processing.
Whilst it isn't nice, we have examples where this is already the case.
The heart rate sensors are similar in that they have very complex
conversions.
It is certainly better than being unable to support the driver at all,
but it does mean you won't ever be able to use it with generic code.
Generic code tends to assume the scale is constant as well, so probably
wouldn't work anyway :)
>
> Or a driver that return an input processed data and a raw data that
> have no unit and requires the processing? If we can return raw data,
> we should be able also to add buffer/trigger support to the driver.
> Otherwise that would be quite tricky to return the processed data in
> the buffer.
Hmm. It is messy so if you actually expect to add buffered support, then
we will need special code in userspace for this device anyway.
We do have devices that jump through a complex conversion then pack
it into a buffer, but that is also somewhat of a hack.
For now at least, go with the raw output only. It may be one of those
exceptions where we do support both processed and raw outputs in the
longer term if there is clear need for generic code with this device.
Jonathan
>
> Thanks for your feedback.
> JB
^ permalink raw reply
* Re: [dpdk-dev] [PATCH v3] examples/tep_term: remove redundant info get
From: David Marchand @ 2020-02-20 15:53 UTC (permalink / raw)
To: Xiaolong Ye; +Cc: dev, Xiaoyun Li, dpdk stable
In-Reply-To: <20200217014115.26572-1-xiaoyun.li@intel.com>
On Mon, Feb 17, 2020 at 2:41 AM Xiaoyun Li <xiaoyun.li@intel.com> wrote:
>
> Removed redundant function call of 'rte_eth_dev_info_get()' since it has
> already been called earlier.
>
> Coverity issue: 349922
> Fixes: 2bb43bd4350a ("examples/tep_term: add TSO offload configuration")
> Cc: stable@dpdk.org
>
> Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
LGTM.
Xiaolong, you had reviewed it before, can you have a look?
Thanks.
--
David Marchand
^ permalink raw reply
* [PATCH RESEND v6 01/16] mm/gup: Rename "nonblocking" to "locked" where proper
From: Peter Xu @ 2020-02-20 15:53 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Andrea Arcangeli, Martin Cracauer, Linus Torvalds, Mike Rapoport,
Kirill A . Shutemov, Johannes Weiner, Dr . David Alan Gilbert,
David Hildenbrand, Bobby Powers, Maya Gokhale, Jerome Glisse,
Mike Kravetz, Matthew Wilcox, Marty McFadden, Mel Gorman, peterx,
Hugh Dickins, Brian Geffon, Denis Plotnikov, Pavel Emelyanov
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
There's plenty of places around __get_user_pages() that has a parameter
"nonblocking" which does not really mean that "it won't block" (because
it can really block) but instead it shows whether the mmap_sem is
released by up_read() during the page fault handling mostly when
VM_FAULT_RETRY is returned.
We have the correct naming in e.g. get_user_pages_locked() or
get_user_pages_remote() as "locked", however there're still many places
that are using the "nonblocking" as name.
Renaming the places to "locked" where proper to better suite the
functionality of the variable. While at it, fixing up some of the
comments accordingly.
Reviewed-by: Mike Rapoport <rppt@linux.vnet.ibm.com>
Reviewed-by: Jerome Glisse <jglisse@redhat.com>
Reviewed-by: David Hildenbrand <david@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
mm/gup.c | 44 +++++++++++++++++++++-----------------------
mm/hugetlb.c | 8 ++++----
2 files changed, 25 insertions(+), 27 deletions(-)
diff --git a/mm/gup.c b/mm/gup.c
index 1b521e0ac1de..1b4411bd0042 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -630,12 +630,12 @@ static int get_gate_page(struct mm_struct *mm, unsigned long address,
}
/*
- * mmap_sem must be held on entry. If @nonblocking != NULL and
- * *@flags does not include FOLL_NOWAIT, the mmap_sem may be released.
- * If it is, *@nonblocking will be set to 0 and -EBUSY returned.
+ * mmap_sem must be held on entry. If @locked != NULL and *@flags
+ * does not include FOLL_NOWAIT, the mmap_sem may be released. If it
+ * is, *@locked will be set to 0 and -EBUSY returned.
*/
static int faultin_page(struct task_struct *tsk, struct vm_area_struct *vma,
- unsigned long address, unsigned int *flags, int *nonblocking)
+ unsigned long address, unsigned int *flags, int *locked)
{
unsigned int fault_flags = 0;
vm_fault_t ret;
@@ -647,7 +647,7 @@ static int faultin_page(struct task_struct *tsk, struct vm_area_struct *vma,
fault_flags |= FAULT_FLAG_WRITE;
if (*flags & FOLL_REMOTE)
fault_flags |= FAULT_FLAG_REMOTE;
- if (nonblocking)
+ if (locked)
fault_flags |= FAULT_FLAG_ALLOW_RETRY;
if (*flags & FOLL_NOWAIT)
fault_flags |= FAULT_FLAG_ALLOW_RETRY | FAULT_FLAG_RETRY_NOWAIT;
@@ -673,8 +673,8 @@ static int faultin_page(struct task_struct *tsk, struct vm_area_struct *vma,
}
if (ret & VM_FAULT_RETRY) {
- if (nonblocking && !(fault_flags & FAULT_FLAG_RETRY_NOWAIT))
- *nonblocking = 0;
+ if (locked && !(fault_flags & FAULT_FLAG_RETRY_NOWAIT))
+ *locked = 0;
return -EBUSY;
}
@@ -751,7 +751,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
* only intends to ensure the pages are faulted in.
* @vmas: array of pointers to vmas corresponding to each page.
* Or NULL if the caller does not require them.
- * @nonblocking: whether waiting for disk IO or mmap_sem contention
+ * @locked: whether we're still with the mmap_sem held
*
* Returns either number of pages pinned (which may be less than the
* number requested), or an error. Details about the return value:
@@ -786,13 +786,11 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
* appropriate) must be called after the page is finished with, and
* before put_page is called.
*
- * If @nonblocking != NULL, __get_user_pages will not wait for disk IO
- * or mmap_sem contention, and if waiting is needed to pin all pages,
- * *@nonblocking will be set to 0. Further, if @gup_flags does not
- * include FOLL_NOWAIT, the mmap_sem will be released via up_read() in
- * this case.
+ * If @locked != NULL, *@locked will be set to 0 when mmap_sem is
+ * released by an up_read(). That can happen if @gup_flags does not
+ * have FOLL_NOWAIT.
*
- * A caller using such a combination of @nonblocking and @gup_flags
+ * A caller using such a combination of @locked and @gup_flags
* must therefore hold the mmap_sem for reading only, and recognize
* when it's been released. Otherwise, it must be held for either
* reading or writing and will not be released.
@@ -804,7 +802,7 @@ static int check_vma_flags(struct vm_area_struct *vma, unsigned long gup_flags)
static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
unsigned long start, unsigned long nr_pages,
unsigned int gup_flags, struct page **pages,
- struct vm_area_struct **vmas, int *nonblocking)
+ struct vm_area_struct **vmas, int *locked)
{
long ret = 0, i = 0;
struct vm_area_struct *vma = NULL;
@@ -850,7 +848,7 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
if (is_vm_hugetlb_page(vma)) {
i = follow_hugetlb_page(mm, vma, pages, vmas,
&start, &nr_pages, i,
- gup_flags, nonblocking);
+ gup_flags, locked);
continue;
}
}
@@ -868,7 +866,7 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
page = follow_page_mask(vma, start, foll_flags, &ctx);
if (!page) {
ret = faultin_page(tsk, vma, start, &foll_flags,
- nonblocking);
+ locked);
switch (ret) {
case 0:
goto retry;
@@ -1129,7 +1127,7 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk,
* @vma: target vma
* @start: start address
* @end: end address
- * @nonblocking:
+ * @locked: whether the mmap_sem is still held
*
* This takes care of mlocking the pages too if VM_LOCKED is set.
*
@@ -1137,14 +1135,14 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk,
*
* vma->vm_mm->mmap_sem must be held.
*
- * If @nonblocking is NULL, it may be held for read or write and will
+ * If @locked is NULL, it may be held for read or write and will
* be unperturbed.
*
- * If @nonblocking is non-NULL, it must held for read only and may be
- * released. If it's released, *@nonblocking will be set to 0.
+ * If @locked is non-NULL, it must held for read only and may be
+ * released. If it's released, *@locked will be set to 0.
*/
long populate_vma_page_range(struct vm_area_struct *vma,
- unsigned long start, unsigned long end, int *nonblocking)
+ unsigned long start, unsigned long end, int *locked)
{
struct mm_struct *mm = vma->vm_mm;
unsigned long nr_pages = (end - start) / PAGE_SIZE;
@@ -1179,7 +1177,7 @@ long populate_vma_page_range(struct vm_area_struct *vma,
* not result in a stack expansion that recurses back here.
*/
return __get_user_pages(current, mm, start, nr_pages, gup_flags,
- NULL, NULL, nonblocking);
+ NULL, NULL, locked);
}
/*
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index dd8737a94bec..c84f721db020 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -4266,7 +4266,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
struct page **pages, struct vm_area_struct **vmas,
unsigned long *position, unsigned long *nr_pages,
- long i, unsigned int flags, int *nonblocking)
+ long i, unsigned int flags, int *locked)
{
unsigned long pfn_offset;
unsigned long vaddr = *position;
@@ -4337,7 +4337,7 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
spin_unlock(ptl);
if (flags & FOLL_WRITE)
fault_flags |= FAULT_FLAG_WRITE;
- if (nonblocking)
+ if (locked)
fault_flags |= FAULT_FLAG_ALLOW_RETRY;
if (flags & FOLL_NOWAIT)
fault_flags |= FAULT_FLAG_ALLOW_RETRY |
@@ -4354,9 +4354,9 @@ long follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
break;
}
if (ret & VM_FAULT_RETRY) {
- if (nonblocking &&
+ if (locked &&
!(fault_flags & FAULT_FLAG_RETRY_NOWAIT))
- *nonblocking = 0;
+ *locked = 0;
*nr_pages = 0;
/*
* VM_FAULT_RETRY must not return an
--
2.24.1
^ permalink raw reply related
* [PATCH RESEND v6 02/16] mm/gup: Fix __get_user_pages() on fault retry of hugetlb
From: Peter Xu @ 2020-02-20 15:53 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Andrea Arcangeli, Martin Cracauer, Linus Torvalds, Mike Rapoport,
Kirill A . Shutemov, Johannes Weiner, Dr . David Alan Gilbert,
David Hildenbrand, Bobby Powers, Maya Gokhale, Jerome Glisse,
Mike Kravetz, Matthew Wilcox, Marty McFadden, Mel Gorman, peterx,
Hugh Dickins, Brian Geffon, Denis Plotnikov, Pavel Emelyanov
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
When follow_hugetlb_page() returns with *locked==0, it means we've got
a VM_FAULT_RETRY within the fauling process and we've released the
mmap_sem. When that happens, we should stop and bail out.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
mm/gup.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/mm/gup.c b/mm/gup.c
index 1b4411bd0042..76cb420c0fb7 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -849,6 +849,16 @@ static long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm,
i = follow_hugetlb_page(mm, vma, pages, vmas,
&start, &nr_pages, i,
gup_flags, locked);
+ if (locked && *locked == 0) {
+ /*
+ * We've got a VM_FAULT_RETRY
+ * and we've lost mmap_sem.
+ * We must stop here.
+ */
+ BUG_ON(gup_flags & FOLL_NOWAIT);
+ BUG_ON(ret != 0);
+ goto out;
+ }
continue;
}
}
--
2.24.1
^ permalink raw reply related
* [PATCH RESEND v6 03/16] mm: Introduce fault_signal_pending()
From: Peter Xu @ 2020-02-20 15:53 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Andrea Arcangeli, Martin Cracauer, Linus Torvalds, Mike Rapoport,
Kirill A . Shutemov, Johannes Weiner, Dr . David Alan Gilbert,
David Hildenbrand, Bobby Powers, Maya Gokhale, Jerome Glisse,
Mike Kravetz, Matthew Wilcox, Marty McFadden, Mel Gorman, peterx,
Hugh Dickins, Brian Geffon, Denis Plotnikov, Pavel Emelyanov
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
For most architectures, we've got a quick path to detect fatal signal
after a handle_mm_fault(). Introduce a helper for that quick path.
It cleans the current codes a bit so we don't need to duplicate the
same check across archs. More importantly, this will be an unified
place that we handle the signal immediately right after an interrupted
page fault, so it'll be much easier for us if we want to change the
behavior of handling signals later on for all the archs.
Note that currently only part of the archs are using this new helper,
because some archs have their own way to handle signals. In the
follow up patches, we'll try to apply this helper to all the rest of
archs.
Another note is that the "regs" parameter in the new helper is not
used yet. It'll be used very soon. Now we kept it in this patch only
to avoid touching all the archs again in the follow up patches.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/alpha/mm/fault.c | 2 +-
arch/arm/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/riscv/mm/fault.c | 2 +-
arch/s390/mm/fault.c | 3 +--
arch/sparc/mm/fault_32.c | 2 +-
arch/sparc/mm/fault_64.c | 2 +-
arch/unicore32/mm/fault.c | 2 +-
arch/xtensa/mm/fault.c | 2 +-
include/linux/sched/signal.h | 13 +++++++++++++
18 files changed, 30 insertions(+), 18 deletions(-)
diff --git a/arch/alpha/mm/fault.c b/arch/alpha/mm/fault.c
index 741e61ef9d3f..aea33b599037 100644
--- a/arch/alpha/mm/fault.c
+++ b/arch/alpha/mm/fault.c
@@ -150,7 +150,7 @@ do_page_fault(unsigned long address, unsigned long mmcsr,
the fault. */
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/arm/mm/fault.c b/arch/arm/mm/fault.c
index bd0f4821f7e1..937b81ff8649 100644
--- a/arch/arm/mm/fault.c
+++ b/arch/arm/mm/fault.c
@@ -295,7 +295,7 @@ do_page_fault(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
* signal first. We do not need to release the mmap_sem because
* it would already be released in __lock_page_or_retry in
* mm/filemap.c. */
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current)) {
+ if (fault_signal_pending(fault, regs)) {
if (!user_mode(regs))
goto no_context;
return 0;
diff --git a/arch/hexagon/mm/vm_fault.c b/arch/hexagon/mm/vm_fault.c
index b3bc71680ae4..d19beaf11b4c 100644
--- a/arch/hexagon/mm/vm_fault.c
+++ b/arch/hexagon/mm/vm_fault.c
@@ -91,7 +91,7 @@ void do_page_fault(unsigned long address, long cause, struct pt_regs *regs)
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return;
/* The most common case -- we are done. */
diff --git a/arch/ia64/mm/fault.c b/arch/ia64/mm/fault.c
index c2f299fe9e04..211b4f439384 100644
--- a/arch/ia64/mm/fault.c
+++ b/arch/ia64/mm/fault.c
@@ -141,7 +141,7 @@ ia64_do_page_fault (unsigned long address, unsigned long isr, struct pt_regs *re
*/
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/m68k/mm/fault.c b/arch/m68k/mm/fault.c
index e9b1d7585b43..a455e202691b 100644
--- a/arch/m68k/mm/fault.c
+++ b/arch/m68k/mm/fault.c
@@ -138,7 +138,7 @@ int do_page_fault(struct pt_regs *regs, unsigned long address,
fault = handle_mm_fault(vma, address, flags);
pr_debug("handle_mm_fault returns %x\n", fault);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return 0;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/microblaze/mm/fault.c b/arch/microblaze/mm/fault.c
index e6a810b0c7ad..cdde01dcdfc3 100644
--- a/arch/microblaze/mm/fault.c
+++ b/arch/microblaze/mm/fault.c
@@ -217,7 +217,7 @@ void do_page_fault(struct pt_regs *regs, unsigned long address,
*/
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/mips/mm/fault.c b/arch/mips/mm/fault.c
index 1e8d00793784..0ee6fafc57bc 100644
--- a/arch/mips/mm/fault.c
+++ b/arch/mips/mm/fault.c
@@ -154,7 +154,7 @@ static void __kprobes __do_page_fault(struct pt_regs *regs, unsigned long write,
*/
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(regs))
return;
perf_sw_event(PERF_COUNT_SW_PAGE_FAULTS, 1, regs, address);
diff --git a/arch/nds32/mm/fault.c b/arch/nds32/mm/fault.c
index 906dfb25353c..0e63f81eff5b 100644
--- a/arch/nds32/mm/fault.c
+++ b/arch/nds32/mm/fault.c
@@ -214,7 +214,7 @@ void do_page_fault(unsigned long entry, unsigned long addr,
* signal first. We do not need to release the mmap_sem because it
* would already be released in __lock_page_or_retry in mm/filemap.c.
*/
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current)) {
+ if (fault_signal_pending(fault, regs)) {
if (!user_mode(regs))
goto no_context;
return;
diff --git a/arch/nios2/mm/fault.c b/arch/nios2/mm/fault.c
index 6a2e716b959f..704ace8ca0ee 100644
--- a/arch/nios2/mm/fault.c
+++ b/arch/nios2/mm/fault.c
@@ -133,7 +133,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long cause,
*/
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/openrisc/mm/fault.c b/arch/openrisc/mm/fault.c
index 5d4d3a9691d0..85c7eb0c0186 100644
--- a/arch/openrisc/mm/fault.c
+++ b/arch/openrisc/mm/fault.c
@@ -161,7 +161,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs, unsigned long address,
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/parisc/mm/fault.c b/arch/parisc/mm/fault.c
index adbd5e2144a3..f9be1d1cb43f 100644
--- a/arch/parisc/mm/fault.c
+++ b/arch/parisc/mm/fault.c
@@ -304,7 +304,7 @@ void do_page_fault(struct pt_regs *regs, unsigned long code,
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/riscv/mm/fault.c b/arch/riscv/mm/fault.c
index cf7248e07f43..1d3869e9ddef 100644
--- a/arch/riscv/mm/fault.c
+++ b/arch/riscv/mm/fault.c
@@ -117,7 +117,7 @@ asmlinkage void do_page_fault(struct pt_regs *regs)
* signal first. We do not need to release the mmap_sem because it
* would already be released in __lock_page_or_retry in mm/filemap.c.
*/
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(tsk))
+ if (fault_signal_pending(fault, regs))
return;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/s390/mm/fault.c b/arch/s390/mm/fault.c
index 7b0bb475c166..179cf92a56e5 100644
--- a/arch/s390/mm/fault.c
+++ b/arch/s390/mm/fault.c
@@ -480,8 +480,7 @@ static inline vm_fault_t do_exception(struct pt_regs *regs, int access)
* the fault.
*/
fault = handle_mm_fault(vma, address, flags);
- /* No reason to continue if interrupted by SIGKILL. */
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current)) {
+ if (fault_signal_pending(fault, regs)) {
fault = VM_FAULT_SIGNAL;
if (flags & FAULT_FLAG_RETRY_NOWAIT)
goto out_up;
diff --git a/arch/sparc/mm/fault_32.c b/arch/sparc/mm/fault_32.c
index 89976c9b936c..6efbeb227644 100644
--- a/arch/sparc/mm/fault_32.c
+++ b/arch/sparc/mm/fault_32.c
@@ -237,7 +237,7 @@ asmlinkage void do_sparc_fault(struct pt_regs *regs, int text_fault, int write,
*/
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/sparc/mm/fault_64.c b/arch/sparc/mm/fault_64.c
index 8b7ddbd14b65..dd1ed6555831 100644
--- a/arch/sparc/mm/fault_64.c
+++ b/arch/sparc/mm/fault_64.c
@@ -425,7 +425,7 @@ asmlinkage void __kprobes do_sparc64_fault(struct pt_regs *regs)
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
goto exit_exception;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/arch/unicore32/mm/fault.c b/arch/unicore32/mm/fault.c
index 76342de9cf8c..59d0e6ec2cfc 100644
--- a/arch/unicore32/mm/fault.c
+++ b/arch/unicore32/mm/fault.c
@@ -250,7 +250,7 @@ static int do_pf(unsigned long addr, unsigned int fsr, struct pt_regs *regs)
* signal first. We do not need to release the mmap_sem because
* it would already be released in __lock_page_or_retry in
* mm/filemap.c. */
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return 0;
if (!(fault & VM_FAULT_ERROR) && (flags & FAULT_FLAG_ALLOW_RETRY)) {
diff --git a/arch/xtensa/mm/fault.c b/arch/xtensa/mm/fault.c
index bee30a77cd70..59515905d4ad 100644
--- a/arch/xtensa/mm/fault.c
+++ b/arch/xtensa/mm/fault.c
@@ -110,7 +110,7 @@ void do_page_fault(struct pt_regs *regs)
*/
fault = handle_mm_fault(vma, address, flags);
- if ((fault & VM_FAULT_RETRY) && fatal_signal_pending(current))
+ if (fault_signal_pending(fault, regs))
return;
if (unlikely(fault & VM_FAULT_ERROR)) {
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 88050259c466..4c87ffce64d1 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -369,6 +369,19 @@ static inline int signal_pending_state(long state, struct task_struct *p)
return (state & TASK_INTERRUPTIBLE) || __fatal_signal_pending(p);
}
+/*
+ * This should only be used in fault handlers to decide whether we
+ * should stop the current fault routine to handle the signals
+ * instead, especially with the case where we've got interrupted with
+ * a VM_FAULT_RETRY.
+ */
+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));
+}
+
/*
* Reevaluate whether the task has signals pending delivery.
* Wake the task if so.
--
2.24.1
^ permalink raw reply related
* [PATCH RESEND v6 00/16] mm: Page fault enhancements
From: Peter Xu @ 2020-02-20 15:53 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Andrea Arcangeli, Martin Cracauer, Linus Torvalds, Mike Rapoport,
Kirill A . Shutemov, Johannes Weiner, Dr . David Alan Gilbert,
David Hildenbrand, Bobby Powers, Maya Gokhale, Jerome Glisse,
Mike Kravetz, Matthew Wilcox, Marty McFadden, Mel Gorman, peterx,
Hugh Dickins, Brian Geffon, Denis Plotnikov, Pavel Emelyanov
[Resend v6]
This is v6 of the series. It is majorly a rebase to 5.6-rc2, nothing
else to be expected (plus some tests after the rebase). Instead of
rewrite the cover letter I decided to use what we have for v5.
Adding extra CCs for both Bobby Powers <bobbypowers@gmail.com> and
Brian Geffon <bgeffon@google.com>.
Online repo: https://github.com/xzpeter/linux/tree/mm-pf-signal-retry
Any review comment is appreciated. Thanks,
=============== v5 cover letter ==================
This is v5 of the series. As Matthew suggested, I split the previous
patch "mm: Return faster for non-fatal signals in user mode faults"
into a few smaller ones:
1. One patch to introduce fatal_signal_pending(), and use it in
archs that can directly apply
2. A few more patches to let the rest archs to use the new helper.
With that we can have an unified entry for signal detection
3. One last patch to change fatal_signal_pending() to detect
userspace non-fatal signal
Nothing should have changed in the rest patches. Because the fault
retry patches will depend on the previous ones, I decided to simply
repost all the patches.
Here's the new patchset layout:
Patch 1-2: cleanup, and potential bugfix of hugetlbfs on fault retry
Patch 3-9: let page fault to respond to non-fatal signals faster
Patch 10: remove the userfaultfd NOPAGE emulation
Patch 11-14: allow page fault to retry more than once
Patch 15-16: let gup code to use FAULT_FLAG_KILLABLE too
I would really appreciate any review comments for the series,
especially for the first two patches which IMHO are even not related
to this patchset and they should either cleanup or fix things.
Smoke tested on x86 only.
Thanks,
v5:
- split "mm: Return faster for non-fatal signals in user mode faults"
into a few more patches, let all archs to use an unified entry for
fast signal handling (fatal_signal_pending)
v4:
- use lore.kernel.org for all the links in commit messages [Kirill]
- one more patch ("mm/gup: Fix __get_user_pages() on fault retry of
hugetlb") to fix hugetlb path on fault retry
- one more patch ("mm/gup: Allow to react to fatal signals") to:
- use down_read_killable() properly [Linus]
- pass in FAULT_FLAG_KILLABLE for all GUP [Linus]
- one more patch ("mm/userfaultfd: Honor FAULT_FLAG_KILLABLE in fault
path") to let handle_userfaultfd() respect FAULT_FLAG_KILLABLE.
Should have no functional change after previous two new patches.
v3:
- check fatal signals in __get_user_page_locked() [Linus]
- add r-bs
v2:
- resent previous version, rebase only
=============== v1 cover letter ==================
This series is split out of userfaultfd-wp series to only cover the
general page fault changes, since it seems to make sense itself.
Basically it does two things:
(a) Allows the page fault handlers to be more interactive on not
only SIGKILL, but also the rest of userspace signals (especially
for user-mode faults), and,
(b) Allows the page fault retry (VM_FAULT_RETRY) to happen for more
than once.
I'm keeping the CC list as in uffd-wp v5, hopefully I'm not sending
too much spams...
And, instead of writting again the cover letter, I'm just copy-pasting
my previous link here which has more details on why we do this:
https://patchwork.kernel.org/cover/10691991/
The major change from that latest version should be that we introduced
a new page fault flag FAULT_FLAG_INTERRUPTIBLE as suggested by Linus
[1] to represents that we would like the fault handler to respond to
non-fatal signals. Also, we're more careful now on when to do the
immediate return of the page fault for such signals. For example, now
we'll only check against signal_pending() for user-mode page faults
and we keep the kernel-mode page fault patch untouched for it. More
information can be found in separate patches.
The patchset is only lightly tested on x86.
All comments are greatly welcomed. Thanks,
[1] https://lkml.org/lkml/2019/6/25/1382
Peter Xu (16):
mm/gup: Rename "nonblocking" to "locked" where proper
mm/gup: Fix __get_user_pages() on fault retry of hugetlb
mm: Introduce fault_signal_pending()
x86/mm: Use helper fault_signal_pending()
arc/mm: Use helper fault_signal_pending()
arm64/mm: Use helper fault_signal_pending()
powerpc/mm: Use helper fault_signal_pending()
sh/mm: Use helper fault_signal_pending()
mm: Return faster for non-fatal signals in user mode faults
userfaultfd: Don't retake mmap_sem to emulate NOPAGE
mm: Introduce FAULT_FLAG_DEFAULT
mm: Introduce FAULT_FLAG_INTERRUPTIBLE
mm: Allow VM_FAULT_RETRY for multiple times
mm/gup: Allow VM_FAULT_RETRY for multiple times
mm/gup: Allow to react to fatal signals
mm/userfaultfd: Honor FAULT_FLAG_KILLABLE in fault path
arch/alpha/mm/fault.c | 6 +--
arch/arc/mm/fault.c | 35 +++++--------
arch/arm/mm/fault.c | 7 +--
arch/arm64/mm/fault.c | 26 +++------
arch/hexagon/mm/vm_fault.c | 5 +-
arch/ia64/mm/fault.c | 5 +-
arch/m68k/mm/fault.c | 7 +--
arch/microblaze/mm/fault.c | 5 +-
arch/mips/mm/fault.c | 5 +-
arch/nds32/mm/fault.c | 5 +-
arch/nios2/mm/fault.c | 7 +--
arch/openrisc/mm/fault.c | 5 +-
arch/parisc/mm/fault.c | 8 ++-
arch/powerpc/mm/fault.c | 20 ++-----
arch/riscv/mm/fault.c | 9 +---
arch/s390/mm/fault.c | 10 ++--
arch/sh/mm/fault.c | 13 +++--
arch/sparc/mm/fault_32.c | 5 +-
arch/sparc/mm/fault_64.c | 5 +-
arch/um/kernel/trap.c | 3 +-
arch/unicore32/mm/fault.c | 8 ++-
arch/x86/mm/fault.c | 30 +++++------
arch/xtensa/mm/fault.c | 5 +-
drivers/gpu/drm/ttm/ttm_bo_vm.c | 12 +++--
fs/userfaultfd.c | 62 ++++++++++------------
include/linux/mm.h | 81 ++++++++++++++++++++++++----
include/linux/sched/signal.h | 14 +++++
mm/filemap.c | 2 +-
mm/gup.c | 93 +++++++++++++++++++++------------
mm/hugetlb.c | 17 +++---
mm/internal.h | 6 +--
31 files changed, 281 insertions(+), 240 deletions(-)
--
2.24.1
^ permalink raw reply
* [PATCH RESEND v6 04/16] x86/mm: Use helper fault_signal_pending()
From: Peter Xu @ 2020-02-20 15:53 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Andrea Arcangeli, Martin Cracauer, Linus Torvalds, Mike Rapoport,
Kirill A . Shutemov, Johannes Weiner, Dr . David Alan Gilbert,
David Hildenbrand, Bobby Powers, Maya Gokhale, Jerome Glisse,
Mike Kravetz, Matthew Wilcox, Marty McFadden, Mel Gorman, peterx,
Hugh Dickins, Brian Geffon, Denis Plotnikov, Pavel Emelyanov
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
Let's move the fatal signal check even earlier so that we can directly
use the new fault_signal_pending() in x86 mm code.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/x86/mm/fault.c | 28 +++++++++++++---------------
1 file changed, 13 insertions(+), 15 deletions(-)
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index fa4ea09593ab..6a00bc8d047f 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1442,27 +1442,25 @@ void do_user_addr_fault(struct pt_regs *regs,
fault = handle_mm_fault(vma, address, flags);
major |= fault & VM_FAULT_MAJOR;
+ /* Quick path to respond to signals */
+ if (fault_signal_pending(fault, regs)) {
+ if (!user_mode(regs))
+ no_context(regs, hw_error_code, address, SIGBUS,
+ BUS_ADRERR);
+ return;
+ }
+
/*
* If we need to retry the mmap_sem has already been released,
* and if there is a fatal signal pending there is no guarantee
* that we made any progress. Handle this case first.
*/
- if (unlikely(fault & VM_FAULT_RETRY)) {
+ if (unlikely((fault & VM_FAULT_RETRY) &&
+ (flags & FAULT_FLAG_ALLOW_RETRY))) {
/* Retry at most once */
- if (flags & FAULT_FLAG_ALLOW_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
- flags |= FAULT_FLAG_TRIED;
- if (!fatal_signal_pending(tsk))
- goto retry;
- }
-
- /* User mode? Just return to handle the fatal exception */
- if (flags & FAULT_FLAG_USER)
- return;
-
- /* Not returning to user mode? Handle exceptions or die: */
- no_context(regs, hw_error_code, address, SIGBUS, BUS_ADRERR);
- return;
+ flags &= ~FAULT_FLAG_ALLOW_RETRY;
+ flags |= FAULT_FLAG_TRIED;
+ goto retry;
}
up_read(&mm->mmap_sem);
--
2.24.1
^ permalink raw reply related
* Re: omap-secure.c:undefined reference to `__arm_smccc_smc'
From: Tony Lindgren @ 2020-02-20 15:54 UTC (permalink / raw)
To: kbuild-all
In-Reply-To: <202002131856.VeW4PhBJ%lkp@intel.com>
[-- Attachment #1: Type: text/plain, Size: 1187 bytes --]
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.
Regards,
Tony
^ permalink raw reply
* Re: omap-secure.c:undefined reference to `__arm_smccc_smc'
From: Tony Lindgren @ 2020-02-20 15:54 UTC (permalink / raw)
To: Andrew F. Davis; +Cc: kbuild-all, linux-kernel, kbuild test robot
In-Reply-To: <202002131856.VeW4PhBJ%lkp@intel.com>
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.
Regards,
Tony
^ permalink raw reply
* [Cluster-devel] [PATCH v7 14/24] btrfs: Convert from readpages to readahead
From: Matthew Wilcox @ 2020-02-20 15:54 UTC (permalink / raw)
To: cluster-devel.redhat.com
In-Reply-To: <20200220154658.GA19577@infradead.org>
On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote:
> On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote:
> > btrfs: Convert from readpages to readahead
> >
> > Implement the new readahead method in btrfs. Add a readahead_page_batch()
> > to optimise fetching a batch of pages at once.
>
> Shouldn't this readahead_page_batch heper go into a separate patch so
> that it clearly stands out?
I'll move it into 'Put readahead pages in cache earlier' for v8 (the
same patch where we add readahead_page())
^ permalink raw reply
* [Ocfs2-devel] [PATCH v7 14/24] btrfs: Convert from readpages to readahead
From: Matthew Wilcox @ 2020-02-20 15:54 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Johannes Thumshirn, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
linux-btrfs@vger.kernel.org, linux-erofs@lists.ozlabs.org,
linux-ext4@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com,
ocfs2-devel@oss.oracle.com, linux-xfs@vger.kernel.org
In-Reply-To: <20200220154658.GA19577@infradead.org>
On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote:
> On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote:
> > btrfs: Convert from readpages to readahead
> >
> > Implement the new readahead method in btrfs. Add a readahead_page_batch()
> > to optimise fetching a batch of pages at once.
>
> Shouldn't this readahead_page_batch heper go into a separate patch so
> that it clearly stands out?
I'll move it into 'Put readahead pages in cache earlier' for v8 (the
same patch where we add readahead_page())
^ permalink raw reply
* Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
From: Matthew Wilcox @ 2020-02-20 15:54 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Johannes Thumshirn, linux-fsdevel@vger.kernel.org,
linux-mm@kvack.org, linux-kernel@vger.kernel.org,
linux-btrfs@vger.kernel.org, linux-erofs@lists.ozlabs.org,
linux-ext4@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com,
ocfs2-devel@oss.oracle.com, linux-xfs@vger.kernel.org
In-Reply-To: <20200220154658.GA19577@infradead.org>
On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote:
> On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote:
> > btrfs: Convert from readpages to readahead
> >
> > Implement the new readahead method in btrfs. Add a readahead_page_batch()
> > to optimise fetching a batch of pages at once.
>
> Shouldn't this readahead_page_batch heper go into a separate patch so
> that it clearly stands out?
I'll move it into 'Put readahead pages in cache earlier' for v8 (the
same patch where we add readahead_page())
^ permalink raw reply
* Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
From: Matthew Wilcox @ 2020-02-20 15:54 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-xfs@vger.kernel.org, Johannes Thumshirn,
linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com,
linux-mm@kvack.org, ocfs2-devel@oss.oracle.com,
linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
linux-erofs@lists.ozlabs.org, linux-btrfs@vger.kernel.org
In-Reply-To: <20200220154658.GA19577@infradead.org>
On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote:
> On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote:
> > btrfs: Convert from readpages to readahead
> >
> > Implement the new readahead method in btrfs. Add a readahead_page_batch()
> > to optimise fetching a batch of pages at once.
>
> Shouldn't this readahead_page_batch heper go into a separate patch so
> that it clearly stands out?
I'll move it into 'Put readahead pages in cache earlier' for v8 (the
same patch where we add readahead_page())
^ permalink raw reply
* Re: [PATCH 10/12] mm: x86: Invoke hypercall when page encryption status is changed
From: Brijesh Singh @ 2020-02-20 15:54 UTC (permalink / raw)
To: Andy Lutomirski, Steve Rutherford
Cc: brijesh.singh, Ashish Kalra, Paolo Bonzini, Thomas Gleixner,
Ingo Molnar, H. Peter Anvin, Radim Krčmář,
Joerg Roedel, Borislav Petkov, Tom Lendacky, David Rientjes, x86,
KVM list, LKML
In-Reply-To: <52450536-AF7B-4206-8F05-CF387A216031@amacapital.net>
On 2/19/20 8:12 PM, Andy Lutomirski wrote:
>
>
>> On Feb 19, 2020, at 5:58 PM, Steve Rutherford <srutherford@google.com> wrote:
>>
>> On Wed, Feb 12, 2020 at 5:18 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>>>
>>> From: Brijesh Singh <brijesh.singh@amd.com>
>>>
>>> Invoke a hypercall when a memory region is changed from encrypted ->
>>> decrypted and vice versa. Hypervisor need to know the page encryption
>>> status during the guest migration.
>>
>> One messy aspect, which I think is fine in practice, is that this
>> presumes that pages are either treated as encrypted or decrypted. If
>> also done on SEV, the in-place re-encryption supported by SME would
>> break SEV migration. Linux doesn't do this now on SEV, and I don't
>> have an intuition for why Linux might want this, but we will need to
>> ensure it is never done in order to ensure that migration works down
>> the line. I don't believe the AMD manual promises this will work
>> anyway.
>>
>> Something feels a bit wasteful about having all future kernels
>> universally announce c-bit status when SEV is enabled, even if KVM
>> isn't listening, since it may be too old (or just not want to know).
>> Might be worth eliding the hypercalls if you get ENOSYS back? There
>> might be a better way of passing paravirt config metadata across than
>> just trying and seeing if the hypercall succeeds, but I'm not super
>> familiar with it.
>
> I actually think this should be a hard requirement to merge this. The host needs to tell the guest that it supports this particular migration strategy and the guest needs to tell the host that it is using it. And the guest needs a way to tell the host that it’s *not* using it right now due to kexec, for example.
>
> I’m still uneasy about a guest being migrated in the window where the hypercall tracking and the page encryption bit don’t match. I guess maybe corruption in this window doesn’t matter?
>
I don't think there is a corruption issue here. Let's consider the below
case:
1) A page is transmitted as C=1 (encrypted)
2) During the migration window, the page encryption bit is changed
to C=0 (decrypted)
3) #2 will cause a change in page table memory, thus dirty memory
the tracker will create retransmission of the page table memory.
4) The page itself will not be re-transmitted because there was
no change to the content of the page.
On destination, the read from the page will get the ciphertext.
The encryption bit change in the page table is used on the next access.
The user of the page needs to ensure that data is written with the
correct encryption bit before reading.
thanks
^ permalink raw reply
* Re: [RFC PATCH v3 05/27] qcow2: Document the Extended L2 Entries feature
From: Max Reitz @ 2020-02-20 15:54 UTC (permalink / raw)
To: Alberto Garcia, qemu-devel
Cc: Kevin Wolf, Anton Nefedov, qemu-block,
Vladimir Sementsov-Ogievskiy, Denis V . Lunev
In-Reply-To: <0b884ddcd0ac3a3c0b8cdd9d09c74566ac107c9a.1577014346.git.berto@igalia.com>
[-- Attachment #1.1: Type: text/plain, Size: 3344 bytes --]
On 22.12.19 12:36, Alberto Garcia wrote:
> Subcluster allocation in qcow2 is implemented by extending the
> existing L2 table entries and adding additional information to
> indicate the allocation status of each subcluster.
>
> This patch documents the changes to the qcow2 format and how they
> affect the calculation of the L2 cache size.
>
> Signed-off-by: Alberto Garcia <berto@igalia.com>
> ---
> docs/interop/qcow2.txt | 68 ++++++++++++++++++++++++++++++++++++++++--
> docs/qcow2-cache.txt | 19 +++++++++++-
> 2 files changed, 83 insertions(+), 4 deletions(-)
>
> diff --git a/docs/interop/qcow2.txt b/docs/interop/qcow2.txt
> index af5711e533..d34261f955 100644
> --- a/docs/interop/qcow2.txt
> +++ b/docs/interop/qcow2.txt
> @@ -39,6 +39,9 @@ The first cluster of a qcow2 image contains the file header:
> as the maximum cluster size and won't be able to open images
> with larger cluster sizes.
>
> + Note: if the image has Extended L2 Entries then cluster_bits
> + must be at least 14 (i.e. 16384 byte clusters).
> +
> 24 - 31: size
> Virtual disk size in bytes.
>
> @@ -109,7 +112,12 @@ in the description of a field.
> An External Data File Name header extension may
> be present if this bit is set.
>
> - Bits 3-63: Reserved (set to 0)
> + Bit 3: Extended L2 Entries. If this bit is set then
I suppose bit 4 now. (Compression is bit 3.)
[...]
> +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).
I don’t see ease of debugging as a particularly good reason; but then
again, I didn’t have to debug this feature yet (as opposed to you).
But since I’m used to counting bits from the right (because this is how
it’s done basically everywhere), I can’t imagine I would find it more
difficult than counting them from the left.
Max
> + 32 - 63 Subcluster reads as zeros (one bit per subcluster)
> +
> + 1: the subcluster reads as zeros. In this case the
> + allocation status bit must be unset. The host
> + cluster offset field may or may not be set.
> + 0: no effect.
> +
> + Bits are assigned starting from the most significant one.
> + (i.e. bit x is used for subcluster 63 - x)
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/6] driver core: Fix driver_deferred_probe_check_state() logic
From: Bjorn Andersson @ 2020-02-20 15:55 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-2-john.stultz@linaro.org>
On Wed 19 Feb 21:04 PST 2020, John Stultz wrote:
> driver_deferred_probe_check_state() has some uninituitive behavior.
>
> * From boot to late_initcall, it returns -EPROBE_DEFER
>
> * From late_initcall to the deferred_probe_timeout (if set)
> it returns -ENODEV
>
> * If the deferred_probe_timeout it set, after it fires, it
> returns -ETIMEDOUT
>
> This is a bit confusing, as its useful to have the function
> return -EPROBE_DEFER while the timeout is still running. This
> behavior has resulted in the somwhat duplicative
> driver_deferred_probe_check_state_continue() function being
> added.
>
> Thus this patch tries to improve the logic, so that it behaves
> as such:
>
> * If deferred_probe_timeout is set, it returns -EPROBE_DEFER
> until the timeout, afterwhich it returns -ETIMEDOUT.
>
> * If deferred_probe_timeout is not set (-1), it returns
> -EPROBE_DEFER until late_initcall, after which it returns
>
> This will make the deferred_probe_timeout value much more
> functional, and will allow us to consolidate the
> driver_deferred_probe_check_state() and
> driver_deferred_probe_check_state_continue() logic in a later
> patch.
>
> 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: I8349b7a403ce8cbce485ea0a0a5512fddffb635c
Please drop the Change-Id.
> ---
> v4:
> * Simplified logic suggested by Andy Shevchenko
> * Clarified commit message to focus on logic change
> ---
> drivers/base/dd.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index b25bcab2a26b..bb383dca39c1 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -237,7 +237,7 @@ __setup("deferred_probe_timeout=", deferred_probe_timeout_setup);
>
> static int __driver_deferred_probe_check_state(struct device *dev)
> {
> - if (!initcalls_done)
> + if (!initcalls_done || deferred_probe_timeout > 0)
> return -EPROBE_DEFER;
>
> if (!deferred_probe_timeout) {
> @@ -252,9 +252,11 @@ static int __driver_deferred_probe_check_state(struct device *dev)
> * driver_deferred_probe_check_state() - Check deferred probe state
> * @dev: device to check
> *
> - * Returns -ENODEV if init is done and all built-in drivers have had a chance
> - * to probe (i.e. initcalls are done), -ETIMEDOUT if deferred probe debug
> - * timeout has expired, or -EPROBE_DEFER if none of those conditions are met.
> + * Returnes -EPROBE_DEFER if initcalls have not completed, or the deferred
As pointed out by Rafael, this should be Return:
With that addressed, you have my
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> + * probe timeout is set, but not expried.
> + * Returns -ETIMEDOUT if the probe timeout was set and has expired.
> + * Returns -ENODEV if initcalls have completed and the deferred probe timeout
> + * was not set.
> *
> * Drivers or subsystems can opt-in to calling this function instead of directly
> * returning -EPROBE_DEFER.
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v3 0/3] Introduce per-task latency_nice for scheduler hints
From: chris hyser @ 2020-02-20 15:55 UTC (permalink / raw)
To: David Laight, Parth Shah, vincent.guittot@linaro.org,
patrick.bellasi@matbug.net, valentin.schneider@arm.com,
dhaval.giani@oracle.com, dietmar.eggemann@arm.com
Cc: linux-kernel@vger.kernel.org, peterz@infradead.org,
mingo@redhat.com, qais.yousef@arm.com, pavel@ucw.cz,
qperret@qperret.net, pjt@google.com, tj@kernel.org
In-Reply-To: <2870e44f41414fd58b58f7831d7386fe@AcuMS.aculab.com>
On 2/20/20 9:39 AM, David Laight wrote:
> From: chris hyser <chris.hyser@oracle.com>
>> Sent: 19 February 2020 17:17
>>
>> On 2/19/20 6:18 AM, David Laight wrote:
>>> From: chris hyser
>>>> Sent: 18 February 2020 23:00
>>> ...
>>>> All, I was asked to take a look at the original latency_nice patchset.
>>>> First, to clarify objectives, Oracle is not
>>>> interested in trading throughput for latency.
>>>> What we found is that the DB has specific tasks which do very little but
>>>> need to do this as absolutely quickly as possible, ie extreme latency
>>>> sensitivity. Second, the key to latency reduction
>>>> in the task wakeup path seems to be limiting variations of "idle cpu" search.
>>>> The latter particularly interests me as an example of "platform size
>>>> based latency" which I believe to be important given all the varying size
>>>> VMs and containers.
>>>
>>> From my experiments there are a few things that seem to affect latency
>>> of waking up real time (sched fifo) tasks on a normal kernel:
>>
>> Sorry. I was only ever talking about sched_other as per the original patchset. I realize the term
>> extreme latency
>> sensitivity may have caused confusion. What that means to DB people is no doubt different than audio
>> people. :-)
>
> Shorter lines.....
>
> ISTM you are making some already complicated code even more complex.
> Better to make it simpler instead.
The code already exists to set a limit to bail out of what is sometimes a needlessly excessive search. Setting that
based on an integer doesn't seem particularly complex. Now whether that is actually useful is what I'm currently looking at.
>
> If you need a thread to run as soon as possible after it is woken
> why not use the RT scheduler (eg SCHED_FIFO) that is what it is for.
Overkill and doesn't play well with cpu cgroup controller.
>
> If there are delays finding an idle cpu to migrate a process to
> (especially on systems with large numbers of cpu) then that is a
> general problem that can be addressed without extra knobs.
There is no if. It is a brute force search. There are delays proportional to the search domain size. You can optimize
the hell of out the brute force, or you use obtained knowledge to bail out early. Getting that knowledge from the user
is a time honored tradition. :-)
-chrish
^ permalink raw reply
* Re: [PATCH] mm: memcontrol: asynchronous reclaim for memory.high
From: Tejun Heo @ 2020-02-20 15:56 UTC (permalink / raw)
To: Daniel Jordan
Cc: Johannes Weiner, Michal Hocko, Andrew Morton, Roman Gushchin,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, cgroups-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, kernel-team-b10kYP2dOMg,
Peter Zijlstra
In-Reply-To: <20200220154524.dql3i5brnjjwecft-S51bK0XF4qpuJJETbFA3a0B3C2bhBk7L0E9HWUfgJXw@public.gmane.org>
On Thu, Feb 20, 2020 at 10:45:24AM -0500, Daniel Jordan wrote:
> Ok, consistency with io and memory is one advantage to doing it that way.
> Creating kthreads in cgroups also seems viable so far, and it's unclear whether
> either approach is significantly simpler or more maintainable than the other,
> at least to me.
The problem with separate kthread approach is that many of these work
units are tiny, and cgroup membership might not be known or doesn't
agree with the processing context from the beginning
For example, the ownership of network packets can't be determined till
processing has progressed quite a bit in shared contexts and each item
too small to bounce around. The only viable way I can think of
splitting aggregate overhead according to the number of packets (or
some other trivially measureable quntity) processed.
Anything sitting in reclaim layer is the same. Reclaim should be
charged to the cgroup whose memory is reclaimed *but* shouldn't block
other cgroups which are waiting for that memory. It has to happen in
the context of the highest priority entity waiting for memory but the
costs incurred must be charged to the memory owners.
So, one way or the other, I think we'll need back charging and once
back charging is needed for big ticket items like network and reclaim,
it's kinda silly to use separate mechanisms for other stuff.
> Is someone on your side working on remote charging right now? I was planning
> to post an RFD comparing these soon and it would make sense to include them.
It's been on the to do list but nobody is working on it yet.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH] mm: memcontrol: asynchronous reclaim for memory.high
From: Tejun Heo @ 2020-02-20 15:56 UTC (permalink / raw)
To: Daniel Jordan
Cc: Johannes Weiner, Michal Hocko, Andrew Morton, Roman Gushchin,
linux-mm, cgroups, linux-kernel, kernel-team, Peter Zijlstra
In-Reply-To: <20200220154524.dql3i5brnjjwecft@ca-dmjordan1.us.oracle.com>
On Thu, Feb 20, 2020 at 10:45:24AM -0500, Daniel Jordan wrote:
> Ok, consistency with io and memory is one advantage to doing it that way.
> Creating kthreads in cgroups also seems viable so far, and it's unclear whether
> either approach is significantly simpler or more maintainable than the other,
> at least to me.
The problem with separate kthread approach is that many of these work
units are tiny, and cgroup membership might not be known or doesn't
agree with the processing context from the beginning
For example, the ownership of network packets can't be determined till
processing has progressed quite a bit in shared contexts and each item
too small to bounce around. The only viable way I can think of
splitting aggregate overhead according to the number of packets (or
some other trivially measureable quntity) processed.
Anything sitting in reclaim layer is the same. Reclaim should be
charged to the cgroup whose memory is reclaimed *but* shouldn't block
other cgroups which are waiting for that memory. It has to happen in
the context of the highest priority entity waiting for memory but the
costs incurred must be charged to the memory owners.
So, one way or the other, I think we'll need back charging and once
back charging is needed for big ticket items like network and reclaim,
it's kinda silly to use separate mechanisms for other stuff.
> Is someone on your side working on remote charging right now? I was planning
> to post an RFD comparing these soon and it would make sense to include them.
It's been on the to do list but nobody is working on it yet.
Thanks.
--
tejun
^ permalink raw reply
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.