* Re: [PATCH v2 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Peter Oskolkov @ 2019-05-24 22:07 UTC (permalink / raw)
To: Dave Chiluk
Cc: Phil Auld, Peter Zijlstra, Ingo Molnar, cgroups,
Linux Kernel Mailing List, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc, Ben Segall
In-Reply-To: <CAC=E7cXVrGRKMNkJPhd4fJi7wgdYk=YcXPV7B8GVNL5M69BarQ@mail.gmail.com>
On Fri, May 24, 2019 at 2:35 PM Dave Chiluk <chiluk+linux@indeed.com> wrote:
>
> On Fri, May 24, 2019 at 11:28 AM Peter Oskolkov <posk@posk.io> wrote:
> >
> > On Fri, May 24, 2019 at 8:15 AM Dave Chiluk <chiluk+linux@indeed.com> wrote:
> > >
> > > On Fri, May 24, 2019 at 9:32 AM Phil Auld <pauld@redhat.com> wrote:
> > > > On Thu, May 23, 2019 at 02:01:58PM -0700 Peter Oskolkov wrote:
> > >
> > > > > If the machine runs at/close to capacity, won't the overallocation
> > > > > of the quota to bursty tasks necessarily negatively impact every other
> > > > > task? Should the "unused" quota be available only on idle CPUs?
> > > > > (Or maybe this is the behavior achieved here, and only the comment and
> > > > > the commit message should be fixed...)
> > > > >
> > > >
> > > > It's bounded by the amount left unused from the previous period. So
> > > > theoretically a process could use almost twice its quota. But then it
> > > > would have nothing left over in the next period. To repeat it would have
> > > > to not use any that next period. Over a longer number of periods it's the
> > > > same amount of CPU usage.
> > > >
> > > > I think that is more fair than throttling a process that has never used
> > > > its full quota.
> > > >
> > > > And it removes complexity.
> > > >
> > > > Cheers,
> > > > Phil
> > >
> > > Actually it's not even that bad. The overallocation of quota to a
> > > bursty task in a period is limited to at most one slice per cpu, and
> > > that slice must not have been used in the previous periods. The slice
> > > size is set with /proc/sys/kernel/sched_cfs_bandwidth_slice_us and
> > > defaults to 5ms. If a bursty task goes from underutilizing quota to
> > > using it's entire quota, it will not be able to burst in the
> > > subsequent periods. Therefore in an absolute worst case contrived
> > > scenario, a bursty task can add at most 5ms to the latency of other
> > > threads on the same CPU. I think this worst case 5ms tradeoff is
> > > entirely worth it.
> > >
> > > This does mean that a theoretically a poorly written massively
> > > threaded application on an 80 core box, that spreads itself onto 80
> > > cpu run queues, can overutilize it's quota in a period by at most 5ms
> > > * 80 CPUs in a sincle period (slice * number of runqueues the
> > > application is running on). But that means that each of those threads
> > > would have had to not be use their quota in a previous period, and it
> > > also means that the application would have to be carefully written to
> > > exacerbate this behavior.
> > >
> > > Additionally if cpu bound threads underutilize a slice of their quota
> > > in a period due to the cfs choosing a bursty task to run, they should
> > > theoretically be able to make it up in the following periods when the
> > > bursty task is unable to "burst".
> >
> > OK, so it is indeed possible that CPU bound threads will underutilize a slice
> > of their quota in a period as a result of this patch. This should probably
> > be clearly stated in the code comments and in the commit message.
> >
> > In addition, I believe that although many workloads will indeed be
> > indifferent to getting their fair share "later", some latency-sensitive
> > workloads will definitely be negatively affected by this temporary
> > CPU quota stealing by bursty antagonists. So there should probably be
> > a way to limit this behavior; for example, by making it tunable
> > per cgroup.
> >
> This patch restores the behavior that existed from at least
> v3.16..v4.18, and the current Redhat 7 kernels. So you are kind of
> championing a moot point as no one has noticed this "bursting"
> behavior in over 5 years. By removing this slice expiration
> altogether we restore the behavior and also solve the root issue of
> 'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
> condition")'.
>
> Since 512ac999d275, many people are now noticing the slice expiration
> and very displeased with the behavior change.
> see: https://github.com/kubernetes/kubernetes/issues/67577
>
> I would love to hear if you know of a single complaint during that 5
> year time window, where someone noticed this bursting and reported
> that it negatively affected their application.
Linux CPU scheduling tail latency is a well-known issue and a major
pain point in some workloads:
https://www.google.com/search?q=linux+cpu+scheduling+tail+latency
Even assuming that nobody noticed this particular cause
of CPU scheduling latencies, it does not mean the problem should be waved
away. At least it should be documented, if at this point it decided that
it is difficult to address it in a meaningful way. And, preferably, a way
to address the issue later on should be discussed and hopefully agreed to.
>
> As for the documentation, I thought about documenting the possible
> adverse side-effect, but I didn't feel it was worthwhile since no one
> had noticed that corner case in the 5 years. I also could not figure
> out a concise way of describing the slight corner case issue without
> overly complicating the documentation. I felt that adding that corner
> case would have detracted rather than added to the documentation's
> usefulness. As far as commenting in code, considering most of this
> commit removes lines, there's not a really great place for it. That's
> why I did my best to describe the behavior in the documentation.
> Smart people can draw conclusions from there as you have done. Please
> keep in mind that this "bursting" is again limited to a single slice
> on each per-cpu run-queue.
>
> Thank you,
> Dave
^ permalink raw reply
* Re: [PATCH v2 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Dave Chiluk @ 2019-05-24 21:35 UTC (permalink / raw)
To: Peter Oskolkov
Cc: Phil Auld, Peter Zijlstra, Ingo Molnar, cgroups,
Linux Kernel Mailing List, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc, Ben Segall
In-Reply-To: <CAFTs51Vm258CkDXi_Jj_cGOMotTvhdYR_VW8aUwAUvgistZOFQ@mail.gmail.com>
On Fri, May 24, 2019 at 11:28 AM Peter Oskolkov <posk@posk.io> wrote:
>
> On Fri, May 24, 2019 at 8:15 AM Dave Chiluk <chiluk+linux@indeed.com> wrote:
> >
> > On Fri, May 24, 2019 at 9:32 AM Phil Auld <pauld@redhat.com> wrote:
> > > On Thu, May 23, 2019 at 02:01:58PM -0700 Peter Oskolkov wrote:
> >
> > > > If the machine runs at/close to capacity, won't the overallocation
> > > > of the quota to bursty tasks necessarily negatively impact every other
> > > > task? Should the "unused" quota be available only on idle CPUs?
> > > > (Or maybe this is the behavior achieved here, and only the comment and
> > > > the commit message should be fixed...)
> > > >
> > >
> > > It's bounded by the amount left unused from the previous period. So
> > > theoretically a process could use almost twice its quota. But then it
> > > would have nothing left over in the next period. To repeat it would have
> > > to not use any that next period. Over a longer number of periods it's the
> > > same amount of CPU usage.
> > >
> > > I think that is more fair than throttling a process that has never used
> > > its full quota.
> > >
> > > And it removes complexity.
> > >
> > > Cheers,
> > > Phil
> >
> > Actually it's not even that bad. The overallocation of quota to a
> > bursty task in a period is limited to at most one slice per cpu, and
> > that slice must not have been used in the previous periods. The slice
> > size is set with /proc/sys/kernel/sched_cfs_bandwidth_slice_us and
> > defaults to 5ms. If a bursty task goes from underutilizing quota to
> > using it's entire quota, it will not be able to burst in the
> > subsequent periods. Therefore in an absolute worst case contrived
> > scenario, a bursty task can add at most 5ms to the latency of other
> > threads on the same CPU. I think this worst case 5ms tradeoff is
> > entirely worth it.
> >
> > This does mean that a theoretically a poorly written massively
> > threaded application on an 80 core box, that spreads itself onto 80
> > cpu run queues, can overutilize it's quota in a period by at most 5ms
> > * 80 CPUs in a sincle period (slice * number of runqueues the
> > application is running on). But that means that each of those threads
> > would have had to not be use their quota in a previous period, and it
> > also means that the application would have to be carefully written to
> > exacerbate this behavior.
> >
> > Additionally if cpu bound threads underutilize a slice of their quota
> > in a period due to the cfs choosing a bursty task to run, they should
> > theoretically be able to make it up in the following periods when the
> > bursty task is unable to "burst".
>
> OK, so it is indeed possible that CPU bound threads will underutilize a slice
> of their quota in a period as a result of this patch. This should probably
> be clearly stated in the code comments and in the commit message.
>
> In addition, I believe that although many workloads will indeed be
> indifferent to getting their fair share "later", some latency-sensitive
> workloads will definitely be negatively affected by this temporary
> CPU quota stealing by bursty antagonists. So there should probably be
> a way to limit this behavior; for example, by making it tunable
> per cgroup.
>
This patch restores the behavior that existed from at least
v3.16..v4.18, and the current Redhat 7 kernels. So you are kind of
championing a moot point as no one has noticed this "bursting"
behavior in over 5 years. By removing this slice expiration
altogether we restore the behavior and also solve the root issue of
'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
condition")'.
Since 512ac999d275, many people are now noticing the slice expiration
and very displeased with the behavior change.
see: https://github.com/kubernetes/kubernetes/issues/67577
I would love to hear if you know of a single complaint during that 5
year time window, where someone noticed this bursting and reported
that it negatively affected their application.
As for the documentation, I thought about documenting the possible
adverse side-effect, but I didn't feel it was worthwhile since no one
had noticed that corner case in the 5 years. I also could not figure
out a concise way of describing the slight corner case issue without
overly complicating the documentation. I felt that adding that corner
case would have detracted rather than added to the documentation's
usefulness. As far as commenting in code, considering most of this
commit removes lines, there's not a really great place for it. That's
why I did my best to describe the behavior in the documentation.
Smart people can draw conclusions from there as you have done. Please
keep in mind that this "bursting" is again limited to a single slice
on each per-cpu run-queue.
Thank you,
Dave
^ permalink raw reply
* Re: [PATCH v12 03/13] dt-bindings: Add doc for the Ingenic TCU drivers
From: Rob Herring @ 2019-05-24 20:21 UTC (permalink / raw)
To: Paul Cercueil
Cc: Mark Rutland, Ralf Baechle, Paul Burton, James Hogan,
Jonathan Corbet, Daniel Lezcano, Thomas Gleixner,
Michael Turquette, Stephen Boyd, Jason Cooper, Marc Zyngier,
Lee Jones, Mathieu Malaterre, linux-kernel, devicetree,
linux-mips, linux-doc, linux-clk, od
In-Reply-To: <20190521145141.9813-4-paul@crapouillou.net>
On Tue, May 21, 2019 at 04:51:31PM +0200, Paul Cercueil wrote:
> Add documentation about how to properly use the Ingenic TCU
> (Timer/Counter Unit) drivers from devicetree.
>
> Signed-off-by: Paul Cercueil <paul@crapouillou.net>
> ---
>
> Notes:
> v4: New patch in this series. Corresponds to V2 patches 3-4-5 with
> added content.
>
> v5: - Edited PWM/watchdog DT bindings documentation to point to the new
> document.
> - Moved main document to
> Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> - Updated documentation to reflect the new devicetree bindings.
>
> v6: - Removed PWM/watchdog documentation files as asked by upstream
> - Removed doc about properties that should be implicit
> - Removed doc about ingenic,timer-channel /
> ingenic,clocksource-channel as they are gone
> - Fix WDT clock name in the binding doc
> - Fix lengths of register areas in watchdog/pwm nodes
>
> v7: No change
>
> v8: - Fix address of the PWM node
> - Added doc about system timer and clocksource children nodes
>
> v9: - Remove doc about system timer and clocksource children
> nodes...
> - Add doc about ingenic,pwm-channels-mask property
>
> v10: No change
>
> v11: Fix info about default value of ingenic,pwm-channels-mask
>
> v12: Drop sub-nodes for now; they will be introduced in a follow-up
> patchset.
Why? I believe I acked them.
>
> .../devicetree/bindings/timer/ingenic,tcu.txt | 59 +++++++++++++++++++
> 1 file changed, 59 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/timer/ingenic,tcu.txt
>
> diff --git a/Documentation/devicetree/bindings/timer/ingenic,tcu.txt b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> new file mode 100644
> index 000000000000..d101cd72c9b0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/ingenic,tcu.txt
> @@ -0,0 +1,59 @@
> +Ingenic JZ47xx SoCs Timer/Counter Unit devicetree bindings
> +==========================================================
> +
> +For a description of the TCU hardware and drivers, have a look at
> +Documentation/mips/ingenic-tcu.txt.
> +
> +Required properties:
> +
> +- compatible: Must be one of:
> + * "ingenic,jz4740-tcu"
> + * "ingenic,jz4725b-tcu"
> + * "ingenic,jz4770-tcu"
> +- reg: Should be the offset/length value corresponding to the TCU registers
> +- clocks: List of phandle & clock specifiers for clocks external to the TCU.
> + The "pclk", "rtc" and "ext" clocks should be provided. The "tcu" clock
> + should be provided if the SoC has it.
> +- clock-names: List of name strings for the external clocks.
> +- #clock-cells: Should be <1>;
> + Clock consumers specify this argument to identify a clock. The valid values
> + may be found in <dt-bindings/clock/ingenic,tcu.h>.
> +- interrupt-controller : Identifies the node as an interrupt controller
> +- #interrupt-cells : Specifies the number of cells needed to encode an
> + interrupt source. The value should be 1.
> +- interrupt-parent : phandle of the interrupt controller.
Drop this 'interrupt-parent' is implied and could be in a parent node.
> +- interrupts : Specifies the interrupt the controller is connected to.
> +
> +Optional properties:
> +
> +- ingenic,pwm-channels-mask: Bitmask of TCU channels reserved for PWM use.
> + Default value is 0xfc.
> +
> +
> +Example
> +==========================================================
> +
> +#include <dt-bindings/clock/jz4770-cgu.h>
> +
> +/ {
> + tcu: timer@10002000 {
> + compatible = "ingenic,jz4770-tcu";
> + reg = <0x10002000 0x1000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges = <0x0 0x10002000 0x1000>;
> +
> + #clock-cells = <1>;
> +
> + clocks = <&cgu JZ4770_CLK_RTC
> + &cgu JZ4770_CLK_EXT
> + &cgu JZ4770_CLK_PCLK>;
> + clock-names = "rtc", "ext", "pclk";
> +
> + interrupt-controller;
> + #interrupt-cells = <1>;
> +
> + interrupt-parent = <&intc>;
> + interrupts = <27 26 25>;
> + };
> +};
> --
> 2.21.0.593.g511ec345e18
>
^ permalink raw reply
* Re: [PATCH v2 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Peter Oskolkov @ 2019-05-24 16:28 UTC (permalink / raw)
To: Dave Chiluk
Cc: Phil Auld, Peter Zijlstra, Ingo Molnar, cgroups,
Linux Kernel Mailing List, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc, Ben Segall
In-Reply-To: <CAC=E7cXxsyMLw1PR+8QchTH8FYL7WX6_8LBVdqueR1yjW+VVkQ@mail.gmail.com>
On Fri, May 24, 2019 at 8:15 AM Dave Chiluk <chiluk+linux@indeed.com> wrote:
>
> On Fri, May 24, 2019 at 9:32 AM Phil Auld <pauld@redhat.com> wrote:
> > On Thu, May 23, 2019 at 02:01:58PM -0700 Peter Oskolkov wrote:
>
> > > If the machine runs at/close to capacity, won't the overallocation
> > > of the quota to bursty tasks necessarily negatively impact every other
> > > task? Should the "unused" quota be available only on idle CPUs?
> > > (Or maybe this is the behavior achieved here, and only the comment and
> > > the commit message should be fixed...)
> > >
> >
> > It's bounded by the amount left unused from the previous period. So
> > theoretically a process could use almost twice its quota. But then it
> > would have nothing left over in the next period. To repeat it would have
> > to not use any that next period. Over a longer number of periods it's the
> > same amount of CPU usage.
> >
> > I think that is more fair than throttling a process that has never used
> > its full quota.
> >
> > And it removes complexity.
> >
> > Cheers,
> > Phil
>
> Actually it's not even that bad. The overallocation of quota to a
> bursty task in a period is limited to at most one slice per cpu, and
> that slice must not have been used in the previous periods. The slice
> size is set with /proc/sys/kernel/sched_cfs_bandwidth_slice_us and
> defaults to 5ms. If a bursty task goes from underutilizing quota to
> using it's entire quota, it will not be able to burst in the
> subsequent periods. Therefore in an absolute worst case contrived
> scenario, a bursty task can add at most 5ms to the latency of other
> threads on the same CPU. I think this worst case 5ms tradeoff is
> entirely worth it.
>
> This does mean that a theoretically a poorly written massively
> threaded application on an 80 core box, that spreads itself onto 80
> cpu run queues, can overutilize it's quota in a period by at most 5ms
> * 80 CPUs in a sincle period (slice * number of runqueues the
> application is running on). But that means that each of those threads
> would have had to not be use their quota in a previous period, and it
> also means that the application would have to be carefully written to
> exacerbate this behavior.
>
> Additionally if cpu bound threads underutilize a slice of their quota
> in a period due to the cfs choosing a bursty task to run, they should
> theoretically be able to make it up in the following periods when the
> bursty task is unable to "burst".
OK, so it is indeed possible that CPU bound threads will underutilize a slice
of their quota in a period as a result of this patch. This should probably
be clearly stated in the code comments and in the commit message.
In addition, I believe that although many workloads will indeed be
indifferent to getting their fair share "later", some latency-sensitive
workloads will definitely be negatively affected by this temporary
CPU quota stealing by bursty antagonists. So there should probably be
a way to limit this behavior; for example, by making it tunable
per cgroup.
>
> Please be careful here quota and slice are being treated differently.
> Quota does not roll-over between periods, only slices of quota that
> has already been allocated to per cpu run queues. If you allocate
> 100ms of quota per period to an application, but it only spreads onto
> 3 cpu run queues that means it can in the worst case use 3 x slice
> size = 15ms in periods following underutilization.
>
> So why does this matter. Well applications that use thread pools
> *(*cough* java *cough*) with lots of tiny little worker threads, tend
> to spread themselves out onto a lot of run queues. These worker
> threads grab quota slices in order to run, then rarely use all of
> their slice (1 or 2ms out of the 5ms). This results in those worker
> threads starving the main application of quota, and then expiring the
> remainder of that quota slice on the per-cpu. Going back to my
> earlier 100ms quota / 80 cpu example. That means only
> 100ms/cfs_bandwidth_slice_us(5ms) = 20 slices are available in a
> period. So only 20 out of these 80 cpus ever get a slice allocated to
> them. By allowing these per-cpu run queues to use their remaining
> slice in following periods these worker threads do not need to be
> allocated additional slice, and thereby the main threads are actually
> able to use the allocated cpu quota.
>
> This can be experienced by running fibtest available at
> https://github.com/indeedeng/fibtest/.
> $ runfibtest 1
> runs a single fast thread taskset to cpu 0
> $ runfibtest 8
> Runs a single fast thread taskset to cpu 0, and 7 slow threads taskset
> to cpus 1-7. This run is expected to show less iterations, but the
> worse problem is that the cpu usage is far less than the 500ms that it
> should have received.
>
> Thanks for the engagement on this,
> Dave Chiluk
^ permalink raw reply
* Re: [PATCH v2 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Phil Auld @ 2019-05-24 15:59 UTC (permalink / raw)
To: Dave Chiluk
Cc: Peter Oskolkov, Peter Zijlstra, Ingo Molnar, cgroups,
Linux Kernel Mailing List, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc, Ben Segall
In-Reply-To: <CAC=E7cXxsyMLw1PR+8QchTH8FYL7WX6_8LBVdqueR1yjW+VVkQ@mail.gmail.com>
On Fri, May 24, 2019 at 10:14:36AM -0500 Dave Chiluk wrote:
> On Fri, May 24, 2019 at 9:32 AM Phil Auld <pauld@redhat.com> wrote:
> > On Thu, May 23, 2019 at 02:01:58PM -0700 Peter Oskolkov wrote:
>
> > > If the machine runs at/close to capacity, won't the overallocation
> > > of the quota to bursty tasks necessarily negatively impact every other
> > > task? Should the "unused" quota be available only on idle CPUs?
> > > (Or maybe this is the behavior achieved here, and only the comment and
> > > the commit message should be fixed...)
> > >
> >
> > It's bounded by the amount left unused from the previous period. So
> > theoretically a process could use almost twice its quota. But then it
> > would have nothing left over in the next period. To repeat it would have
> > to not use any that next period. Over a longer number of periods it's the
> > same amount of CPU usage.
> >
> > I think that is more fair than throttling a process that has never used
> > its full quota.
> >
> > And it removes complexity.
> >
> > Cheers,
> > Phil
>
> Actually it's not even that bad. The overallocation of quota to a
> bursty task in a period is limited to at most one slice per cpu, and
> that slice must not have been used in the previous periods. The slice
> size is set with /proc/sys/kernel/sched_cfs_bandwidth_slice_us and
> defaults to 5ms. If a bursty task goes from underutilizing quota to
> using it's entire quota, it will not be able to burst in the
> subsequent periods. Therefore in an absolute worst case contrived
> scenario, a bursty task can add at most 5ms to the latency of other
> threads on the same CPU. I think this worst case 5ms tradeoff is
> entirely worth it.
>
> This does mean that a theoretically a poorly written massively
> threaded application on an 80 core box, that spreads itself onto 80
> cpu run queues, can overutilize it's quota in a period by at most 5ms
> * 80 CPUs in a sincle period (slice * number of runqueues the
> application is running on). But that means that each of those threads
> would have had to not be use their quota in a previous period, and it
> also means that the application would have to be carefully written to
> exacerbate this behavior.
>
> Additionally if cpu bound threads underutilize a slice of their quota
> in a period due to the cfs choosing a bursty task to run, they should
> theoretically be able to make it up in the following periods when the
> bursty task is unable to "burst".
>
> Please be careful here quota and slice are being treated differently.
> Quota does not roll-over between periods, only slices of quota that
> has already been allocated to per cpu run queues. If you allocate
> 100ms of quota per period to an application, but it only spreads onto
> 3 cpu run queues that means it can in the worst case use 3 x slice
> size = 15ms in periods following underutilization.
>
> So why does this matter. Well applications that use thread pools
> *(*cough* java *cough*) with lots of tiny little worker threads, tend
> to spread themselves out onto a lot of run queues. These worker
> threads grab quota slices in order to run, then rarely use all of
> their slice (1 or 2ms out of the 5ms). This results in those worker
> threads starving the main application of quota, and then expiring the
> remainder of that quota slice on the per-cpu. Going back to my
> earlier 100ms quota / 80 cpu example. That means only
> 100ms/cfs_bandwidth_slice_us(5ms) = 20 slices are available in a
> period. So only 20 out of these 80 cpus ever get a slice allocated to
> them. By allowing these per-cpu run queues to use their remaining
> slice in following periods these worker threads do not need to be
> allocated additional slice, and thereby the main threads are actually
> able to use the allocated cpu quota.
>
> This can be experienced by running fibtest available at
> https://github.com/indeedeng/fibtest/.
> $ runfibtest 1
> runs a single fast thread taskset to cpu 0
> $ runfibtest 8
> Runs a single fast thread taskset to cpu 0, and 7 slow threads taskset
> to cpus 1-7. This run is expected to show less iterations, but the
> worse problem is that the cpu usage is far less than the 500ms that it
> should have received.
>
> Thanks for the engagement on this,
> Dave Chiluk
Thanks for the clarification. This is an even better explanation.
Fwiw, I ran some of my cfs throttling tests with this, none of which are
designed to measure or hit this particular issue. They are more focused
on starvation and hard lockups that I've hit. But I did not see any issues
or oddities with this patch.
Cheers,
Phil
--
^ permalink raw reply
* Re: [PATCH v2 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Dave Chiluk @ 2019-05-24 15:14 UTC (permalink / raw)
To: Phil Auld
Cc: Peter Oskolkov, Peter Zijlstra, Ingo Molnar, cgroups,
Linux Kernel Mailing List, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc, Ben Segall
In-Reply-To: <20190524143204.GB4684@lorien.usersys.redhat.com>
On Fri, May 24, 2019 at 9:32 AM Phil Auld <pauld@redhat.com> wrote:
> On Thu, May 23, 2019 at 02:01:58PM -0700 Peter Oskolkov wrote:
> > If the machine runs at/close to capacity, won't the overallocation
> > of the quota to bursty tasks necessarily negatively impact every other
> > task? Should the "unused" quota be available only on idle CPUs?
> > (Or maybe this is the behavior achieved here, and only the comment and
> > the commit message should be fixed...)
> >
>
> It's bounded by the amount left unused from the previous period. So
> theoretically a process could use almost twice its quota. But then it
> would have nothing left over in the next period. To repeat it would have
> to not use any that next period. Over a longer number of periods it's the
> same amount of CPU usage.
>
> I think that is more fair than throttling a process that has never used
> its full quota.
>
> And it removes complexity.
>
> Cheers,
> Phil
Actually it's not even that bad. The overallocation of quota to a
bursty task in a period is limited to at most one slice per cpu, and
that slice must not have been used in the previous periods. The slice
size is set with /proc/sys/kernel/sched_cfs_bandwidth_slice_us and
defaults to 5ms. If a bursty task goes from underutilizing quota to
using it's entire quota, it will not be able to burst in the
subsequent periods. Therefore in an absolute worst case contrived
scenario, a bursty task can add at most 5ms to the latency of other
threads on the same CPU. I think this worst case 5ms tradeoff is
entirely worth it.
This does mean that a theoretically a poorly written massively
threaded application on an 80 core box, that spreads itself onto 80
cpu run queues, can overutilize it's quota in a period by at most 5ms
* 80 CPUs in a sincle period (slice * number of runqueues the
application is running on). But that means that each of those threads
would have had to not be use their quota in a previous period, and it
also means that the application would have to be carefully written to
exacerbate this behavior.
Additionally if cpu bound threads underutilize a slice of their quota
in a period due to the cfs choosing a bursty task to run, they should
theoretically be able to make it up in the following periods when the
bursty task is unable to "burst".
Please be careful here quota and slice are being treated differently.
Quota does not roll-over between periods, only slices of quota that
has already been allocated to per cpu run queues. If you allocate
100ms of quota per period to an application, but it only spreads onto
3 cpu run queues that means it can in the worst case use 3 x slice
size = 15ms in periods following underutilization.
So why does this matter. Well applications that use thread pools
*(*cough* java *cough*) with lots of tiny little worker threads, tend
to spread themselves out onto a lot of run queues. These worker
threads grab quota slices in order to run, then rarely use all of
their slice (1 or 2ms out of the 5ms). This results in those worker
threads starving the main application of quota, and then expiring the
remainder of that quota slice on the per-cpu. Going back to my
earlier 100ms quota / 80 cpu example. That means only
100ms/cfs_bandwidth_slice_us(5ms) = 20 slices are available in a
period. So only 20 out of these 80 cpus ever get a slice allocated to
them. By allowing these per-cpu run queues to use their remaining
slice in following periods these worker threads do not need to be
allocated additional slice, and thereby the main threads are actually
able to use the allocated cpu quota.
This can be experienced by running fibtest available at
https://github.com/indeedeng/fibtest/.
$ runfibtest 1
runs a single fast thread taskset to cpu 0
$ runfibtest 8
Runs a single fast thread taskset to cpu 0, and 7 slow threads taskset
to cpus 1-7. This run is expected to show less iterations, but the
worse problem is that the cpu usage is far less than the 500ms that it
should have received.
Thanks for the engagement on this,
Dave Chiluk
^ permalink raw reply
* Re: [PATCH v2 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Phil Auld @ 2019-05-24 14:32 UTC (permalink / raw)
To: Peter Oskolkov
Cc: Dave Chiluk, Peter Zijlstra, Ingo Molnar, cgroups,
Linux Kernel Mailing List, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc
In-Reply-To: <CAFTs51W0KdK4nw6wydn2HjNYvFRC8DYMmVeKX9FAe+4YUGEAZg@mail.gmail.com>
On Thu, May 23, 2019 at 02:01:58PM -0700 Peter Oskolkov wrote:
> On Thu, May 23, 2019 at 11:44 AM Dave Chiluk <chiluk+linux@indeed.com> wrote:
> >
> > It has been observed, that highly-threaded, non-cpu-bound applications
> > running under cpu.cfs_quota_us constraints can hit a high percentage of
> > periods throttled while simultaneously not consuming the allocated
> > amount of quota. This use case is typical of user-interactive non-cpu
> > bound applications, such as those running in kubernetes or mesos when
> > run on multiple cpu cores.
> >
> > This has been root caused to threads being allocated per cpu bandwidth
> > slices, and then not fully using that slice within the period. At which
> > point the slice and quota expires. This expiration of unused slice
> > results in applications not being able to utilize the quota for which
> > they are allocated.
> >
> > The expiration of per-cpu slices was recently fixed by
> > 'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
> > condition")'. Prior to that it appears that this has been broken since
> > at least 'commit 51f2176d74ac ("sched/fair: Fix unlocked reads of some
> > cfs_b->quota/period")' which was introduced in v3.16-rc1 in 2014. That
> > added the following conditional which resulted in slices never being
> > expired.
> >
> > if (cfs_rq->runtime_expires != cfs_b->runtime_expires) {
> > /* extend local deadline, drift is bounded above by 2 ticks */
> > cfs_rq->runtime_expires += TICK_NSEC;
> >
> > Because this was broken for nearly 5 years, and has recently been fixed
> > and is now being noticed by many users running kubernetes
> > (https://github.com/kubernetes/kubernetes/issues/67577) it is my opinion
> > that the mechanisms around expiring runtime should be removed
> > altogether.
> >
> > This allows only per-cpu slices to live longer than the period boundary.
> > This allows threads on runqueues that do not use much CPU to continue to
> > use their remaining slice over a longer period of time than
> > cpu.cfs_period_us. However, this helps prevents the above condition of
> > hitting throttling while also not fully utilizing your cpu quota.
> >
> > This theoretically allows a machine to use slightly more than it's
> > allotted quota in some periods. This overflow would be bounded by the
> > remaining per-cpu slice that was left un-used in the previous period.
> > For CPU bound tasks this will change nothing, as they should
> > theoretically fully utilize all of their quota and slices in each
> > period. For user-interactive tasks as described above this provides a
> > much better user/application experience as their cpu utilization will
> > more closely match the amount they requested when they hit throttling.
> >
> > This greatly improves performance of high-thread-count, non-cpu bound
> > applications with low cfs_quota_us allocation on high-core-count
> > machines. In the case of an artificial testcase, this performance
> > discrepancy has been observed to be almost 30x performance improvement,
> > while still maintaining correct cpu quota restrictions albeit over
> > longer time intervals than cpu.cfs_period_us.
>
> If the machine runs at/close to capacity, won't the overallocation
> of the quota to bursty tasks necessarily negatively impact every other
> task? Should the "unused" quota be available only on idle CPUs?
> (Or maybe this is the behavior achieved here, and only the comment and
> the commit message should be fixed...)
>
It's bounded by the amount left unused from the previous period. So
theoretically a process could use almost twice its quota. But then it
would have nothing left over in the next period. To repeat it would have
to not use any that next period. Over a longer number of periods it's the
same amount of CPU usage.
I think that is more fair than throttling a process that has never used
its full quota.
And it removes complexity.
Cheers,
Phil
> > That testcase is
> > available at https://github.com/indeedeng/fibtest.
> >
> > Fixes: 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition")
> > Signed-off-by: Dave Chiluk <chiluk+linux@indeed.com>
> > ---
> > Documentation/scheduler/sched-bwc.txt | 29 +++++++++++---
> > kernel/sched/fair.c | 71 +++--------------------------------
> > kernel/sched/sched.h | 4 --
> > 3 files changed, 29 insertions(+), 75 deletions(-)
> >
> > diff --git a/Documentation/scheduler/sched-bwc.txt b/Documentation/scheduler/sched-bwc.txt
> > index f6b1873..4ded8ae 100644
> > --- a/Documentation/scheduler/sched-bwc.txt
> > +++ b/Documentation/scheduler/sched-bwc.txt
> > @@ -8,16 +8,33 @@ CFS bandwidth control is a CONFIG_FAIR_GROUP_SCHED extension which allows the
> > specification of the maximum CPU bandwidth available to a group or hierarchy.
> >
> > The bandwidth allowed for a group is specified using a quota and period. Within
> > -each given "period" (microseconds), a group is allowed to consume only up to
> > -"quota" microseconds of CPU time. When the CPU bandwidth consumption of a
> > -group exceeds this limit (for that period), the tasks belonging to its
> > -hierarchy will be throttled and are not allowed to run again until the next
> > -period.
> > +each given "period" (microseconds), a task group is allocated up to "quota"
> > +microseconds of CPU time. When the CPU bandwidth consumption of a group
> > +exceeds this limit (for that period), the tasks belonging to its hierarchy will
> > +be throttled and are not allowed to run again until the next period.
> >
> > A group's unused runtime is globally tracked, being refreshed with quota units
> > above at each period boundary. As threads consume this bandwidth it is
> > transferred to cpu-local "silos" on a demand basis. The amount transferred
> > -within each of these updates is tunable and described as the "slice".
> > +within each of these updates is tunable and described as the "slice". Slices
> > +that are allocated to cpu-local silos do not expire at the end of the period,
> > +but unallocated quota does. This doesn't affect cpu-bound applications as they
> > +by definition consume all of their bandwidth in each each period.
> > +
> > +However for highly-threaded user-interactive/non-cpu bound applications this
> > +non-expiration nuance allows applications to burst past their quota limits
> > +equal to the amount of unused slice per cpu that the task group is running on.
> > +This slight burst requires that quota had gone unused in previous periods.
> > +Additionally this burst amount is limited to the size of a slice for every cpu
> > +a task group is run on. As a result, this mechanism still strictly limits the
> > +task group to quota average usage over a longer time windows. This provides
> > +better more predictable user experience for highly threaded applications with
> > +small quota limits on high core count machines. It also eliminates the
> > +propensity to throttle these applications while simultanously using less than
> > +quota amounts of cpu. Another way to say this, is that by allowing the unused
> > +portion of a slice to be used in following periods we have decreased the
> > +possibility of wasting unused quota on cpu-local silos that don't need much cpu
> > +time.
> >
> > Management
> > ----------
> > diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> > index f35930f..a675c69 100644
> > --- a/kernel/sched/fair.c
> > +++ b/kernel/sched/fair.c
> > @@ -4295,8 +4295,6 @@ void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
> >
> > now = sched_clock_cpu(smp_processor_id());
> > cfs_b->runtime = cfs_b->quota;
> > - cfs_b->runtime_expires = now + ktime_to_ns(cfs_b->period);
> > - cfs_b->expires_seq++;
> > }
> >
> > static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
> > @@ -4318,8 +4316,7 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> > {
> > struct task_group *tg = cfs_rq->tg;
> > struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
> > - u64 amount = 0, min_amount, expires;
> > - int expires_seq;
> > + u64 amount = 0, min_amount;
> >
> > /* note: this is a positive sum as runtime_remaining <= 0 */
> > min_amount = sched_cfs_bandwidth_slice() - cfs_rq->runtime_remaining;
> > @@ -4336,61 +4333,17 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> > cfs_b->idle = 0;
> > }
> > }
> > - expires_seq = cfs_b->expires_seq;
> > - expires = cfs_b->runtime_expires;
> > raw_spin_unlock(&cfs_b->lock);
> >
> > cfs_rq->runtime_remaining += amount;
> > - /*
> > - * we may have advanced our local expiration to account for allowed
> > - * spread between our sched_clock and the one on which runtime was
> > - * issued.
> > - */
> > - if (cfs_rq->expires_seq != expires_seq) {
> > - cfs_rq->expires_seq = expires_seq;
> > - cfs_rq->runtime_expires = expires;
> > - }
> >
> > return cfs_rq->runtime_remaining > 0;
> > }
> >
> > -/*
> > - * Note: This depends on the synchronization provided by sched_clock and the
> > - * fact that rq->clock snapshots this value.
> > - */
> > -static void expire_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> > -{
> > - struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);
> > -
> > - /* if the deadline is ahead of our clock, nothing to do */
> > - if (likely((s64)(rq_clock(rq_of(cfs_rq)) - cfs_rq->runtime_expires) < 0))
> > - return;
> > -
> > - if (cfs_rq->runtime_remaining < 0)
> > - return;
> > -
> > - /*
> > - * If the local deadline has passed we have to consider the
> > - * possibility that our sched_clock is 'fast' and the global deadline
> > - * has not truly expired.
> > - *
> > - * Fortunately we can check determine whether this the case by checking
> > - * whether the global deadline(cfs_b->expires_seq) has advanced.
> > - */
> > - if (cfs_rq->expires_seq == cfs_b->expires_seq) {
> > - /* extend local deadline, drift is bounded above by 2 ticks */
> > - cfs_rq->runtime_expires += TICK_NSEC;
> > - } else {
> > - /* global deadline is ahead, expiration has passed */
> > - cfs_rq->runtime_remaining = 0;
> > - }
> > -}
> > -
> > static void __account_cfs_rq_runtime(struct cfs_rq *cfs_rq, u64 delta_exec)
> > {
> > /* dock delta_exec before expiring quota (as it could span periods) */
> > cfs_rq->runtime_remaining -= delta_exec;
> > - expire_cfs_rq_runtime(cfs_rq);
> >
> > if (likely(cfs_rq->runtime_remaining > 0))
> > return;
> > @@ -4581,8 +4534,7 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
> > resched_curr(rq);
> > }
> >
> > -static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
> > - u64 remaining, u64 expires)
> > +static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b, u64 remaining)
> > {
> > struct cfs_rq *cfs_rq;
> > u64 runtime;
> > @@ -4604,7 +4556,6 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
> > remaining -= runtime;
> >
> > cfs_rq->runtime_remaining += runtime;
> > - cfs_rq->runtime_expires = expires;
> >
> > /* we check whether we're throttled above */
> > if (cfs_rq->runtime_remaining > 0)
> > @@ -4629,7 +4580,7 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
> > */
> > static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, unsigned long flags)
> > {
> > - u64 runtime, runtime_expires;
> > + u64 runtime;
> > int throttled;
> >
> > /* no need to continue the timer with no bandwidth constraint */
> > @@ -4657,8 +4608,6 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
> > /* account preceding periods in which throttling occurred */
> > cfs_b->nr_throttled += overrun;
> >
> > - runtime_expires = cfs_b->runtime_expires;
> > -
> > /*
> > * This check is repeated as we are holding onto the new bandwidth while
> > * we unthrottle. This can potentially race with an unthrottled group
> > @@ -4671,8 +4620,7 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
> > cfs_b->distribute_running = 1;
> > raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> > /* we can't nest cfs_b->lock while distributing bandwidth */
> > - runtime = distribute_cfs_runtime(cfs_b, runtime,
> > - runtime_expires);
> > + runtime = distribute_cfs_runtime(cfs_b, runtime);
> > raw_spin_lock_irqsave(&cfs_b->lock, flags);
> >
> > cfs_b->distribute_running = 0;
> > @@ -4749,8 +4697,7 @@ static void __return_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> > return;
> >
> > raw_spin_lock(&cfs_b->lock);
> > - if (cfs_b->quota != RUNTIME_INF &&
> > - cfs_rq->runtime_expires == cfs_b->runtime_expires) {
> > + if (cfs_b->quota != RUNTIME_INF) {
> > cfs_b->runtime += slack_runtime;
> >
> > /* we are under rq->lock, defer unthrottling using a timer */
> > @@ -4783,7 +4730,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> > {
> > u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
> > unsigned long flags;
> > - u64 expires;
> >
> > /* confirm we're still not at a refresh boundary */
> > raw_spin_lock_irqsave(&cfs_b->lock, flags);
> > @@ -4800,7 +4746,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> > if (cfs_b->quota != RUNTIME_INF && cfs_b->runtime > slice)
> > runtime = cfs_b->runtime;
> >
> > - expires = cfs_b->runtime_expires;
> > if (runtime)
> > cfs_b->distribute_running = 1;
> >
> > @@ -4809,11 +4754,9 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> > if (!runtime)
> > return;
> >
> > - runtime = distribute_cfs_runtime(cfs_b, runtime, expires);
> > + runtime = distribute_cfs_runtime(cfs_b, runtime);
> >
> > raw_spin_lock_irqsave(&cfs_b->lock, flags);
> > - if (expires == cfs_b->runtime_expires)
> > - lsub_positive(&cfs_b->runtime, runtime);
> > cfs_b->distribute_running = 0;
> > raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> > }
> > @@ -4969,8 +4912,6 @@ void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
> >
> > cfs_b->period_active = 1;
> > overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> > - cfs_b->runtime_expires += (overrun + 1) * ktime_to_ns(cfs_b->period);
> > - cfs_b->expires_seq++;
> > hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
> > }
> >
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index b52ed1a..0c0ed23 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -341,8 +341,6 @@ struct cfs_bandwidth {
> > u64 quota;
> > u64 runtime;
> > s64 hierarchical_quota;
> > - u64 runtime_expires;
> > - int expires_seq;
> >
> > short idle;
> > short period_active;
> > @@ -562,8 +560,6 @@ struct cfs_rq {
> >
> > #ifdef CONFIG_CFS_BANDWIDTH
> > int runtime_enabled;
> > - int expires_seq;
> > - u64 runtime_expires;
> > s64 runtime_remaining;
> >
> > u64 throttled_clock;
> > --
> > 1.8.3.1
> >
--
^ permalink raw reply
* Re: [PATCH] Documentation: gpio: remove duplicated lines
From: Linus Walleij @ 2019-05-24 11:27 UTC (permalink / raw)
To: Luca Ceresoli
Cc: open list:GPIO SUBSYSTEM, Bartosz Golaszewski, Jonathan Corbet,
linux-kernel@vger.kernel.org, Linux Doc Mailing List
In-Reply-To: <20190510090339.17211-1-luca@lucaceresoli.net>
On Fri, May 10, 2019 at 11:03 AM Luca Ceresoli <luca@lucaceresoli.net> wrote:
> The 'default (active high)' lines are repeated twice. Avoid people stare at
> their screens looking for differences.
>
> Signed-off-by: Luca Ceresoli <luca@lucaceresoli.net>
Patch applied with Bartosz's ACK.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v2 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Peter Zijlstra @ 2019-05-24 8:55 UTC (permalink / raw)
To: Dave Chiluk
Cc: Ingo Molnar, cgroups, linux-kernel, Brendan Gregg, Kyle Anderson,
Gabriel Munos, John Hammond, Cong Wang, Jonathan Corbet,
linux-doc, Ben Segall, Paul Turner
In-Reply-To: <1558637087-20283-2-git-send-email-chiluk+linux@indeed.com>
(it always helps to Cc the people who actually wrote the code)
Ben, can you have a look at this?
On Thu, May 23, 2019 at 01:44:47PM -0500, Dave Chiluk wrote:
> It has been observed, that highly-threaded, non-cpu-bound applications
> running under cpu.cfs_quota_us constraints can hit a high percentage of
> periods throttled while simultaneously not consuming the allocated
> amount of quota. This use case is typical of user-interactive non-cpu
> bound applications, such as those running in kubernetes or mesos when
> run on multiple cpu cores.
>
> This has been root caused to threads being allocated per cpu bandwidth
> slices, and then not fully using that slice within the period. At which
> point the slice and quota expires. This expiration of unused slice
> results in applications not being able to utilize the quota for which
> they are allocated.
>
> The expiration of per-cpu slices was recently fixed by
> 'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
> condition")'. Prior to that it appears that this has been broken since
> at least 'commit 51f2176d74ac ("sched/fair: Fix unlocked reads of some
> cfs_b->quota/period")' which was introduced in v3.16-rc1 in 2014. That
> added the following conditional which resulted in slices never being
> expired.
>
> if (cfs_rq->runtime_expires != cfs_b->runtime_expires) {
> /* extend local deadline, drift is bounded above by 2 ticks */
> cfs_rq->runtime_expires += TICK_NSEC;
>
> Because this was broken for nearly 5 years, and has recently been fixed
> and is now being noticed by many users running kubernetes
> (https://github.com/kubernetes/kubernetes/issues/67577) it is my opinion
> that the mechanisms around expiring runtime should be removed
> altogether.
>
> This allows only per-cpu slices to live longer than the period boundary.
> This allows threads on runqueues that do not use much CPU to continue to
> use their remaining slice over a longer period of time than
> cpu.cfs_period_us. However, this helps prevents the above condition of
> hitting throttling while also not fully utilizing your cpu quota.
>
> This theoretically allows a machine to use slightly more than it's
> allotted quota in some periods. This overflow would be bounded by the
> remaining per-cpu slice that was left un-used in the previous period.
> For CPU bound tasks this will change nothing, as they should
> theoretically fully utilize all of their quota and slices in each
> period. For user-interactive tasks as described above this provides a
> much better user/application experience as their cpu utilization will
> more closely match the amount they requested when they hit throttling.
>
> This greatly improves performance of high-thread-count, non-cpu bound
> applications with low cfs_quota_us allocation on high-core-count
> machines. In the case of an artificial testcase, this performance
> discrepancy has been observed to be almost 30x performance improvement,
> while still maintaining correct cpu quota restrictions albeit over
> longer time intervals than cpu.cfs_period_us. That testcase is
> available at https://github.com/indeedeng/fibtest.
>
> Fixes: 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition")
> Signed-off-by: Dave Chiluk <chiluk+linux@indeed.com>
> ---
> Documentation/scheduler/sched-bwc.txt | 29 +++++++++++---
> kernel/sched/fair.c | 71 +++--------------------------------
> kernel/sched/sched.h | 4 --
> 3 files changed, 29 insertions(+), 75 deletions(-)
>
> diff --git a/Documentation/scheduler/sched-bwc.txt b/Documentation/scheduler/sched-bwc.txt
> index f6b1873..4ded8ae 100644
> --- a/Documentation/scheduler/sched-bwc.txt
> +++ b/Documentation/scheduler/sched-bwc.txt
> @@ -8,16 +8,33 @@ CFS bandwidth control is a CONFIG_FAIR_GROUP_SCHED extension which allows the
> specification of the maximum CPU bandwidth available to a group or hierarchy.
>
> The bandwidth allowed for a group is specified using a quota and period. Within
> -each given "period" (microseconds), a group is allowed to consume only up to
> -"quota" microseconds of CPU time. When the CPU bandwidth consumption of a
> -group exceeds this limit (for that period), the tasks belonging to its
> -hierarchy will be throttled and are not allowed to run again until the next
> -period.
> +each given "period" (microseconds), a task group is allocated up to "quota"
> +microseconds of CPU time. When the CPU bandwidth consumption of a group
> +exceeds this limit (for that period), the tasks belonging to its hierarchy will
> +be throttled and are not allowed to run again until the next period.
>
> A group's unused runtime is globally tracked, being refreshed with quota units
> above at each period boundary. As threads consume this bandwidth it is
> transferred to cpu-local "silos" on a demand basis. The amount transferred
> -within each of these updates is tunable and described as the "slice".
> +within each of these updates is tunable and described as the "slice". Slices
> +that are allocated to cpu-local silos do not expire at the end of the period,
> +but unallocated quota does. This doesn't affect cpu-bound applications as they
> +by definition consume all of their bandwidth in each each period.
> +
> +However for highly-threaded user-interactive/non-cpu bound applications this
> +non-expiration nuance allows applications to burst past their quota limits
> +equal to the amount of unused slice per cpu that the task group is running on.
> +This slight burst requires that quota had gone unused in previous periods.
> +Additionally this burst amount is limited to the size of a slice for every cpu
> +a task group is run on. As a result, this mechanism still strictly limits the
> +task group to quota average usage over a longer time windows. This provides
> +better more predictable user experience for highly threaded applications with
> +small quota limits on high core count machines. It also eliminates the
> +propensity to throttle these applications while simultanously using less than
> +quota amounts of cpu. Another way to say this, is that by allowing the unused
> +portion of a slice to be used in following periods we have decreased the
> +possibility of wasting unused quota on cpu-local silos that don't need much cpu
> +time.
>
> Management
> ----------
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index f35930f..a675c69 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4295,8 +4295,6 @@ void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
>
> now = sched_clock_cpu(smp_processor_id());
> cfs_b->runtime = cfs_b->quota;
> - cfs_b->runtime_expires = now + ktime_to_ns(cfs_b->period);
> - cfs_b->expires_seq++;
> }
>
> static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
> @@ -4318,8 +4316,7 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> {
> struct task_group *tg = cfs_rq->tg;
> struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
> - u64 amount = 0, min_amount, expires;
> - int expires_seq;
> + u64 amount = 0, min_amount;
>
> /* note: this is a positive sum as runtime_remaining <= 0 */
> min_amount = sched_cfs_bandwidth_slice() - cfs_rq->runtime_remaining;
> @@ -4336,61 +4333,17 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> cfs_b->idle = 0;
> }
> }
> - expires_seq = cfs_b->expires_seq;
> - expires = cfs_b->runtime_expires;
> raw_spin_unlock(&cfs_b->lock);
>
> cfs_rq->runtime_remaining += amount;
> - /*
> - * we may have advanced our local expiration to account for allowed
> - * spread between our sched_clock and the one on which runtime was
> - * issued.
> - */
> - if (cfs_rq->expires_seq != expires_seq) {
> - cfs_rq->expires_seq = expires_seq;
> - cfs_rq->runtime_expires = expires;
> - }
>
> return cfs_rq->runtime_remaining > 0;
> }
>
> -/*
> - * Note: This depends on the synchronization provided by sched_clock and the
> - * fact that rq->clock snapshots this value.
> - */
> -static void expire_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> -{
> - struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);
> -
> - /* if the deadline is ahead of our clock, nothing to do */
> - if (likely((s64)(rq_clock(rq_of(cfs_rq)) - cfs_rq->runtime_expires) < 0))
> - return;
> -
> - if (cfs_rq->runtime_remaining < 0)
> - return;
> -
> - /*
> - * If the local deadline has passed we have to consider the
> - * possibility that our sched_clock is 'fast' and the global deadline
> - * has not truly expired.
> - *
> - * Fortunately we can check determine whether this the case by checking
> - * whether the global deadline(cfs_b->expires_seq) has advanced.
> - */
> - if (cfs_rq->expires_seq == cfs_b->expires_seq) {
> - /* extend local deadline, drift is bounded above by 2 ticks */
> - cfs_rq->runtime_expires += TICK_NSEC;
> - } else {
> - /* global deadline is ahead, expiration has passed */
> - cfs_rq->runtime_remaining = 0;
> - }
> -}
> -
> static void __account_cfs_rq_runtime(struct cfs_rq *cfs_rq, u64 delta_exec)
> {
> /* dock delta_exec before expiring quota (as it could span periods) */
> cfs_rq->runtime_remaining -= delta_exec;
> - expire_cfs_rq_runtime(cfs_rq);
>
> if (likely(cfs_rq->runtime_remaining > 0))
> return;
> @@ -4581,8 +4534,7 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
> resched_curr(rq);
> }
>
> -static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
> - u64 remaining, u64 expires)
> +static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b, u64 remaining)
> {
> struct cfs_rq *cfs_rq;
> u64 runtime;
> @@ -4604,7 +4556,6 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
> remaining -= runtime;
>
> cfs_rq->runtime_remaining += runtime;
> - cfs_rq->runtime_expires = expires;
>
> /* we check whether we're throttled above */
> if (cfs_rq->runtime_remaining > 0)
> @@ -4629,7 +4580,7 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
> */
> static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, unsigned long flags)
> {
> - u64 runtime, runtime_expires;
> + u64 runtime;
> int throttled;
>
> /* no need to continue the timer with no bandwidth constraint */
> @@ -4657,8 +4608,6 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
> /* account preceding periods in which throttling occurred */
> cfs_b->nr_throttled += overrun;
>
> - runtime_expires = cfs_b->runtime_expires;
> -
> /*
> * This check is repeated as we are holding onto the new bandwidth while
> * we unthrottle. This can potentially race with an unthrottled group
> @@ -4671,8 +4620,7 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
> cfs_b->distribute_running = 1;
> raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> /* we can't nest cfs_b->lock while distributing bandwidth */
> - runtime = distribute_cfs_runtime(cfs_b, runtime,
> - runtime_expires);
> + runtime = distribute_cfs_runtime(cfs_b, runtime);
> raw_spin_lock_irqsave(&cfs_b->lock, flags);
>
> cfs_b->distribute_running = 0;
> @@ -4749,8 +4697,7 @@ static void __return_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> return;
>
> raw_spin_lock(&cfs_b->lock);
> - if (cfs_b->quota != RUNTIME_INF &&
> - cfs_rq->runtime_expires == cfs_b->runtime_expires) {
> + if (cfs_b->quota != RUNTIME_INF) {
> cfs_b->runtime += slack_runtime;
>
> /* we are under rq->lock, defer unthrottling using a timer */
> @@ -4783,7 +4730,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> {
> u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
> unsigned long flags;
> - u64 expires;
>
> /* confirm we're still not at a refresh boundary */
> raw_spin_lock_irqsave(&cfs_b->lock, flags);
> @@ -4800,7 +4746,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> if (cfs_b->quota != RUNTIME_INF && cfs_b->runtime > slice)
> runtime = cfs_b->runtime;
>
> - expires = cfs_b->runtime_expires;
> if (runtime)
> cfs_b->distribute_running = 1;
>
> @@ -4809,11 +4754,9 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> if (!runtime)
> return;
>
> - runtime = distribute_cfs_runtime(cfs_b, runtime, expires);
> + runtime = distribute_cfs_runtime(cfs_b, runtime);
>
> raw_spin_lock_irqsave(&cfs_b->lock, flags);
> - if (expires == cfs_b->runtime_expires)
> - lsub_positive(&cfs_b->runtime, runtime);
> cfs_b->distribute_running = 0;
> raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> }
> @@ -4969,8 +4912,6 @@ void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
>
> cfs_b->period_active = 1;
> overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> - cfs_b->runtime_expires += (overrun + 1) * ktime_to_ns(cfs_b->period);
> - cfs_b->expires_seq++;
> hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
> }
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index b52ed1a..0c0ed23 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -341,8 +341,6 @@ struct cfs_bandwidth {
> u64 quota;
> u64 runtime;
> s64 hierarchical_quota;
> - u64 runtime_expires;
> - int expires_seq;
>
> short idle;
> short period_active;
> @@ -562,8 +560,6 @@ struct cfs_rq {
>
> #ifdef CONFIG_CFS_BANDWIDTH
> int runtime_enabled;
> - int expires_seq;
> - u64 runtime_expires;
> s64 runtime_remaining;
>
> u64 throttled_clock;
> --
> 1.8.3.1
>
^ permalink raw reply
* [PATCH v2 3/3] KVM: vmx: handle vm-exit for UMWAIT and TPAUSE
From: Tao Xu @ 2019-05-24 7:56 UTC (permalink / raw)
To: pbonzini, rkrcmar, corbet, tglx, mingo, bp, hpa,
sean.j.christopherson
Cc: x86, kvm, linux-doc, linux-kernel, tao3.xu, jingqi.liu
In-Reply-To: <20190524075637.29496-1-tao3.xu@intel.com>
As the latest Intel 64 and IA-32 Architectures Software Developer's
Manual, UMWAIT and TPAUSE instructions cause a VM exit if the
“RDTSC exiting” and “enable user wait and pause” VM-execution controls
are both 1.
This patch is to handle the vm-exit for UMWAIT and TPAUSE as invalid_op.
Co-developed-by: Jingqi Liu <jingqi.liu@intel.com>
Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---
arch/x86/include/uapi/asm/vmx.h | 6 +++++-
arch/x86/kvm/vmx/vmx.c | 16 ++++++++++++++++
2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/arch/x86/include/uapi/asm/vmx.h b/arch/x86/include/uapi/asm/vmx.h
index d213ec5c3766..d88d7a68849b 100644
--- a/arch/x86/include/uapi/asm/vmx.h
+++ b/arch/x86/include/uapi/asm/vmx.h
@@ -85,6 +85,8 @@
#define EXIT_REASON_PML_FULL 62
#define EXIT_REASON_XSAVES 63
#define EXIT_REASON_XRSTORS 64
+#define EXIT_REASON_UMWAIT 67
+#define EXIT_REASON_TPAUSE 68
#define VMX_EXIT_REASONS \
{ EXIT_REASON_EXCEPTION_NMI, "EXCEPTION_NMI" }, \
@@ -142,7 +144,9 @@
{ EXIT_REASON_RDSEED, "RDSEED" }, \
{ EXIT_REASON_PML_FULL, "PML_FULL" }, \
{ EXIT_REASON_XSAVES, "XSAVES" }, \
- { EXIT_REASON_XRSTORS, "XRSTORS" }
+ { EXIT_REASON_XRSTORS, "XRSTORS" }, \
+ { EXIT_REASON_UMWAIT, "UMWAIT" }, \
+ { EXIT_REASON_TPAUSE, "TPAUSE" }
#define VMX_ABORT_SAVE_GUEST_MSR_FAIL 1
#define VMX_ABORT_LOAD_HOST_PDPTE_FAIL 2
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 49e107692aee..0743a4ac2b61 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -5337,6 +5337,20 @@ static int handle_monitor(struct kvm_vcpu *vcpu)
return handle_nop(vcpu);
}
+static int handle_umwait(struct kvm_vcpu *vcpu)
+{
+ printk_once(KERN_WARNING "kvm: Can't use UMWAIT instruction "
+ "when RDTSC exiting VM-execution control is enabled!\n");
+ return handle_invalid_op(vcpu);
+}
+
+static int handle_tpause(struct kvm_vcpu *vcpu)
+{
+ printk_once(KERN_WARNING "kvm: Can't use TPAUSE instruction "
+ "when RDTSC exiting VM-execution control is enabled!\n");
+ return handle_invalid_op(vcpu);
+}
+
static int handle_invpcid(struct kvm_vcpu *vcpu)
{
u32 vmx_instruction_info;
@@ -5547,6 +5561,8 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
[EXIT_REASON_VMFUNC] = handle_vmx_instruction,
[EXIT_REASON_PREEMPTION_TIMER] = handle_preemption_timer,
[EXIT_REASON_ENCLS] = handle_encls,
+ [EXIT_REASON_UMWAIT] = handle_umwait,
+ [EXIT_REASON_TPAUSE] = handle_tpause,
};
static const int kvm_vmx_max_exit_handlers =
--
2.20.1
^ permalink raw reply related
* [PATCH v2 2/3] KVM: vmx: Emulate MSR IA32_UMWAIT_CONTROL
From: Tao Xu @ 2019-05-24 7:56 UTC (permalink / raw)
To: pbonzini, rkrcmar, corbet, tglx, mingo, bp, hpa,
sean.j.christopherson
Cc: x86, kvm, linux-doc, linux-kernel, tao3.xu, jingqi.liu
In-Reply-To: <20190524075637.29496-1-tao3.xu@intel.com>
UMWAIT and TPAUSE instructions use IA32_UMWAIT_CONTROL at MSR index E1H
to determines the maximum time in TSC-quanta that the processor can reside
in either C0.1 or C0.2.
This patch is to emulate MSR IA32_UMWAIT_CONTROL in guest and
differentiate MSR_TEST_CTL between host and guest.
Co-developed-by: Jingqi Liu <jingqi.liu@intel.com>
Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---
arch/x86/kvm/vmx/vmx.c | 42 ++++++++++++++++++++++++++++++++++++++++++
arch/x86/kvm/vmx/vmx.h | 1 +
arch/x86/kvm/x86.c | 1 +
3 files changed, 44 insertions(+)
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index a65ee7ea47b4..49e107692aee 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -1676,6 +1676,14 @@ static int vmx_get_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
#endif
case MSR_EFER:
return kvm_get_msr_common(vcpu, msr_info);
+ case MSR_IA32_UMWAIT_CONTROL:
+ if (!kvm_enable_usr_wait_pause(vmx->vcpu.kvm) ||
+ (!msr_info->host_initiated &&
+ !guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG)))
+ return 1;
+
+ msr_info->data = vmx->msr_ia32_umwait_control;
+ break;
case MSR_IA32_SPEC_CTRL:
if (!msr_info->host_initiated &&
!guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
@@ -1838,6 +1846,16 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
return 1;
vmcs_write64(GUEST_BNDCFGS, data);
break;
+ case MSR_IA32_UMWAIT_CONTROL:
+ if (!kvm_enable_usr_wait_pause(vmx->vcpu.kvm) ||
+ !guest_cpuid_has(vcpu, X86_FEATURE_WAITPKG))
+ return 1;
+
+ if (!data)
+ break;
+
+ vmx->msr_ia32_umwait_control = data;
+ break;
case MSR_IA32_SPEC_CTRL:
if (!msr_info->host_initiated &&
!guest_cpuid_has(vcpu, X86_FEATURE_SPEC_CTRL))
@@ -4085,6 +4103,8 @@ static void vmx_vcpu_setup(struct vcpu_vmx *vmx)
++vmx->nmsrs;
}
+ vmx->msr_ia32_umwait_control = 0;
+
vm_exit_controls_init(vmx, vmx_vmexit_ctrl());
/* 22.2.1, 20.8.1 */
@@ -4123,6 +4143,8 @@ static void vmx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
vmx->rmode.vm86_active = 0;
vmx->spec_ctrl = 0;
+ vmx->msr_ia32_umwait_control = 0;
+
vcpu->arch.microcode_version = 0x100000000ULL;
vmx->vcpu.arch.regs[VCPU_REGS_RDX] = get_rdx_init_val();
kvm_set_cr8(vcpu, 0);
@@ -6327,6 +6349,24 @@ static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx)
msrs[i].host, false);
}
+static void atomic_switch_ia32_umwait_control(struct vcpu_vmx *vmx)
+{
+ u64 host_umwait_control;
+
+ if (!kvm_enable_usr_wait_pause(vmx->vcpu.kvm))
+ return;
+
+ if (rdmsrl_safe(MSR_IA32_UMWAIT_CONTROL, &host_umwait_control))
+ return;
+
+ if (vmx->msr_ia32_umwait_control != host_umwait_control)
+ add_atomic_switch_msr(vmx, MSR_IA32_UMWAIT_CONTROL,
+ vmx->msr_ia32_umwait_control,
+ host_umwait_control, false);
+ else
+ clear_atomic_switch_msr(vmx, MSR_IA32_UMWAIT_CONTROL);
+}
+
static void vmx_arm_hv_timer(struct vcpu_vmx *vmx, u32 val)
{
vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, val);
@@ -6435,6 +6475,8 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
atomic_switch_perf_msrs(vmx);
+ atomic_switch_ia32_umwait_control(vmx);
+
vmx_update_hv_timer(vcpu);
/*
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 63d37ccce3dc..7b779f8816fb 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -194,6 +194,7 @@ struct vcpu_vmx {
#endif
u64 spec_ctrl;
+ u64 msr_ia32_umwait_control;
u32 vm_entry_controls_shadow;
u32 vm_exit_controls_shadow;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 38a89c878c5d..245ed4a63765 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1138,6 +1138,7 @@ static u32 msrs_to_save[] = {
MSR_IA32_RTIT_ADDR1_A, MSR_IA32_RTIT_ADDR1_B,
MSR_IA32_RTIT_ADDR2_A, MSR_IA32_RTIT_ADDR2_B,
MSR_IA32_RTIT_ADDR3_A, MSR_IA32_RTIT_ADDR3_B,
+ MSR_IA32_UMWAIT_CONTROL,
};
static unsigned num_msrs_to_save;
--
2.20.1
^ permalink raw reply related
* [PATCH v2 0/3] KVM: x86: Enable user wait instructions
From: Tao Xu @ 2019-05-24 7:56 UTC (permalink / raw)
To: pbonzini, rkrcmar, corbet, tglx, mingo, bp, hpa,
sean.j.christopherson
Cc: x86, kvm, linux-doc, linux-kernel, tao3.xu, jingqi.liu
UMONITOR, UMWAIT and TPAUSE are a set of user wait instructions.
UMONITOR arms address monitoring hardware using an address. A store
to an address within the specified address range triggers the
monitoring hardware to wake up the processor waiting in umwait.
UMWAIT instructs the processor to enter an implementation-dependent
optimized state while monitoring a range of addresses. The optimized
state may be either a light-weight power/performance optimized state
(c0.1 state) or an improved power/performance optimized state
(c0.2 state).
TPAUSE instructs the processor to enter an implementation-dependent
optimized state c0.1 or c0.2 state and wake up when time-stamp counter
reaches specified timeout.
Availability of the user wait instructions is indicated by the presence
of the CPUID feature flag WAITPKG CPUID.0x07.0x0:ECX[5].
The patches enable the umonitor, umwait and tpause features in KVM.
Because umwait and tpause can put a (psysical) CPU into a power saving
state, by default we dont't expose it to kvm and provide a capability to
enable it. With this capability enabled, a VM can use UMONITOR, UMWAIT
and TPAUSE instructions. If the instruction causes a delay, the amount
of time delayed is called here the physical delay. The physical delay is
first computed by determining the virtual delay (the time to delay
relative to the VM’s timestamp counter). Otherwise, UMONITOR, UMWAIT
and TPAUSE cause an invalid-opcode exception(#UD).
The release document ref below link:
https://software.intel.com/sites/default/files/\
managed/39/c5/325462-sdm-vol-1-2abcd-3abcd.pdf
This patch has a dependency on https://lkml.org/lkml/2019/1/16/909
Changelog:
v2:
Separated from the series https://lkml.org/lkml/2018/7/10/160
Add provide a capability to enable UMONITOR, UMWAIT and TPAUSE
v1:
Sent out with MOVDIRI/MOVDIR64B instructions patches
Tao Xu (3):
KVM: x86: add support for user wait instructions
KVM: vmx: Emulate MSR IA32_UMWAIT_CONTROL
KVM: vmx: handle vm-exit for UMWAIT and TPAUSE
Documentation/virtual/kvm/api.txt | 12 ++++++
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/include/asm/vmx.h | 1 +
arch/x86/include/uapi/asm/vmx.h | 6 ++-
arch/x86/kvm/cpuid.c | 2 +-
arch/x86/kvm/vmx/vmx.c | 62 +++++++++++++++++++++++++++++++
arch/x86/kvm/vmx/vmx.h | 1 +
arch/x86/kvm/x86.c | 8 ++++
arch/x86/kvm/x86.h | 5 +++
include/uapi/linux/kvm.h | 1 +
10 files changed, 97 insertions(+), 2 deletions(-)
--
2.20.1
^ permalink raw reply
* [PATCH v2 1/3] KVM: x86: add support for user wait instructions
From: Tao Xu @ 2019-05-24 7:56 UTC (permalink / raw)
To: pbonzini, rkrcmar, corbet, tglx, mingo, bp, hpa,
sean.j.christopherson
Cc: x86, kvm, linux-doc, linux-kernel, tao3.xu, jingqi.liu
In-Reply-To: <20190524075637.29496-1-tao3.xu@intel.com>
This patch adds support for UMONITOR, UMWAIT and TPAUSE instructions
in kvm, and by default dont't expose it to kvm and provide a capability
to enable it.
Co-developed-by: Jingqi Liu <jingqi.liu@intel.com>
Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
Signed-off-by: Tao Xu <tao3.xu@intel.com>
---
Documentation/virtual/kvm/api.txt | 12 ++++++++++++
arch/x86/include/asm/kvm_host.h | 1 +
arch/x86/include/asm/vmx.h | 1 +
arch/x86/kvm/cpuid.c | 2 +-
arch/x86/kvm/vmx/vmx.c | 4 ++++
arch/x86/kvm/x86.c | 7 +++++++
arch/x86/kvm/x86.h | 5 +++++
include/uapi/linux/kvm.h | 1 +
8 files changed, 32 insertions(+), 1 deletion(-)
diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index ba6c42c576dd..3d0196220486 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -4997,6 +4997,18 @@ it hard or impossible to use it correctly. The availability of
KVM_CAP_MANUAL_DIRTY_LOG_PROTECT2 signals that those bugs are fixed.
Userspace should not try to use KVM_CAP_MANUAL_DIRTY_LOG_PROTECT.
+7.19 KVM_CAP_ENABLE_USR_WAIT_PAUSE
+
+Architectures: x86
+Parameters: args[0] whether feature should be enabled or not
+
+With this capability enabled, a VM can use UMONITOR, UMWAIT and TPAUSE
+instructions. If the instruction causes a delay, the amount of
+time delayed is called here the physical delay. The physical delay is
+first computed by determining the virtual delay (the time to delay
+relative to the VM’s timestamp counter). Otherwise, UMONITOR, UMWAIT
+and TPAUSE cause an invalid-opcode exception(#UD).
+
8. Other capabilities.
----------------------
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 450d69a1e6fa..0da87c2e1c4d 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -882,6 +882,7 @@ struct kvm_arch {
bool mwait_in_guest;
bool hlt_in_guest;
bool pause_in_guest;
+ bool enable_usr_wait_pause;
unsigned long irq_sources_bitmap;
s64 kvmclock_offset;
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 4e4133e86484..1c94b1009288 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -82,6 +82,7 @@
#define SECONDARY_EXEC_PT_USE_GPA 0x01000000
#define SECONDARY_EXEC_MODE_BASED_EPT_EXEC 0x00400000
#define SECONDARY_EXEC_TSC_SCALING 0x02000000
+#define SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE 0x04000000
#define PIN_BASED_EXT_INTR_MASK 0x00000001
#define PIN_BASED_NMI_EXITING 0x00000008
diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 80a642a0143d..1cc001870a9d 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -405,7 +405,7 @@ static inline int __do_cpuid_ent(struct kvm_cpuid_entry2 *entry, u32 function,
F(AVX512VBMI) | F(LA57) | F(PKU) | 0 /*OSPKE*/ |
F(AVX512_VPOPCNTDQ) | F(UMIP) | F(AVX512_VBMI2) | F(GFNI) |
F(VAES) | F(VPCLMULQDQ) | F(AVX512_VNNI) | F(AVX512_BITALG) |
- F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B);
+ F(CLDEMOTE) | F(MOVDIRI) | F(MOVDIR64B) | 0 /*WAITPKG*/;
/* cpuid 7.0.edx*/
const u32 kvm_cpuid_7_0_edx_x86_features =
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 1ac167614032..a65ee7ea47b4 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2247,6 +2247,7 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
SECONDARY_EXEC_RDRAND_EXITING |
SECONDARY_EXEC_ENABLE_PML |
SECONDARY_EXEC_TSC_SCALING |
+ SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE |
SECONDARY_EXEC_PT_USE_GPA |
SECONDARY_EXEC_PT_CONCEAL_VMX |
SECONDARY_EXEC_ENABLE_VMFUNC |
@@ -3880,6 +3881,9 @@ static void vmx_compute_secondary_exec_control(struct vcpu_vmx *vmx)
exec_control &= ~SECONDARY_EXEC_UNRESTRICTED_GUEST;
if (kvm_pause_in_guest(vmx->vcpu.kvm))
exec_control &= ~SECONDARY_EXEC_PAUSE_LOOP_EXITING;
+ if (!kvm_enable_usr_wait_pause(vmx->vcpu.kvm) ||
+ (vmcs_config.cpu_based_exec_ctrl & CPU_BASED_RDTSC_EXITING))
+ exec_control &= ~SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE;
if (!kvm_vcpu_apicv_active(vcpu))
exec_control &= ~(SECONDARY_EXEC_APIC_REGISTER_VIRT |
SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 536b78c4af6e..38a89c878c5d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3141,6 +3141,9 @@ int kvm_vm_ioctl_check_extension(struct kvm *kvm, long ext)
r = kvm_x86_ops->get_nested_state ?
kvm_x86_ops->get_nested_state(NULL, NULL, 0) : 0;
break;
+ case KVM_CAP_ENABLE_USR_WAIT_PAUSE:
+ r = boot_cpu_has(X86_FEATURE_WAITPKG);
+ break;
default:
break;
}
@@ -4622,6 +4625,10 @@ int kvm_vm_ioctl_enable_cap(struct kvm *kvm,
kvm->arch.exception_payload_enabled = cap->args[0];
r = 0;
break;
+ case KVM_CAP_ENABLE_USR_WAIT_PAUSE:
+ kvm->arch.enable_usr_wait_pause = true;
+ r = 0;
+ break;
default:
r = -EINVAL;
break;
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index a470ff0868c5..37685e6679f3 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -333,6 +333,11 @@ static inline bool kvm_pause_in_guest(struct kvm *kvm)
return kvm->arch.pause_in_guest;
}
+static inline bool kvm_enable_usr_wait_pause(struct kvm *kvm)
+{
+ return kvm->arch.enable_usr_wait_pause;
+}
+
DECLARE_PER_CPU(struct kvm_vcpu *, current_vcpu);
static inline void kvm_before_interrupt(struct kvm_vcpu *vcpu)
diff --git a/include/uapi/linux/kvm.h b/include/uapi/linux/kvm.h
index 2fe12b40d503..5a19a5984c57 100644
--- a/include/uapi/linux/kvm.h
+++ b/include/uapi/linux/kvm.h
@@ -993,6 +993,7 @@ struct kvm_ppc_resize_hpt {
#define KVM_CAP_ARM_SVE 170
#define KVM_CAP_ARM_PTRAUTH_ADDRESS 171
#define KVM_CAP_ARM_PTRAUTH_GENERIC 172
+#define KVM_CAP_ENABLE_USR_WAIT_PAUSE 173
#ifdef KVM_CAP_IRQ_ROUTING
--
2.20.1
^ permalink raw reply related
* [v4 PATCH] RISC-V: Add an Image header that boot loader can parse.
From: Atish Patra @ 2019-05-24 4:18 UTC (permalink / raw)
To: linux-kernel
Cc: Atish Patra, Karsten Merker, Albert Ou, Anup Patel,
Jonathan Corbet, linux-doc, linux-riscv, Nick Kossifidis,
Palmer Dabbelt, Zong Li, paul.walmsley@sifive.com,
marek.vasut@gmail.com, linux-arm-kernel, mark.rutland,
catalin.marinas, ard.biesheuvel
Currently, the last stage boot loaders such as U-Boot can accept only
uImage which is an unnecessary additional step in automating boot
process.
Add an image header that boot loader understands and boot Linux from
flat Image directly.
This header is based on ARM64 boot image header and provides an
opportunity to combine both ARM64 & RISC-V image headers in future.
Also make sure that PE/COFF header can co-exist in the same image so
that EFI stub can be supported for RISC-V in future. EFI specification
needs PE/COFF image header in the beginning of the kernel image in order
to load it as an EFI application. In order to support EFI stub, code0
should be replaced with "MZ" magic string and res4(at offset 0x3c)
should point to the rest of the PE/COFF header (which will be added
during EFI support).
Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
Signed-off-by: Atish Patra <atish.patra@wdc.com>
Reviewed-by: Karsten Merker <merker@debian.org>
Tested-by: Karsten Merker <merker@debian.org> (QEMU+OpenSBI+U-Boot)
---
I have not sent out corresponding U-Boot patch as all the changes are
compatible with current u-boot support. Once, the kernel header format
is agreed upon, I will update the U-Boot patch.
Changes from v3->v4
1. Update the commit text to clarify about PE/COFF header.
Changes from v2->v3
1. Modified reserved fields to define a header version.
2. Added header documentation.
Changes from v1-v2:
1. Added additional reserved elements to make it fully PE compatible.
---
| 50 ++++++++++++++++++
arch/riscv/include/asm/image.h | 64 +++++++++++++++++++++++
arch/riscv/kernel/head.S | 32 ++++++++++++
3 files changed, 146 insertions(+)
create mode 100644 Documentation/riscv/boot-image-header.txt
create mode 100644 arch/riscv/include/asm/image.h
--git a/Documentation/riscv/boot-image-header.txt b/Documentation/riscv/boot-image-header.txt
new file mode 100644
index 000000000000..68abc2353cec
--- /dev/null
+++ b/Documentation/riscv/boot-image-header.txt
@@ -0,0 +1,50 @@
+ Boot image header in RISC-V Linux
+ =============================================
+
+Author: Atish Patra <atish.patra@wdc.com>
+Date : 20 May 2019
+
+This document only describes the boot image header details for RISC-V Linux.
+The complete booting guide will be available at Documentation/riscv/booting.txt.
+
+The following 64-byte header is present in decompressed Linux kernel image.
+
+ u32 code0; /* Executable code */
+ u32 code1; /* Executable code */
+ u64 text_offset; /* Image load offset, little endian */
+ u64 image_size; /* Effective Image size, little endian */
+ u64 flags; /* kernel flags, little endian */
+ u32 version; /* Version of this header */
+ u32 res1 = 0; /* Reserved */
+ u64 res2 = 0; /* Reserved */
+ u64 magic = 0x5643534952; /* Magic number, little endian, "RISCV" */
+ u32 res3; /* Reserved for additional RISC-V specific header */
+ u32 res4; /* Reserved for PE COFF offset */
+
+This header format is compliant with PE/COFF header and largely inspired from
+ARM64 header. Thus, both ARM64 & RISC-V header can be combined into one common
+header in future.
+
+Notes:
+- This header can also be reused to support EFI stub for RISC-V in future. EFI
+ specification needs PE/COFF image header in the beginning of the kernel image
+ in order to load it as an EFI application. In order to support EFI stub,
+ code0 should be replaced with "MZ" magic string and res5(at offset 0x3c) should
+ point to the rest of the PE/COFF header.
+
+- version field indicate header version number.
+ Bits 0:15 - Minor version
+ Bits 16:31 - Major version
+
+ This preserves compatibility across newer and older version of the header.
+ The current version is defined as 0.1.
+
+- res3 is reserved for offset to any other additional fields. This makes the
+ header extendible in future. One example would be to accommodate ISA
+ extension for RISC-V in future. For current version, it is set to be zero.
+
+- In current header, the flag field has only one field.
+ Bit 0: Kernel endianness. 1 if BE, 0 if LE.
+
+- Image size is mandatory for boot loader to load kernel image. Booting will
+ fail otherwise.
diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
new file mode 100644
index 000000000000..61c9f20d2f19
--- /dev/null
+++ b/arch/riscv/include/asm/image.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ASM_IMAGE_H
+#define __ASM_IMAGE_H
+
+#define RISCV_IMAGE_MAGIC "RISCV"
+
+
+#define RISCV_IMAGE_FLAG_BE_SHIFT 0
+#define RISCV_IMAGE_FLAG_BE_MASK 0x1
+
+#define RISCV_IMAGE_FLAG_LE 0
+#define RISCV_IMAGE_FLAG_BE 1
+
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_BE
+#else
+#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
+#endif
+
+#define __HEAD_FLAG(field) (__HEAD_FLAG_##field << \
+ RISCV_IMAGE_FLAG_##field##_SHIFT)
+
+#define __HEAD_FLAGS (__HEAD_FLAG(BE))
+
+#define RISCV_HEADER_VERSION_MAJOR 0
+#define RISCV_HEADER_VERSION_MINOR 1
+
+#define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
+ RISCV_HEADER_VERSION_MINOR)
+
+#ifndef __ASSEMBLY__
+/*
+ * struct riscv_image_header - riscv kernel image header
+ *
+ * @code0: Executable code
+ * @code1: Executable code
+ * @text_offset: Image load offset
+ * @image_size: Effective Image size
+ * @flags: kernel flags
+ * @version: version
+ * @reserved: reserved
+ * @reserved: reserved
+ * @magic: Magic number
+ * @reserved: reserved (will be used for additional RISC-V specific header)
+ * @reserved: reserved (will be used for PE COFF offset)
+ */
+
+struct riscv_image_header {
+ u32 code0;
+ u32 code1;
+ u64 text_offset;
+ u64 image_size;
+ u64 flags;
+ u32 version;
+ u32 res1;
+ u64 res2;
+ u64 magic;
+ u32 res3;
+ u32 res4;
+};
+#endif /* __ASSEMBLY__ */
+#endif /* __ASM_IMAGE_H */
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 370c66ce187a..577893bb150d 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -19,9 +19,41 @@
#include <asm/thread_info.h>
#include <asm/page.h>
#include <asm/csr.h>
+#include <asm/image.h>
__INIT
ENTRY(_start)
+ /*
+ * Image header expected by Linux boot-loaders. The image header data
+ * structure is described in asm/image.h.
+ * Do not modify it without modifying the structure and all bootloaders
+ * that expects this header format!!
+ */
+ /* jump to start kernel */
+ j _start_kernel
+ /* reserved */
+ .word 0
+ .balign 8
+#if __riscv_xlen == 64
+ /* Image load offset(2MB) from start of RAM */
+ .dword 0x200000
+#else
+ /* Image load offset(4MB) from start of RAM */
+ .dword 0x400000
+#endif
+ /* Effective size of kernel image */
+ .dword _end - _start
+ .dword __HEAD_FLAGS
+ .word RISCV_HEADER_VERSION
+ .word 0
+ .dword 0
+ .asciz RISCV_IMAGE_MAGIC
+ .word 0
+ .balign 4
+ .word 0
+
+.global _start_kernel
+_start_kernel:
/* Mask all interrupts */
csrw CSR_SIE, zero
csrw CSR_SIP, zero
--
2.21.0
^ permalink raw reply related
* Re: [v3 PATCH] RISC-V: Add a PE/COFF compliant Image header.
From: Atish Patra @ 2019-05-24 0:16 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Linux Kernel Mailing List, Mark Rutland, Jonathan Corbet,
Albert Ou, Linux Doc Mailing List, Catalin Marinas, Anup Patel,
Will Deacon, Zong Li, Palmer Dabbelt, Tom Rini,
paul.walmsley@sifive.com, Nick Kossifidis,
linux-riscv@lists.infradead.org, marek.vasut@gmail.com,
linux-arm-kernel
In-Reply-To: <CAKv+Gu9VnjtgdkqfJJ1qQQ0W=z+uYN9Y-1n3Md3tV+d6a63wZA@mail.gmail.com>
On 5/23/19 2:09 PM, Ard Biesheuvel wrote:
> On Thu, 23 May 2019 at 19:35, Atish Patra <atish.patra@wdc.com> wrote:
>>
>> Currently, last stage boot loaders such as U-Boot can accept only
>> uImage which is an unnecessary additional step in automating boot flows.
>>
>> Add a PE/COFF compliant image header that boot loaders can parse and
>> directly load kernel flat Image. The existing booting methods will continue
>> to work as it is.
>>
>
> This statement does not make sense. This patch does not implement a
> single one of the various elements that make up a valid PE/COFF
> header.
>
Probably "compliant" is not the correct word. I meant to say that
PE/COFF header can be implemented in future with this header.
> The arm64 Image header has been designed in a way so that it can
> co-exist with a PE/COFF header in the same image, and this is what
Correct. "co-exist" is much better than "compliant"
Sorry for the choosing ambiguous words. I will update the commit text in
next version.
> this patch duplicates. The arm64 Image header has nothing to do with
> PE/COFF.
>
> A PE/COFF executable header consists of
> - the letters MZ at offset 0x0 (the MS-DOS header)
> - the offset to the PE header at offset 0x3c
> - the characters PE\0\0 at the offset mentioned above, followed by the
> standard COFF header fields
> - a PE32 or PE32+ (depending on the bitness) optional* header,
> followed by a set of section headers.
>
>
>
>
>> Another goal of this header is to support EFI stub for RISC-V in future.
>> EFI specification needs PE/COFF image header in the beginning of the kernel
>> image in order to load it as an EFI application. In order to support
>> EFI stub, code0 should be replaced with "MZ" magic string and res5(at
>> offset 0x3c) should point to the rest of the PE/COFF header (which will
>> be added during EFI support).
>>
>> This patch is based on ARM64 boot image header and provides an opprtunity
>> to combine both ARM64 & RISC-V image headers.
>>
>> Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
>>
>> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>>
>> ---
>> I have not sent out corresponding U-Boot patch as all the changes are
>> compatible with current u-boot support. Once, the kernel header format
>> is agreed upon, I will update the U-Boot patch.
>>
>> Changes from v2->v3
>> 1. Modified reserved fields to define a header version.
>> 2. Added header documentation.
>>
>> Changes from v1-v2:
>> 1. Added additional reserved elements to make it fully PE compatible.
>> ---
>> Documentation/riscv/boot-image-header.txt | 50 ++++++++++++++++++
>> arch/riscv/include/asm/image.h | 64 +++++++++++++++++++++++
>> arch/riscv/kernel/head.S | 32 ++++++++++++
>> 3 files changed, 146 insertions(+)
>> create mode 100644 Documentation/riscv/boot-image-header.txt
>> create mode 100644 arch/riscv/include/asm/image.h
>>
>> diff --git a/Documentation/riscv/boot-image-header.txt b/Documentation/riscv/boot-image-header.txt
>> new file mode 100644
>> index 000000000000..68abc2353cec
>> --- /dev/null
>> +++ b/Documentation/riscv/boot-image-header.txt
>> @@ -0,0 +1,50 @@
>> + Boot image header in RISC-V Linux
>> + =============================================
>> +
>> +Author: Atish Patra <atish.patra@wdc.com>
>> +Date : 20 May 2019
>> +
>> +This document only describes the boot image header details for RISC-V Linux.
>> +The complete booting guide will be available at Documentation/riscv/booting.txt.
>> +
>> +The following 64-byte header is present in decompressed Linux kernel image.
>> +
>> + u32 code0; /* Executable code */
>> + u32 code1; /* Executable code */
>> + u64 text_offset; /* Image load offset, little endian */
>> + u64 image_size; /* Effective Image size, little endian */
>> + u64 flags; /* kernel flags, little endian */
>> + u32 version; /* Version of this header */
>> + u32 res1 = 0; /* Reserved */
>> + u64 res2 = 0; /* Reserved */
>> + u64 magic = 0x5643534952; /* Magic number, little endian, "RISCV" */
>> + u32 res3; /* Reserved for additional RISC-V specific header */
>> + u32 res4; /* Reserved for PE COFF offset */
>> +
>> +This header format is compliant with PE/COFF header and largely inspired from
>> +ARM64 header. Thus, both ARM64 & RISC-V header can be combined into one common
>> +header in future.
>> +
>> +Notes:
>> +- This header can also be reused to support EFI stub for RISC-V in future. EFI
>> + specification needs PE/COFF image header in the beginning of the kernel image
>> + in order to load it as an EFI application. In order to support EFI stub,
>> + code0 should be replaced with "MZ" magic string and res5(at offset 0x3c) should
>> + point to the rest of the PE/COFF header.
>> +
>> +- version field indicate header version number.
>> + Bits 0:15 - Minor version
>> + Bits 16:31 - Major version
>> +
>> + This preserves compatibility across newer and older version of the header.
>> + The current version is defined as 0.1.
>> +
>> +- res3 is reserved for offset to any other additional fields. This makes the
>> + header extendible in future. One example would be to accommodate ISA
>> + extension for RISC-V in future. For current version, it is set to be zero.
>> +
>> +- In current header, the flag field has only one field.
>> + Bit 0: Kernel endianness. 1 if BE, 0 if LE.
>> +
>> +- Image size is mandatory for boot loader to load kernel image. Booting will
>> + fail otherwise.
>> diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
>> new file mode 100644
>> index 000000000000..61c9f20d2f19
>> --- /dev/null
>> +++ b/arch/riscv/include/asm/image.h
>> @@ -0,0 +1,64 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +
>> +#ifndef __ASM_IMAGE_H
>> +#define __ASM_IMAGE_H
>> +
>> +#define RISCV_IMAGE_MAGIC "RISCV"
>> +
>> +
>> +#define RISCV_IMAGE_FLAG_BE_SHIFT 0
>> +#define RISCV_IMAGE_FLAG_BE_MASK 0x1
>> +
>> +#define RISCV_IMAGE_FLAG_LE 0
>> +#define RISCV_IMAGE_FLAG_BE 1
>> +
>> +
>> +#ifdef CONFIG_CPU_BIG_ENDIAN
>> +#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_BE
>> +#else
>> +#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
>> +#endif
>> +
>> +#define __HEAD_FLAG(field) (__HEAD_FLAG_##field << \
>> + RISCV_IMAGE_FLAG_##field##_SHIFT)
>> +
>> +#define __HEAD_FLAGS (__HEAD_FLAG(BE))
>> +
>> +#define RISCV_HEADER_VERSION_MAJOR 0
>> +#define RISCV_HEADER_VERSION_MINOR 1
>> +
>> +#define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
>> + RISCV_HEADER_VERSION_MINOR)
>> +
>> +#ifndef __ASSEMBLY__
>> +/*
>> + * struct riscv_image_header - riscv kernel image header
>> + *
>> + * @code0: Executable code
>> + * @code1: Executable code
>> + * @text_offset: Image load offset
>> + * @image_size: Effective Image size
>> + * @flags: kernel flags
>> + * @version: version
>> + * @reserved: reserved
>> + * @reserved: reserved
>> + * @magic: Magic number
>> + * @reserved: reserved (will be used for additional RISC-V specific header)
>> + * @reserved: reserved (will be used for PE COFF offset)
>> + */
>> +
>> +struct riscv_image_header {
>> + u32 code0;
>> + u32 code1;
>> + u64 text_offset;
>> + u64 image_size;
>> + u64 flags;
>> + u32 version;
>> + u32 res1;
>> + u64 res2;
>> + u64 magic;
>> + u32 res3;
>> + u32 res4;
>> +};
>> +#endif /* __ASSEMBLY__ */
>> +#endif /* __ASM_IMAGE_H */
>> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
>> index 370c66ce187a..577893bb150d 100644
>> --- a/arch/riscv/kernel/head.S
>> +++ b/arch/riscv/kernel/head.S
>> @@ -19,9 +19,41 @@
>> #include <asm/thread_info.h>
>> #include <asm/page.h>
>> #include <asm/csr.h>
>> +#include <asm/image.h>
>>
>> __INIT
>> ENTRY(_start)
>> + /*
>> + * Image header expected by Linux boot-loaders. The image header data
>> + * structure is described in asm/image.h.
>> + * Do not modify it without modifying the structure and all bootloaders
>> + * that expects this header format!!
>> + */
>> + /* jump to start kernel */
>> + j _start_kernel
>> + /* reserved */
>> + .word 0
>> + .balign 8
>> +#if __riscv_xlen == 64
>> + /* Image load offset(2MB) from start of RAM */
>> + .dword 0x200000
>> +#else
>> + /* Image load offset(4MB) from start of RAM */
>> + .dword 0x400000
>> +#endif
>> + /* Effective size of kernel image */
>> + .dword _end - _start
>> + .dword __HEAD_FLAGS
>> + .word RISCV_HEADER_VERSION
>> + .word 0
>> + .dword 0
>> + .asciz RISCV_IMAGE_MAGIC
>> + .word 0
>> + .balign 4
>> + .word 0
>> +
>> +.global _start_kernel
>> +_start_kernel:
>> /* Mask all interrupts */
>> csrw CSR_SIE, zero
>> csrw CSR_SIP, zero
>> --
>> 2.21.0
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Regards,
Atish
^ permalink raw reply
* Re: [v3 PATCH] RISC-V: Add a PE/COFF compliant Image header.
From: Ard Biesheuvel @ 2019-05-23 21:09 UTC (permalink / raw)
To: Atish Patra
Cc: Linux Kernel Mailing List, Mark Rutland, Jonathan Corbet,
Albert Ou, Linux Doc Mailing List, Catalin Marinas, Anup Patel,
Will Deacon, Zong Li, Palmer Dabbelt, Tom Rini, paul.walmsley,
Nick Kossifidis, linux-riscv, marek.vasut, linux-arm-kernel
In-Reply-To: <20190523183516.583-1-atish.patra@wdc.com>
On Thu, 23 May 2019 at 19:35, Atish Patra <atish.patra@wdc.com> wrote:
>
> Currently, last stage boot loaders such as U-Boot can accept only
> uImage which is an unnecessary additional step in automating boot flows.
>
> Add a PE/COFF compliant image header that boot loaders can parse and
> directly load kernel flat Image. The existing booting methods will continue
> to work as it is.
>
This statement does not make sense. This patch does not implement a
single one of the various elements that make up a valid PE/COFF
header.
The arm64 Image header has been designed in a way so that it can
co-exist with a PE/COFF header in the same image, and this is what
this patch duplicates. The arm64 Image header has nothing to do with
PE/COFF.
A PE/COFF executable header consists of
- the letters MZ at offset 0x0 (the MS-DOS header)
- the offset to the PE header at offset 0x3c
- the characters PE\0\0 at the offset mentioned above, followed by the
standard COFF header fields
- a PE32 or PE32+ (depending on the bitness) optional* header,
followed by a set of section headers.
> Another goal of this header is to support EFI stub for RISC-V in future.
> EFI specification needs PE/COFF image header in the beginning of the kernel
> image in order to load it as an EFI application. In order to support
> EFI stub, code0 should be replaced with "MZ" magic string and res5(at
> offset 0x3c) should point to the rest of the PE/COFF header (which will
> be added during EFI support).
>
> This patch is based on ARM64 boot image header and provides an opprtunity
> to combine both ARM64 & RISC-V image headers.
>
> Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
>
> Signed-off-by: Atish Patra <atish.patra@wdc.com>
>
> ---
> I have not sent out corresponding U-Boot patch as all the changes are
> compatible with current u-boot support. Once, the kernel header format
> is agreed upon, I will update the U-Boot patch.
>
> Changes from v2->v3
> 1. Modified reserved fields to define a header version.
> 2. Added header documentation.
>
> Changes from v1-v2:
> 1. Added additional reserved elements to make it fully PE compatible.
> ---
> Documentation/riscv/boot-image-header.txt | 50 ++++++++++++++++++
> arch/riscv/include/asm/image.h | 64 +++++++++++++++++++++++
> arch/riscv/kernel/head.S | 32 ++++++++++++
> 3 files changed, 146 insertions(+)
> create mode 100644 Documentation/riscv/boot-image-header.txt
> create mode 100644 arch/riscv/include/asm/image.h
>
> diff --git a/Documentation/riscv/boot-image-header.txt b/Documentation/riscv/boot-image-header.txt
> new file mode 100644
> index 000000000000..68abc2353cec
> --- /dev/null
> +++ b/Documentation/riscv/boot-image-header.txt
> @@ -0,0 +1,50 @@
> + Boot image header in RISC-V Linux
> + =============================================
> +
> +Author: Atish Patra <atish.patra@wdc.com>
> +Date : 20 May 2019
> +
> +This document only describes the boot image header details for RISC-V Linux.
> +The complete booting guide will be available at Documentation/riscv/booting.txt.
> +
> +The following 64-byte header is present in decompressed Linux kernel image.
> +
> + u32 code0; /* Executable code */
> + u32 code1; /* Executable code */
> + u64 text_offset; /* Image load offset, little endian */
> + u64 image_size; /* Effective Image size, little endian */
> + u64 flags; /* kernel flags, little endian */
> + u32 version; /* Version of this header */
> + u32 res1 = 0; /* Reserved */
> + u64 res2 = 0; /* Reserved */
> + u64 magic = 0x5643534952; /* Magic number, little endian, "RISCV" */
> + u32 res3; /* Reserved for additional RISC-V specific header */
> + u32 res4; /* Reserved for PE COFF offset */
> +
> +This header format is compliant with PE/COFF header and largely inspired from
> +ARM64 header. Thus, both ARM64 & RISC-V header can be combined into one common
> +header in future.
> +
> +Notes:
> +- This header can also be reused to support EFI stub for RISC-V in future. EFI
> + specification needs PE/COFF image header in the beginning of the kernel image
> + in order to load it as an EFI application. In order to support EFI stub,
> + code0 should be replaced with "MZ" magic string and res5(at offset 0x3c) should
> + point to the rest of the PE/COFF header.
> +
> +- version field indicate header version number.
> + Bits 0:15 - Minor version
> + Bits 16:31 - Major version
> +
> + This preserves compatibility across newer and older version of the header.
> + The current version is defined as 0.1.
> +
> +- res3 is reserved for offset to any other additional fields. This makes the
> + header extendible in future. One example would be to accommodate ISA
> + extension for RISC-V in future. For current version, it is set to be zero.
> +
> +- In current header, the flag field has only one field.
> + Bit 0: Kernel endianness. 1 if BE, 0 if LE.
> +
> +- Image size is mandatory for boot loader to load kernel image. Booting will
> + fail otherwise.
> diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
> new file mode 100644
> index 000000000000..61c9f20d2f19
> --- /dev/null
> +++ b/arch/riscv/include/asm/image.h
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __ASM_IMAGE_H
> +#define __ASM_IMAGE_H
> +
> +#define RISCV_IMAGE_MAGIC "RISCV"
> +
> +
> +#define RISCV_IMAGE_FLAG_BE_SHIFT 0
> +#define RISCV_IMAGE_FLAG_BE_MASK 0x1
> +
> +#define RISCV_IMAGE_FLAG_LE 0
> +#define RISCV_IMAGE_FLAG_BE 1
> +
> +
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> +#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_BE
> +#else
> +#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
> +#endif
> +
> +#define __HEAD_FLAG(field) (__HEAD_FLAG_##field << \
> + RISCV_IMAGE_FLAG_##field##_SHIFT)
> +
> +#define __HEAD_FLAGS (__HEAD_FLAG(BE))
> +
> +#define RISCV_HEADER_VERSION_MAJOR 0
> +#define RISCV_HEADER_VERSION_MINOR 1
> +
> +#define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
> + RISCV_HEADER_VERSION_MINOR)
> +
> +#ifndef __ASSEMBLY__
> +/*
> + * struct riscv_image_header - riscv kernel image header
> + *
> + * @code0: Executable code
> + * @code1: Executable code
> + * @text_offset: Image load offset
> + * @image_size: Effective Image size
> + * @flags: kernel flags
> + * @version: version
> + * @reserved: reserved
> + * @reserved: reserved
> + * @magic: Magic number
> + * @reserved: reserved (will be used for additional RISC-V specific header)
> + * @reserved: reserved (will be used for PE COFF offset)
> + */
> +
> +struct riscv_image_header {
> + u32 code0;
> + u32 code1;
> + u64 text_offset;
> + u64 image_size;
> + u64 flags;
> + u32 version;
> + u32 res1;
> + u64 res2;
> + u64 magic;
> + u32 res3;
> + u32 res4;
> +};
> +#endif /* __ASSEMBLY__ */
> +#endif /* __ASM_IMAGE_H */
> diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
> index 370c66ce187a..577893bb150d 100644
> --- a/arch/riscv/kernel/head.S
> +++ b/arch/riscv/kernel/head.S
> @@ -19,9 +19,41 @@
> #include <asm/thread_info.h>
> #include <asm/page.h>
> #include <asm/csr.h>
> +#include <asm/image.h>
>
> __INIT
> ENTRY(_start)
> + /*
> + * Image header expected by Linux boot-loaders. The image header data
> + * structure is described in asm/image.h.
> + * Do not modify it without modifying the structure and all bootloaders
> + * that expects this header format!!
> + */
> + /* jump to start kernel */
> + j _start_kernel
> + /* reserved */
> + .word 0
> + .balign 8
> +#if __riscv_xlen == 64
> + /* Image load offset(2MB) from start of RAM */
> + .dword 0x200000
> +#else
> + /* Image load offset(4MB) from start of RAM */
> + .dword 0x400000
> +#endif
> + /* Effective size of kernel image */
> + .dword _end - _start
> + .dword __HEAD_FLAGS
> + .word RISCV_HEADER_VERSION
> + .word 0
> + .dword 0
> + .asciz RISCV_IMAGE_MAGIC
> + .word 0
> + .balign 4
> + .word 0
> +
> +.global _start_kernel
> +_start_kernel:
> /* Mask all interrupts */
> csrw CSR_SIE, zero
> csrw CSR_SIP, zero
> --
> 2.21.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Peter Oskolkov @ 2019-05-23 21:01 UTC (permalink / raw)
To: Dave Chiluk
Cc: Peter Zijlstra, Ingo Molnar, cgroups, Linux Kernel Mailing List,
Brendan Gregg, Kyle Anderson, Gabriel Munos, John Hammond,
Cong Wang, Jonathan Corbet, linux-doc
In-Reply-To: <1558637087-20283-2-git-send-email-chiluk+linux@indeed.com>
On Thu, May 23, 2019 at 11:44 AM Dave Chiluk <chiluk+linux@indeed.com> wrote:
>
> It has been observed, that highly-threaded, non-cpu-bound applications
> running under cpu.cfs_quota_us constraints can hit a high percentage of
> periods throttled while simultaneously not consuming the allocated
> amount of quota. This use case is typical of user-interactive non-cpu
> bound applications, such as those running in kubernetes or mesos when
> run on multiple cpu cores.
>
> This has been root caused to threads being allocated per cpu bandwidth
> slices, and then not fully using that slice within the period. At which
> point the slice and quota expires. This expiration of unused slice
> results in applications not being able to utilize the quota for which
> they are allocated.
>
> The expiration of per-cpu slices was recently fixed by
> 'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
> condition")'. Prior to that it appears that this has been broken since
> at least 'commit 51f2176d74ac ("sched/fair: Fix unlocked reads of some
> cfs_b->quota/period")' which was introduced in v3.16-rc1 in 2014. That
> added the following conditional which resulted in slices never being
> expired.
>
> if (cfs_rq->runtime_expires != cfs_b->runtime_expires) {
> /* extend local deadline, drift is bounded above by 2 ticks */
> cfs_rq->runtime_expires += TICK_NSEC;
>
> Because this was broken for nearly 5 years, and has recently been fixed
> and is now being noticed by many users running kubernetes
> (https://github.com/kubernetes/kubernetes/issues/67577) it is my opinion
> that the mechanisms around expiring runtime should be removed
> altogether.
>
> This allows only per-cpu slices to live longer than the period boundary.
> This allows threads on runqueues that do not use much CPU to continue to
> use their remaining slice over a longer period of time than
> cpu.cfs_period_us. However, this helps prevents the above condition of
> hitting throttling while also not fully utilizing your cpu quota.
>
> This theoretically allows a machine to use slightly more than it's
> allotted quota in some periods. This overflow would be bounded by the
> remaining per-cpu slice that was left un-used in the previous period.
> For CPU bound tasks this will change nothing, as they should
> theoretically fully utilize all of their quota and slices in each
> period. For user-interactive tasks as described above this provides a
> much better user/application experience as their cpu utilization will
> more closely match the amount they requested when they hit throttling.
>
> This greatly improves performance of high-thread-count, non-cpu bound
> applications with low cfs_quota_us allocation on high-core-count
> machines. In the case of an artificial testcase, this performance
> discrepancy has been observed to be almost 30x performance improvement,
> while still maintaining correct cpu quota restrictions albeit over
> longer time intervals than cpu.cfs_period_us.
If the machine runs at/close to capacity, won't the overallocation
of the quota to bursty tasks necessarily negatively impact every other
task? Should the "unused" quota be available only on idle CPUs?
(Or maybe this is the behavior achieved here, and only the comment and
the commit message should be fixed...)
> That testcase is
> available at https://github.com/indeedeng/fibtest.
>
> Fixes: 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition")
> Signed-off-by: Dave Chiluk <chiluk+linux@indeed.com>
> ---
> Documentation/scheduler/sched-bwc.txt | 29 +++++++++++---
> kernel/sched/fair.c | 71 +++--------------------------------
> kernel/sched/sched.h | 4 --
> 3 files changed, 29 insertions(+), 75 deletions(-)
>
> diff --git a/Documentation/scheduler/sched-bwc.txt b/Documentation/scheduler/sched-bwc.txt
> index f6b1873..4ded8ae 100644
> --- a/Documentation/scheduler/sched-bwc.txt
> +++ b/Documentation/scheduler/sched-bwc.txt
> @@ -8,16 +8,33 @@ CFS bandwidth control is a CONFIG_FAIR_GROUP_SCHED extension which allows the
> specification of the maximum CPU bandwidth available to a group or hierarchy.
>
> The bandwidth allowed for a group is specified using a quota and period. Within
> -each given "period" (microseconds), a group is allowed to consume only up to
> -"quota" microseconds of CPU time. When the CPU bandwidth consumption of a
> -group exceeds this limit (for that period), the tasks belonging to its
> -hierarchy will be throttled and are not allowed to run again until the next
> -period.
> +each given "period" (microseconds), a task group is allocated up to "quota"
> +microseconds of CPU time. When the CPU bandwidth consumption of a group
> +exceeds this limit (for that period), the tasks belonging to its hierarchy will
> +be throttled and are not allowed to run again until the next period.
>
> A group's unused runtime is globally tracked, being refreshed with quota units
> above at each period boundary. As threads consume this bandwidth it is
> transferred to cpu-local "silos" on a demand basis. The amount transferred
> -within each of these updates is tunable and described as the "slice".
> +within each of these updates is tunable and described as the "slice". Slices
> +that are allocated to cpu-local silos do not expire at the end of the period,
> +but unallocated quota does. This doesn't affect cpu-bound applications as they
> +by definition consume all of their bandwidth in each each period.
> +
> +However for highly-threaded user-interactive/non-cpu bound applications this
> +non-expiration nuance allows applications to burst past their quota limits
> +equal to the amount of unused slice per cpu that the task group is running on.
> +This slight burst requires that quota had gone unused in previous periods.
> +Additionally this burst amount is limited to the size of a slice for every cpu
> +a task group is run on. As a result, this mechanism still strictly limits the
> +task group to quota average usage over a longer time windows. This provides
> +better more predictable user experience for highly threaded applications with
> +small quota limits on high core count machines. It also eliminates the
> +propensity to throttle these applications while simultanously using less than
> +quota amounts of cpu. Another way to say this, is that by allowing the unused
> +portion of a slice to be used in following periods we have decreased the
> +possibility of wasting unused quota on cpu-local silos that don't need much cpu
> +time.
>
> Management
> ----------
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index f35930f..a675c69 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -4295,8 +4295,6 @@ void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
>
> now = sched_clock_cpu(smp_processor_id());
> cfs_b->runtime = cfs_b->quota;
> - cfs_b->runtime_expires = now + ktime_to_ns(cfs_b->period);
> - cfs_b->expires_seq++;
> }
>
> static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
> @@ -4318,8 +4316,7 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> {
> struct task_group *tg = cfs_rq->tg;
> struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
> - u64 amount = 0, min_amount, expires;
> - int expires_seq;
> + u64 amount = 0, min_amount;
>
> /* note: this is a positive sum as runtime_remaining <= 0 */
> min_amount = sched_cfs_bandwidth_slice() - cfs_rq->runtime_remaining;
> @@ -4336,61 +4333,17 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> cfs_b->idle = 0;
> }
> }
> - expires_seq = cfs_b->expires_seq;
> - expires = cfs_b->runtime_expires;
> raw_spin_unlock(&cfs_b->lock);
>
> cfs_rq->runtime_remaining += amount;
> - /*
> - * we may have advanced our local expiration to account for allowed
> - * spread between our sched_clock and the one on which runtime was
> - * issued.
> - */
> - if (cfs_rq->expires_seq != expires_seq) {
> - cfs_rq->expires_seq = expires_seq;
> - cfs_rq->runtime_expires = expires;
> - }
>
> return cfs_rq->runtime_remaining > 0;
> }
>
> -/*
> - * Note: This depends on the synchronization provided by sched_clock and the
> - * fact that rq->clock snapshots this value.
> - */
> -static void expire_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> -{
> - struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);
> -
> - /* if the deadline is ahead of our clock, nothing to do */
> - if (likely((s64)(rq_clock(rq_of(cfs_rq)) - cfs_rq->runtime_expires) < 0))
> - return;
> -
> - if (cfs_rq->runtime_remaining < 0)
> - return;
> -
> - /*
> - * If the local deadline has passed we have to consider the
> - * possibility that our sched_clock is 'fast' and the global deadline
> - * has not truly expired.
> - *
> - * Fortunately we can check determine whether this the case by checking
> - * whether the global deadline(cfs_b->expires_seq) has advanced.
> - */
> - if (cfs_rq->expires_seq == cfs_b->expires_seq) {
> - /* extend local deadline, drift is bounded above by 2 ticks */
> - cfs_rq->runtime_expires += TICK_NSEC;
> - } else {
> - /* global deadline is ahead, expiration has passed */
> - cfs_rq->runtime_remaining = 0;
> - }
> -}
> -
> static void __account_cfs_rq_runtime(struct cfs_rq *cfs_rq, u64 delta_exec)
> {
> /* dock delta_exec before expiring quota (as it could span periods) */
> cfs_rq->runtime_remaining -= delta_exec;
> - expire_cfs_rq_runtime(cfs_rq);
>
> if (likely(cfs_rq->runtime_remaining > 0))
> return;
> @@ -4581,8 +4534,7 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
> resched_curr(rq);
> }
>
> -static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
> - u64 remaining, u64 expires)
> +static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b, u64 remaining)
> {
> struct cfs_rq *cfs_rq;
> u64 runtime;
> @@ -4604,7 +4556,6 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
> remaining -= runtime;
>
> cfs_rq->runtime_remaining += runtime;
> - cfs_rq->runtime_expires = expires;
>
> /* we check whether we're throttled above */
> if (cfs_rq->runtime_remaining > 0)
> @@ -4629,7 +4580,7 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
> */
> static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, unsigned long flags)
> {
> - u64 runtime, runtime_expires;
> + u64 runtime;
> int throttled;
>
> /* no need to continue the timer with no bandwidth constraint */
> @@ -4657,8 +4608,6 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
> /* account preceding periods in which throttling occurred */
> cfs_b->nr_throttled += overrun;
>
> - runtime_expires = cfs_b->runtime_expires;
> -
> /*
> * This check is repeated as we are holding onto the new bandwidth while
> * we unthrottle. This can potentially race with an unthrottled group
> @@ -4671,8 +4620,7 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
> cfs_b->distribute_running = 1;
> raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> /* we can't nest cfs_b->lock while distributing bandwidth */
> - runtime = distribute_cfs_runtime(cfs_b, runtime,
> - runtime_expires);
> + runtime = distribute_cfs_runtime(cfs_b, runtime);
> raw_spin_lock_irqsave(&cfs_b->lock, flags);
>
> cfs_b->distribute_running = 0;
> @@ -4749,8 +4697,7 @@ static void __return_cfs_rq_runtime(struct cfs_rq *cfs_rq)
> return;
>
> raw_spin_lock(&cfs_b->lock);
> - if (cfs_b->quota != RUNTIME_INF &&
> - cfs_rq->runtime_expires == cfs_b->runtime_expires) {
> + if (cfs_b->quota != RUNTIME_INF) {
> cfs_b->runtime += slack_runtime;
>
> /* we are under rq->lock, defer unthrottling using a timer */
> @@ -4783,7 +4730,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> {
> u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
> unsigned long flags;
> - u64 expires;
>
> /* confirm we're still not at a refresh boundary */
> raw_spin_lock_irqsave(&cfs_b->lock, flags);
> @@ -4800,7 +4746,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> if (cfs_b->quota != RUNTIME_INF && cfs_b->runtime > slice)
> runtime = cfs_b->runtime;
>
> - expires = cfs_b->runtime_expires;
> if (runtime)
> cfs_b->distribute_running = 1;
>
> @@ -4809,11 +4754,9 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> if (!runtime)
> return;
>
> - runtime = distribute_cfs_runtime(cfs_b, runtime, expires);
> + runtime = distribute_cfs_runtime(cfs_b, runtime);
>
> raw_spin_lock_irqsave(&cfs_b->lock, flags);
> - if (expires == cfs_b->runtime_expires)
> - lsub_positive(&cfs_b->runtime, runtime);
> cfs_b->distribute_running = 0;
> raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> }
> @@ -4969,8 +4912,6 @@ void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
>
> cfs_b->period_active = 1;
> overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
> - cfs_b->runtime_expires += (overrun + 1) * ktime_to_ns(cfs_b->period);
> - cfs_b->expires_seq++;
> hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
> }
>
> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> index b52ed1a..0c0ed23 100644
> --- a/kernel/sched/sched.h
> +++ b/kernel/sched/sched.h
> @@ -341,8 +341,6 @@ struct cfs_bandwidth {
> u64 quota;
> u64 runtime;
> s64 hierarchical_quota;
> - u64 runtime_expires;
> - int expires_seq;
>
> short idle;
> short period_active;
> @@ -562,8 +560,6 @@ struct cfs_rq {
>
> #ifdef CONFIG_CFS_BANDWIDTH
> int runtime_enabled;
> - int expires_seq;
> - u64 runtime_expires;
> s64 runtime_remaining;
>
> u64 throttled_clock;
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH v3] doc: kernel-parameters.txt: fix documentation of nmi_watchdog parameter
From: Jonathan Corbet @ 2019-05-23 19:01 UTC (permalink / raw)
To: Steven Rostedt
Cc: Zhenzhong Duan, linux-kernel, linux-doc, paulmck, josh,
mathieu.desnoyers, jiangshanlai, joel, tglx, mingo, gregkh,
keescook, srinivas.eeda
In-Reply-To: <20190523143703.2fb71f71@gandalf.local.home>
On Thu, 23 May 2019 14:37:03 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Tue, 21 May 2019 10:32:08 +0800
> Zhenzhong Duan <zhenzhong.duan@oracle.com> wrote:
>
> > The default behavior of hardlockup depends on the config of
> > CONFIG_BOOTPARAM_HARDLOCKUP_PANIC.
> >
>
> Jon,
>
> You want to take this in your tree?
Sure, I'll take it.
Thanks,
jon
^ permalink raw reply
* [PATCH v2 1/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Dave Chiluk @ 2019-05-23 18:44 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, cgroups, linux-kernel, Brendan Gregg,
Kyle Anderson, Gabriel Munos, John Hammond, Cong Wang,
Jonathan Corbet, linux-doc
In-Reply-To: <1558637087-20283-1-git-send-email-chiluk+linux@indeed.com>
It has been observed, that highly-threaded, non-cpu-bound applications
running under cpu.cfs_quota_us constraints can hit a high percentage of
periods throttled while simultaneously not consuming the allocated
amount of quota. This use case is typical of user-interactive non-cpu
bound applications, such as those running in kubernetes or mesos when
run on multiple cpu cores.
This has been root caused to threads being allocated per cpu bandwidth
slices, and then not fully using that slice within the period. At which
point the slice and quota expires. This expiration of unused slice
results in applications not being able to utilize the quota for which
they are allocated.
The expiration of per-cpu slices was recently fixed by
'commit 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift
condition")'. Prior to that it appears that this has been broken since
at least 'commit 51f2176d74ac ("sched/fair: Fix unlocked reads of some
cfs_b->quota/period")' which was introduced in v3.16-rc1 in 2014. That
added the following conditional which resulted in slices never being
expired.
if (cfs_rq->runtime_expires != cfs_b->runtime_expires) {
/* extend local deadline, drift is bounded above by 2 ticks */
cfs_rq->runtime_expires += TICK_NSEC;
Because this was broken for nearly 5 years, and has recently been fixed
and is now being noticed by many users running kubernetes
(https://github.com/kubernetes/kubernetes/issues/67577) it is my opinion
that the mechanisms around expiring runtime should be removed
altogether.
This allows only per-cpu slices to live longer than the period boundary.
This allows threads on runqueues that do not use much CPU to continue to
use their remaining slice over a longer period of time than
cpu.cfs_period_us. However, this helps prevents the above condition of
hitting throttling while also not fully utilizing your cpu quota.
This theoretically allows a machine to use slightly more than it's
allotted quota in some periods. This overflow would be bounded by the
remaining per-cpu slice that was left un-used in the previous period.
For CPU bound tasks this will change nothing, as they should
theoretically fully utilize all of their quota and slices in each
period. For user-interactive tasks as described above this provides a
much better user/application experience as their cpu utilization will
more closely match the amount they requested when they hit throttling.
This greatly improves performance of high-thread-count, non-cpu bound
applications with low cfs_quota_us allocation on high-core-count
machines. In the case of an artificial testcase, this performance
discrepancy has been observed to be almost 30x performance improvement,
while still maintaining correct cpu quota restrictions albeit over
longer time intervals than cpu.cfs_period_us. That testcase is
available at https://github.com/indeedeng/fibtest.
Fixes: 512ac999d275 ("sched/fair: Fix bandwidth timer clock drift condition")
Signed-off-by: Dave Chiluk <chiluk+linux@indeed.com>
---
Documentation/scheduler/sched-bwc.txt | 29 +++++++++++---
kernel/sched/fair.c | 71 +++--------------------------------
kernel/sched/sched.h | 4 --
3 files changed, 29 insertions(+), 75 deletions(-)
diff --git a/Documentation/scheduler/sched-bwc.txt b/Documentation/scheduler/sched-bwc.txt
index f6b1873..4ded8ae 100644
--- a/Documentation/scheduler/sched-bwc.txt
+++ b/Documentation/scheduler/sched-bwc.txt
@@ -8,16 +8,33 @@ CFS bandwidth control is a CONFIG_FAIR_GROUP_SCHED extension which allows the
specification of the maximum CPU bandwidth available to a group or hierarchy.
The bandwidth allowed for a group is specified using a quota and period. Within
-each given "period" (microseconds), a group is allowed to consume only up to
-"quota" microseconds of CPU time. When the CPU bandwidth consumption of a
-group exceeds this limit (for that period), the tasks belonging to its
-hierarchy will be throttled and are not allowed to run again until the next
-period.
+each given "period" (microseconds), a task group is allocated up to "quota"
+microseconds of CPU time. When the CPU bandwidth consumption of a group
+exceeds this limit (for that period), the tasks belonging to its hierarchy will
+be throttled and are not allowed to run again until the next period.
A group's unused runtime is globally tracked, being refreshed with quota units
above at each period boundary. As threads consume this bandwidth it is
transferred to cpu-local "silos" on a demand basis. The amount transferred
-within each of these updates is tunable and described as the "slice".
+within each of these updates is tunable and described as the "slice". Slices
+that are allocated to cpu-local silos do not expire at the end of the period,
+but unallocated quota does. This doesn't affect cpu-bound applications as they
+by definition consume all of their bandwidth in each each period.
+
+However for highly-threaded user-interactive/non-cpu bound applications this
+non-expiration nuance allows applications to burst past their quota limits
+equal to the amount of unused slice per cpu that the task group is running on.
+This slight burst requires that quota had gone unused in previous periods.
+Additionally this burst amount is limited to the size of a slice for every cpu
+a task group is run on. As a result, this mechanism still strictly limits the
+task group to quota average usage over a longer time windows. This provides
+better more predictable user experience for highly threaded applications with
+small quota limits on high core count machines. It also eliminates the
+propensity to throttle these applications while simultanously using less than
+quota amounts of cpu. Another way to say this, is that by allowing the unused
+portion of a slice to be used in following periods we have decreased the
+possibility of wasting unused quota on cpu-local silos that don't need much cpu
+time.
Management
----------
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index f35930f..a675c69 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -4295,8 +4295,6 @@ void __refill_cfs_bandwidth_runtime(struct cfs_bandwidth *cfs_b)
now = sched_clock_cpu(smp_processor_id());
cfs_b->runtime = cfs_b->quota;
- cfs_b->runtime_expires = now + ktime_to_ns(cfs_b->period);
- cfs_b->expires_seq++;
}
static inline struct cfs_bandwidth *tg_cfs_bandwidth(struct task_group *tg)
@@ -4318,8 +4316,7 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
{
struct task_group *tg = cfs_rq->tg;
struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(tg);
- u64 amount = 0, min_amount, expires;
- int expires_seq;
+ u64 amount = 0, min_amount;
/* note: this is a positive sum as runtime_remaining <= 0 */
min_amount = sched_cfs_bandwidth_slice() - cfs_rq->runtime_remaining;
@@ -4336,61 +4333,17 @@ static int assign_cfs_rq_runtime(struct cfs_rq *cfs_rq)
cfs_b->idle = 0;
}
}
- expires_seq = cfs_b->expires_seq;
- expires = cfs_b->runtime_expires;
raw_spin_unlock(&cfs_b->lock);
cfs_rq->runtime_remaining += amount;
- /*
- * we may have advanced our local expiration to account for allowed
- * spread between our sched_clock and the one on which runtime was
- * issued.
- */
- if (cfs_rq->expires_seq != expires_seq) {
- cfs_rq->expires_seq = expires_seq;
- cfs_rq->runtime_expires = expires;
- }
return cfs_rq->runtime_remaining > 0;
}
-/*
- * Note: This depends on the synchronization provided by sched_clock and the
- * fact that rq->clock snapshots this value.
- */
-static void expire_cfs_rq_runtime(struct cfs_rq *cfs_rq)
-{
- struct cfs_bandwidth *cfs_b = tg_cfs_bandwidth(cfs_rq->tg);
-
- /* if the deadline is ahead of our clock, nothing to do */
- if (likely((s64)(rq_clock(rq_of(cfs_rq)) - cfs_rq->runtime_expires) < 0))
- return;
-
- if (cfs_rq->runtime_remaining < 0)
- return;
-
- /*
- * If the local deadline has passed we have to consider the
- * possibility that our sched_clock is 'fast' and the global deadline
- * has not truly expired.
- *
- * Fortunately we can check determine whether this the case by checking
- * whether the global deadline(cfs_b->expires_seq) has advanced.
- */
- if (cfs_rq->expires_seq == cfs_b->expires_seq) {
- /* extend local deadline, drift is bounded above by 2 ticks */
- cfs_rq->runtime_expires += TICK_NSEC;
- } else {
- /* global deadline is ahead, expiration has passed */
- cfs_rq->runtime_remaining = 0;
- }
-}
-
static void __account_cfs_rq_runtime(struct cfs_rq *cfs_rq, u64 delta_exec)
{
/* dock delta_exec before expiring quota (as it could span periods) */
cfs_rq->runtime_remaining -= delta_exec;
- expire_cfs_rq_runtime(cfs_rq);
if (likely(cfs_rq->runtime_remaining > 0))
return;
@@ -4581,8 +4534,7 @@ void unthrottle_cfs_rq(struct cfs_rq *cfs_rq)
resched_curr(rq);
}
-static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
- u64 remaining, u64 expires)
+static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b, u64 remaining)
{
struct cfs_rq *cfs_rq;
u64 runtime;
@@ -4604,7 +4556,6 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
remaining -= runtime;
cfs_rq->runtime_remaining += runtime;
- cfs_rq->runtime_expires = expires;
/* we check whether we're throttled above */
if (cfs_rq->runtime_remaining > 0)
@@ -4629,7 +4580,7 @@ static u64 distribute_cfs_runtime(struct cfs_bandwidth *cfs_b,
*/
static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, unsigned long flags)
{
- u64 runtime, runtime_expires;
+ u64 runtime;
int throttled;
/* no need to continue the timer with no bandwidth constraint */
@@ -4657,8 +4608,6 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
/* account preceding periods in which throttling occurred */
cfs_b->nr_throttled += overrun;
- runtime_expires = cfs_b->runtime_expires;
-
/*
* This check is repeated as we are holding onto the new bandwidth while
* we unthrottle. This can potentially race with an unthrottled group
@@ -4671,8 +4620,7 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, u
cfs_b->distribute_running = 1;
raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
/* we can't nest cfs_b->lock while distributing bandwidth */
- runtime = distribute_cfs_runtime(cfs_b, runtime,
- runtime_expires);
+ runtime = distribute_cfs_runtime(cfs_b, runtime);
raw_spin_lock_irqsave(&cfs_b->lock, flags);
cfs_b->distribute_running = 0;
@@ -4749,8 +4697,7 @@ static void __return_cfs_rq_runtime(struct cfs_rq *cfs_rq)
return;
raw_spin_lock(&cfs_b->lock);
- if (cfs_b->quota != RUNTIME_INF &&
- cfs_rq->runtime_expires == cfs_b->runtime_expires) {
+ if (cfs_b->quota != RUNTIME_INF) {
cfs_b->runtime += slack_runtime;
/* we are under rq->lock, defer unthrottling using a timer */
@@ -4783,7 +4730,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
{
u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
unsigned long flags;
- u64 expires;
/* confirm we're still not at a refresh boundary */
raw_spin_lock_irqsave(&cfs_b->lock, flags);
@@ -4800,7 +4746,6 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
if (cfs_b->quota != RUNTIME_INF && cfs_b->runtime > slice)
runtime = cfs_b->runtime;
- expires = cfs_b->runtime_expires;
if (runtime)
cfs_b->distribute_running = 1;
@@ -4809,11 +4754,9 @@ static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
if (!runtime)
return;
- runtime = distribute_cfs_runtime(cfs_b, runtime, expires);
+ runtime = distribute_cfs_runtime(cfs_b, runtime);
raw_spin_lock_irqsave(&cfs_b->lock, flags);
- if (expires == cfs_b->runtime_expires)
- lsub_positive(&cfs_b->runtime, runtime);
cfs_b->distribute_running = 0;
raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
}
@@ -4969,8 +4912,6 @@ void start_cfs_bandwidth(struct cfs_bandwidth *cfs_b)
cfs_b->period_active = 1;
overrun = hrtimer_forward_now(&cfs_b->period_timer, cfs_b->period);
- cfs_b->runtime_expires += (overrun + 1) * ktime_to_ns(cfs_b->period);
- cfs_b->expires_seq++;
hrtimer_start_expires(&cfs_b->period_timer, HRTIMER_MODE_ABS_PINNED);
}
diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
index b52ed1a..0c0ed23 100644
--- a/kernel/sched/sched.h
+++ b/kernel/sched/sched.h
@@ -341,8 +341,6 @@ struct cfs_bandwidth {
u64 quota;
u64 runtime;
s64 hierarchical_quota;
- u64 runtime_expires;
- int expires_seq;
short idle;
short period_active;
@@ -562,8 +560,6 @@ struct cfs_rq {
#ifdef CONFIG_CFS_BANDWIDTH
int runtime_enabled;
- int expires_seq;
- u64 runtime_expires;
s64 runtime_remaining;
u64 throttled_clock;
--
1.8.3.1
^ permalink raw reply related
* [PATCH v2 0/1] sched/fair: Fix low cpu usage with high throttling by removing expiration of cpu-local slices
From: Dave Chiluk @ 2019-05-23 18:44 UTC (permalink / raw)
To: Peter Zijlstra, Ingo Molnar, cgroups, linux-kernel, Brendan Gregg,
Kyle Anderson, Gabriel Munos, John Hammond, Cong Wang,
Jonathan Corbet, linux-doc
In-Reply-To: <1558121424-2914-1-git-send-email-chiluk+linux@indeed.com>
Changelog v2
- Fixed some checkpatch errors in the commit message.
^ permalink raw reply
* Re: [PATCH v3] doc: kernel-parameters.txt: fix documentation of nmi_watchdog parameter
From: Steven Rostedt @ 2019-05-23 18:37 UTC (permalink / raw)
To: Zhenzhong Duan, corbet
Cc: linux-kernel, linux-doc, paulmck, josh, mathieu.desnoyers,
jiangshanlai, joel, tglx, mingo, gregkh, keescook, srinivas.eeda
In-Reply-To: <1558405928-29449-1-git-send-email-zhenzhong.duan@oracle.com>
On Tue, 21 May 2019 10:32:08 +0800
Zhenzhong Duan <zhenzhong.duan@oracle.com> wrote:
> The default behavior of hardlockup depends on the config of
> CONFIG_BOOTPARAM_HARDLOCKUP_PANIC.
>
Jon,
You want to take this in your tree?
-- Steve
> Fix the description of nmi_watchdog to make it clear.
>
> Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> Acked-by: Ingo Molnar <mingo@kernel.org>
> Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-doc@vger.kernel.org
> ---
> v3: add Suggested-by and Acked-by
> v2: fix description using words suggested by Steven Rostedt
>
> Documentation/admin-guide/kernel-parameters.txt | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 08df588..b9d4358 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -2805,8 +2805,9 @@
> 0 - turn hardlockup detector in nmi_watchdog off
> 1 - turn hardlockup detector in nmi_watchdog on
> When panic is specified, panic when an NMI watchdog
> - timeout occurs (or 'nopanic' to override the opposite
> - default). To disable both hard and soft lockup detectors,
> + timeout occurs (or 'nopanic' to not panic on an NMI
> + watchdog, if CONFIG_BOOTPARAM_HARDLOCKUP_PANIC is set)
> + To disable both hard and soft lockup detectors,
> please see 'nowatchdog'.
> This is useful when you use a panic=... timeout and
> need the box quickly up again.
^ permalink raw reply
* Re: [PATCH] Fix comments with paths pointing to renamed or moved files.
From: Steven Rostedt @ 2019-05-23 18:35 UTC (permalink / raw)
To: Lukas Prause
Cc: lizefan, tj, hannes, peterz, mingo, will.deacon, rjw, len.brown,
pavel, linux-doc, linux-kernel, cgroups, linux-pm, Lennart Glauer
In-Reply-To: <20190521155430.14941-1-langspielplatte@black-mesa.xyz>
On Tue, 21 May 2019 17:54:30 +0200
Lukas Prause <langspielplatte@black-mesa.xyz> wrote:
> This patch corrects renamed or moved paths in comments.
>
> Signed-off-by: Lukas Prause <langspielplatte@black-mesa.xyz>
> Signed-off-by: Lennart Glauer <mail@lennart-glauer.de>
> ---
> kernel/cgroup/cpuset.c | 2 +-
> kernel/locking/mutex-debug.h | 2 +-
> kernel/power/power.h | 2 +-
> kernel/sched/core.c | 2 +-
> kernel/trace/trace.h | 14 +++++++-------
Acked-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
-- Steve
> 5 files changed, 11 insertions(+), 11 deletions(-)
^ permalink raw reply
* [v3 PATCH] RISC-V: Add a PE/COFF compliant Image header.
From: Atish Patra @ 2019-05-23 18:35 UTC (permalink / raw)
To: linux-kernel
Cc: Atish Patra, Albert Ou, Anup Patel, Jonathan Corbet, linux-doc,
linux-riscv, Nick Kossifidis, Palmer Dabbelt, Zong Li,
linux-arm-kernel, mark.rutland, marek.vasut, catalin.marinas,
will.deacon, trini, paul.walmsley
Currently, last stage boot loaders such as U-Boot can accept only
uImage which is an unnecessary additional step in automating boot flows.
Add a PE/COFF compliant image header that boot loaders can parse and
directly load kernel flat Image. The existing booting methods will continue
to work as it is.
Another goal of this header is to support EFI stub for RISC-V in future.
EFI specification needs PE/COFF image header in the beginning of the kernel
image in order to load it as an EFI application. In order to support
EFI stub, code0 should be replaced with "MZ" magic string and res5(at
offset 0x3c) should point to the rest of the PE/COFF header (which will
be added during EFI support).
This patch is based on ARM64 boot image header and provides an opprtunity
to combine both ARM64 & RISC-V image headers.
Tested on both QEMU and HiFive Unleashed using OpenSBI + U-Boot + Linux.
Signed-off-by: Atish Patra <atish.patra@wdc.com>
---
I have not sent out corresponding U-Boot patch as all the changes are
compatible with current u-boot support. Once, the kernel header format
is agreed upon, I will update the U-Boot patch.
Changes from v2->v3
1. Modified reserved fields to define a header version.
2. Added header documentation.
Changes from v1-v2:
1. Added additional reserved elements to make it fully PE compatible.
---
| 50 ++++++++++++++++++
arch/riscv/include/asm/image.h | 64 +++++++++++++++++++++++
arch/riscv/kernel/head.S | 32 ++++++++++++
3 files changed, 146 insertions(+)
create mode 100644 Documentation/riscv/boot-image-header.txt
create mode 100644 arch/riscv/include/asm/image.h
--git a/Documentation/riscv/boot-image-header.txt b/Documentation/riscv/boot-image-header.txt
new file mode 100644
index 000000000000..68abc2353cec
--- /dev/null
+++ b/Documentation/riscv/boot-image-header.txt
@@ -0,0 +1,50 @@
+ Boot image header in RISC-V Linux
+ =============================================
+
+Author: Atish Patra <atish.patra@wdc.com>
+Date : 20 May 2019
+
+This document only describes the boot image header details for RISC-V Linux.
+The complete booting guide will be available at Documentation/riscv/booting.txt.
+
+The following 64-byte header is present in decompressed Linux kernel image.
+
+ u32 code0; /* Executable code */
+ u32 code1; /* Executable code */
+ u64 text_offset; /* Image load offset, little endian */
+ u64 image_size; /* Effective Image size, little endian */
+ u64 flags; /* kernel flags, little endian */
+ u32 version; /* Version of this header */
+ u32 res1 = 0; /* Reserved */
+ u64 res2 = 0; /* Reserved */
+ u64 magic = 0x5643534952; /* Magic number, little endian, "RISCV" */
+ u32 res3; /* Reserved for additional RISC-V specific header */
+ u32 res4; /* Reserved for PE COFF offset */
+
+This header format is compliant with PE/COFF header and largely inspired from
+ARM64 header. Thus, both ARM64 & RISC-V header can be combined into one common
+header in future.
+
+Notes:
+- This header can also be reused to support EFI stub for RISC-V in future. EFI
+ specification needs PE/COFF image header in the beginning of the kernel image
+ in order to load it as an EFI application. In order to support EFI stub,
+ code0 should be replaced with "MZ" magic string and res5(at offset 0x3c) should
+ point to the rest of the PE/COFF header.
+
+- version field indicate header version number.
+ Bits 0:15 - Minor version
+ Bits 16:31 - Major version
+
+ This preserves compatibility across newer and older version of the header.
+ The current version is defined as 0.1.
+
+- res3 is reserved for offset to any other additional fields. This makes the
+ header extendible in future. One example would be to accommodate ISA
+ extension for RISC-V in future. For current version, it is set to be zero.
+
+- In current header, the flag field has only one field.
+ Bit 0: Kernel endianness. 1 if BE, 0 if LE.
+
+- Image size is mandatory for boot loader to load kernel image. Booting will
+ fail otherwise.
diff --git a/arch/riscv/include/asm/image.h b/arch/riscv/include/asm/image.h
new file mode 100644
index 000000000000..61c9f20d2f19
--- /dev/null
+++ b/arch/riscv/include/asm/image.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ASM_IMAGE_H
+#define __ASM_IMAGE_H
+
+#define RISCV_IMAGE_MAGIC "RISCV"
+
+
+#define RISCV_IMAGE_FLAG_BE_SHIFT 0
+#define RISCV_IMAGE_FLAG_BE_MASK 0x1
+
+#define RISCV_IMAGE_FLAG_LE 0
+#define RISCV_IMAGE_FLAG_BE 1
+
+
+#ifdef CONFIG_CPU_BIG_ENDIAN
+#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_BE
+#else
+#define __HEAD_FLAG_BE RISCV_IMAGE_FLAG_LE
+#endif
+
+#define __HEAD_FLAG(field) (__HEAD_FLAG_##field << \
+ RISCV_IMAGE_FLAG_##field##_SHIFT)
+
+#define __HEAD_FLAGS (__HEAD_FLAG(BE))
+
+#define RISCV_HEADER_VERSION_MAJOR 0
+#define RISCV_HEADER_VERSION_MINOR 1
+
+#define RISCV_HEADER_VERSION (RISCV_HEADER_VERSION_MAJOR << 16 | \
+ RISCV_HEADER_VERSION_MINOR)
+
+#ifndef __ASSEMBLY__
+/*
+ * struct riscv_image_header - riscv kernel image header
+ *
+ * @code0: Executable code
+ * @code1: Executable code
+ * @text_offset: Image load offset
+ * @image_size: Effective Image size
+ * @flags: kernel flags
+ * @version: version
+ * @reserved: reserved
+ * @reserved: reserved
+ * @magic: Magic number
+ * @reserved: reserved (will be used for additional RISC-V specific header)
+ * @reserved: reserved (will be used for PE COFF offset)
+ */
+
+struct riscv_image_header {
+ u32 code0;
+ u32 code1;
+ u64 text_offset;
+ u64 image_size;
+ u64 flags;
+ u32 version;
+ u32 res1;
+ u64 res2;
+ u64 magic;
+ u32 res3;
+ u32 res4;
+};
+#endif /* __ASSEMBLY__ */
+#endif /* __ASM_IMAGE_H */
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S
index 370c66ce187a..577893bb150d 100644
--- a/arch/riscv/kernel/head.S
+++ b/arch/riscv/kernel/head.S
@@ -19,9 +19,41 @@
#include <asm/thread_info.h>
#include <asm/page.h>
#include <asm/csr.h>
+#include <asm/image.h>
__INIT
ENTRY(_start)
+ /*
+ * Image header expected by Linux boot-loaders. The image header data
+ * structure is described in asm/image.h.
+ * Do not modify it without modifying the structure and all bootloaders
+ * that expects this header format!!
+ */
+ /* jump to start kernel */
+ j _start_kernel
+ /* reserved */
+ .word 0
+ .balign 8
+#if __riscv_xlen == 64
+ /* Image load offset(2MB) from start of RAM */
+ .dword 0x200000
+#else
+ /* Image load offset(4MB) from start of RAM */
+ .dword 0x400000
+#endif
+ /* Effective size of kernel image */
+ .dword _end - _start
+ .dword __HEAD_FLAGS
+ .word RISCV_HEADER_VERSION
+ .word 0
+ .dword 0
+ .asciz RISCV_IMAGE_MAGIC
+ .word 0
+ .balign 4
+ .word 0
+
+.global _start_kernel
+_start_kernel:
/* Mask all interrupts */
csrw CSR_SIE, zero
csrw CSR_SIP, zero
--
2.21.0
^ permalink raw reply related
* Re: [GIT PULL] Documentation fixes for 5.2
From: pr-tracker-bot @ 2019-05-23 15:15 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: Linus Torvalds, linux-doc, LKML
In-Reply-To: <20190523084805.63901c65@lwn.net>
The pull request you sent on Thu, 23 May 2019 08:48:05 -0600:
> git://git.lwn.net/linux.git tags/docs-5.2-fixes
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/61686afe1ff378021da4e5509d081fb5196212d5
Thank you!
--
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/prtracker
^ permalink raw reply
* [GIT PULL] Documentation fixes for 5.2
From: Jonathan Corbet @ 2019-05-23 14:48 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-doc, LKML
The following changes since commit
a188339ca5a396acc588e5851ed7e19f66b0ebd9:
Linux 5.2-rc1 (2019-05-19 15:47:09 -0700)
are available in the Git repository at:
git://git.lwn.net/linux.git tags/docs-5.2-fixes
for you to fetch changes up to a65fd4f0def56f59822b2c49522d36319bc8da8b:
Documentation: kdump: fix minor typo (2019-05-21 09:31:28 -0600)
----------------------------------------------------------------
A handful of fixes for a docs build problem, along with catching the
spdxcheck.py script up with the current state of affairs.
----------------------------------------------------------------
Cengiz Can (1):
Documentation: kdump: fix minor typo
Randy Dunlap (1):
counter: fix Documentation build error due to incorrect source file name
Sven Eckelmann (2):
scripts/spdxcheck.py: Fix path to deprecated licenses
scripts/spdxcheck.py: Add dual license subdirectory
Documentation/driver-api/generic-counter.rst | 2 +-
Documentation/kdump/kdump.txt | 2 +-
scripts/spdxcheck.py | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox