* Re: [PATCH] mm: memcontrol: asynchronous reclaim for memory.high
From: Tejun Heo @ 2020-02-20 15:56 UTC (permalink / raw)
To: Daniel Jordan
Cc: Johannes Weiner, Michal Hocko, Andrew Morton, Roman Gushchin,
linux-mm, cgroups, linux-kernel, kernel-team, Peter Zijlstra
In-Reply-To: <20200220154524.dql3i5brnjjwecft@ca-dmjordan1.us.oracle.com>
On Thu, Feb 20, 2020 at 10:45:24AM -0500, Daniel Jordan wrote:
> Ok, consistency with io and memory is one advantage to doing it that way.
> Creating kthreads in cgroups also seems viable so far, and it's unclear whether
> either approach is significantly simpler or more maintainable than the other,
> at least to me.
The problem with separate kthread approach is that many of these work
units are tiny, and cgroup membership might not be known or doesn't
agree with the processing context from the beginning
For example, the ownership of network packets can't be determined till
processing has progressed quite a bit in shared contexts and each item
too small to bounce around. The only viable way I can think of
splitting aggregate overhead according to the number of packets (or
some other trivially measureable quntity) processed.
Anything sitting in reclaim layer is the same. Reclaim should be
charged to the cgroup whose memory is reclaimed *but* shouldn't block
other cgroups which are waiting for that memory. It has to happen in
the context of the highest priority entity waiting for memory but the
costs incurred must be charged to the memory owners.
So, one way or the other, I think we'll need back charging and once
back charging is needed for big ticket items like network and reclaim,
it's kinda silly to use separate mechanisms for other stuff.
> Is someone on your side working on remote charging right now? I was planning
> to post an RFD comparing these soon and it would make sense to include them.
It's been on the to do list but nobody is working on it yet.
Thanks.
--
tejun
^ permalink raw reply
* Re: [PATCH v2] xfs: add agf freeblocks verify in xfs_agf_verify
From: Darrick J. Wong @ 2020-02-20 15:56 UTC (permalink / raw)
To: Zheng Bin; +Cc: sandeen, bfoster, dchinner, linux-xfs, renxudong1, yi.zhang
In-Reply-To: <1582197182-142137-1-git-send-email-zhengbin13@huawei.com>
On Thu, Feb 20, 2020 at 07:13:02PM +0800, Zheng Bin wrote:
> We recently used fuzz(hydra) to test XFS and automatically generate
> tmp.img(XFS v5 format, but some metadata is wrong)
>
> xfs_repair information(just one AG):
> agf_freeblks 0, counted 3224 in ag 0
> agf_longest 536874136, counted 3224 in ag 0
> sb_fdblocks 613, counted 3228
>
> Test as follows:
> mount tmp.img tmpdir
> cp file1M tmpdir
> sync
>
> In 4.19-stable, sync will stuck, the reason is:
> xfs_mountfs
> xfs_check_summary_counts
> if ((!xfs_sb_version_haslazysbcount(&mp->m_sb) ||
> XFS_LAST_UNMOUNT_WAS_CLEAN(mp)) &&
> !xfs_fs_has_sickness(mp, XFS_SICK_FS_COUNTERS))
> return 0; -->just return, incore sb_fdblocks still be 613
> xfs_initialize_perag_data
>
> cp file1M tmpdir -->ok(write file to pagecache)
> sync -->stuck(write pagecache to disk)
> xfs_map_blocks
> xfs_iomap_write_allocate
> while (count_fsb != 0) {
> nimaps = 0;
> while (nimaps == 0) { --> endless loop
> nimaps = 1;
> xfs_bmapi_write(..., &nimaps) --> nimaps becomes 0 again
> xfs_bmapi_write
> xfs_bmap_alloc
> xfs_bmap_btalloc
> xfs_alloc_vextent
> xfs_alloc_fix_freelist
> xfs_alloc_space_available -->fail(agf_freeblks is 0)
>
> In linux-next, sync not stuck, cause commit c2b3164320b5 ("xfs:
> use the latest extent at writeback delalloc conversion time") remove
> the above while, dmesg is as follows:
> [ 55.250114] XFS (loop0): page discard on page ffffea0008bc7380, inode 0x1b0c, offset 0.
>
> Users do not know why this page is discard, the better soultion is:
> 1. Like xfs_repair, make sure sb_fdblocks is equal to counted
> (xfs_initialize_perag_data did this, who is not called at this mount)
> 2. Add agf verify, if fail, will tell users to repair
>
> This patch use the second soultion.
>
> Signed-off-by: Zheng Bin <zhengbin13@huawei.com>
> Signed-off-by: Ren Xudong <renxudong1@huawei.com>
> ---
> v1->v2: modify comment, add more agf verify
> fs/xfs/libxfs/xfs_alloc.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/fs/xfs/libxfs/xfs_alloc.c b/fs/xfs/libxfs/xfs_alloc.c
> index d8053bc..5faed42 100644
> --- a/fs/xfs/libxfs/xfs_alloc.c
> +++ b/fs/xfs/libxfs/xfs_alloc.c
> @@ -2839,6 +2839,7 @@ xfs_agf_verify(
> {
> struct xfs_mount *mp = bp->b_mount;
> struct xfs_agf *agf = XFS_BUF_TO_AGF(bp);
> + int i;
>
> if (xfs_sb_version_hascrc(&mp->m_sb)) {
> if (!uuid_equal(&agf->agf_uuid, &mp->m_sb.sb_meta_uuid))
> @@ -2858,6 +2859,22 @@ xfs_agf_verify(
> be32_to_cpu(agf->agf_flcount) <= xfs_agfl_size(mp)))
> return __this_address;
>
> + if (be32_to_cpu(agf->agf_length) > mp->m_sb.sb_dblocks ||
> + be32_to_cpu(agf->agf_btreeblks) > be32_to_cpu(agf->agf_length) ||
Isn't this already covered later on?
> + be32_to_cpu(agf->agf_rmap_blocks) > be32_to_cpu(agf->agf_length) ||
> + be32_to_cpu(agf->agf_refcount_blocks) > be32_to_cpu(agf->agf_length) ||
Do these fields need checking when the corresponding feature isn't
enabled?
> + be32_to_cpu(agf->agf_spare2) != 0)
If, for some reason, these unused "spare" fields are *not* zero, won't
this cause mount failures on existing filesystems?
> + return __this_address;
Please try to check only one agf field per if clause, because we use the
logged __this_address to figure out which field triggered the corruption
error.
> +
> + for (i = 0; i < ARRAY_SIZE(agf->agf_spare64); i++)
> + if (be64_to_cpu(agf->agf_spare64[i]) != 0)
> + return __this_address;
memchr_inv if you leave in the spare check.
> +
> + if (be32_to_cpu(agf->agf_freeblks) < be32_to_cpu(agf->agf_longest) ||
> + be32_to_cpu(agf->agf_freeblks) > be32_to_cpu(agf->agf_length) ||
Already covered in this function.
--D
> + be32_to_cpu(agf->agf_freeblks) > mp->m_sb.sb_fdblocks)
> + return __this_address;
> +
> if (be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]) < 1 ||
> be32_to_cpu(agf->agf_levels[XFS_BTNUM_CNT]) < 1 ||
> be32_to_cpu(agf->agf_levels[XFS_BTNUM_BNO]) > XFS_BTREE_MAXLEVELS ||
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH v4 2/6] driver core: Set deferred_probe_timeout to a longer default if CONFIG_MODULES is set
From: Bjorn Andersson @ 2020-02-20 15:55 UTC (permalink / raw)
To: John Stultz
Cc: lkml, Rob Herring, Rafael J. Wysocki, Kevin Hilman, Ulf Hansson,
Pavel Machek, Len Brown, Todd Kjos, Liam Girdwood, Mark Brown,
Thierry Reding, Linus Walleij, Greg Kroah-Hartman, linux-pm
In-Reply-To: <20200220050440.45878-3-john.stultz@linaro.org>
On Wed 19 Feb 21:04 PST 2020, John Stultz wrote:
> When using modules, its common for the modules not to be loaded
> until quite late by userland. With the current code,
> driver_deferred_probe_check_state() will stop returning
> EPROBE_DEFER after late_initcall, which can cause module
> dependency resolution to fail after that.
>
> So allow a longer window of 30 seconds (picked somewhat
> arbitrarily, but influenced by the similar regulator core
> timeout value) in the case where modules are enabled.
>
> Cc: Rob Herring <robh@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Cc: Kevin Hilman <khilman@kernel.org>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Len Brown <len.brown@intel.com>
> Cc: Todd Kjos <tkjos@google.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Thierry Reding <treding@nvidia.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: linux-pm@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> Change-Id: I9c5a02a54915ff53f9f14d49c601f41d7105e05e
Change-Id...
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Regards,
Bjorn
> ---
> v4:
> * Split out into its own patch as suggested by Mark
> * Made change conditional on CONFIG_MODULES
> ---
> drivers/base/dd.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index bb383dca39c1..fa138f24e2d3 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -224,7 +224,16 @@ static int deferred_devs_show(struct seq_file *s, void *data)
> }
> DEFINE_SHOW_ATTRIBUTE(deferred_devs);
>
> +#ifdef CONFIG_MODULES
> +/*
> + * In the case of modules, set the default probe timeout to
> + * 30 seconds to give userland some time to load needed modules
> + */
> +static int deferred_probe_timeout = 30;
> +#else
> +/* In the case of !modules, no probe timeout needed */
> static int deferred_probe_timeout = -1;
> +#endif
> static int __init deferred_probe_timeout_setup(char *str)
> {
> int timeout;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH v8 0/8] btrfs: remove buffer heads form superblock handling
From: David Sterba @ 2020-02-20 15:56 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: David Sterba, Nikolay Borisov, Josef Bacik,
linux-btrfs @ vger . kernel . org, Christoph Hellwig
In-Reply-To: <20200213152436.13276-1-johannes.thumshirn@wdc.com>
On Fri, Feb 14, 2020 at 12:24:28AM +0900, Johannes Thumshirn wrote:
> This patch series removes the use of buffer_heads from btrfs' super block read
> and write paths. It also converts the integrity-checking code to only work
> with pages and BIOs.
I've fixed the small things and added this patchset to misc-next.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: mscc: ocelot: Workaround to allow traffic to CPU in standalone mode
From: Vladimir Oltean @ 2020-02-20 15:56 UTC (permalink / raw)
To: Allan W. Nielsen
Cc: Andrew Lunn, David S. Miller, Horatiu Vultur, Alexandre Belloni,
Florian Fainelli, Vivien Didelot, Joergen Andreasen,
Claudiu Manoil, netdev, Microchip Linux Driver Support
In-Reply-To: <20200220132329.43s6tq3xsoo7htuz@lx-anielsen.microsemi.net>
On Thu, 20 Feb 2020 at 15:23, Allan W. Nielsen
<allan.nielsen@microchip.com> wrote:
>
> Horatiu and I have looked further into this, done a few experiments, and
> discussed with the HW engineers who have a more detailed version of how
> the chips are working and how Ocelot and Felix differs.
>
> Here are our findings:
>
> - The most significant bit in the PGID table is "special" as it is a
> CPU-copy bit.
Wow.
Looking at the code after this realization, it is so confusing to call
the NPI port "ocelot->cpu" now, since it doesn't benefit from this
"privilege" that a "real" CPU port (from the Ocelot hardware
perspective) has.
Not to mention how strange it is for the hardware to behave this way.
> - This bit is not being used in the source filtering!
So BIT(6) on Felix and BIT(11) on Ocelot are just being interpreted
for the first and second PGID lookup (destination and aggregation
masks) but not for source masks? Don't you want to actually document
this somewhere?
So the frame is copied to the CPU based on the AND between first and
second lookup, and to all the other ports, including the NPI port,
based on the first, second and third PGID lookup.
So frames can reach the NPI port directly, via 3 PGID lookups, or
indirectly, via 2 PGID lookups. What I did is make the direct path
work. You're suggesting me to set the indirect mode up.
> This means that
> your original patch can be applied without breaking Ocelot (the
> uninitialized cpu field must be fixed though).
So my patch makes the Felix NPI port work in an unintended way and
does not affect Ocelot in any way. Roger.
> - Still I do not think we should do this as it is not the root-casuse
> - In Felix we have 2 ways to get frames to the CPU, in Ocelot we have 1
> (Ocelot also has two if it uses an NPI port, but it does not do that
> in the current driver).
> - In Felix you can get frames to the CPU by either using the CPU port
> (port 6), or by using the NPI port (which can be any in the range of
> 0-5).
> - But you should only use the CPU port, and not the NPI port
> directly. Using the NPI port directly will cause the two targets
> to behave differently, and this is not what we do when testing all
> the use-cases on the switch.
Differently in what way?
> - In Ocelot you can only get frames to the CPU by using the CPU port
> (port 11).
>
> Due to this, I very much think you need to fix this, such that Felix
> always port 6 to reach the CPU (with the exception of writing
> QSYS_EXT_CPU_CFG where you "connect" the CPU queue/port to the NPI
> port).
>
What PGIDs should I use for the NPI port if I use it with indirection
via port 6?
> If you do this change, then the Ocelot and Felix should start to work in
> the same way.
>
> Then, if you want the CPU to be part of the unicast flooding (this is
> where this discussion started), then you should add the CPU port to the
> PGID entry pointed at in ANA:ANA:FLOODING:FLD_UNICAST. This should be
> done for Felix and not for Ocelot.
No, the question is why don't _you_ want the CPU to be in the
FLD_UNICAST PGID (which is PGID_UC). The distinction you're making
between Felix and Ocelot here is quite arbitrary, and seems to be
based just on "your CPU is more powerful".
So right now, multicast and broadcast traffic goes to PGID_MC (61),
and unknown unicast goes to PGID_UC (60).
The destination ports mask for PGID_MC is
GENMASK(ocelot->num_phys_ports, 0) - 0x7f for me, 0xfff for you.
The destination ports mask for PGID_UC is
GENMASK(ocelot->num_phys_ports - 1, 0) - this is the hardware default
value - 0x3f for me, 0x7ff for you.
So you are keeping the non-physical CPU port in the destination mask
for broadcast, but not in that for unknown unicast. In the way the
system is configured, it is still susceptible to broadcast storms. So
I don't think there is any real benefit in crippling the system like
this. If port 6 was in PGID_UC by default, I would have never bat an
eye and more than likely never needed to know the gory details.
Is it possible to set up policers for traffic going to CPU? I _did_
see this phrase already in the manual:
"Frames where the DMAC lookup returned a PGID with the CPU port set
are always forwarded to the CPU even when the frame
is policed by the storm policers."
So the traffic I'm seeing now on the NPI port is not copied to the
CPU, it is forwarded.
If there's no other way to set up storm policers for the mode you're
suggesting me to change Felix to, then I would respectfully keep it
the way it is right now.
>
> If you want the analyser (where the MAC table sits), to "learn" the CPU
> MAC (which is needed if you do not want to have the CPU mac as a static
> entry in the MAC-table), then you need to set the 'src-port' to 6 (if it
> is Ocelot then it will be 11) in the IFH:
>
Please tell me more about this. Don't I need to set BYPASS=1 for
frames to go on the front-panel port specified in DEST? And doesn't
BYPASS=1 mean no source MAC learning for injected traffic?
As much as I would like the analyzer to run, I won't do it if it is
going to compromise xmit from Linux. I don't want traffic sent from
Linux to an unknown unicast in standalone mode to be flooded to all
front-panel ports with no way for me to control it.
> anielsen@lx-anielsen ~ $ ef hex ifh-oc1 help
> ifh-oc1 Injection Frame Header for Ocelot1
>
> Specify the ifh-oc1 header by using one or more of the following fields:
> - Name ------------ offset:width --- Description --------------------------
> bypass + 0: 1 Skip analyzer processing
> b1-rew-mac + 1: 1 Replace SMAC address
> b1-rew-op + 2: 9 Rewriter operation command
> b0-masq + 1: 1 Enable masquerading
> b0-masq-port + 2: 4 Masquerading port
> rew-val + 11: 32 Receive time stamp
> res1 + 43: 17 Reserved
> dest + 60: 12 Destination set for the frame. Dest[11] is the CPU
> res2 + 72: 9 Reserved
> src-port + 81: 4 The port number where the frame was injected (0-12) <------------------- THIS FIELD
> res3 + 85: 2 Reserved
> trfm-timer + 87: 4 Timer for periodic transmissions (1..8). If zero then normal injection
> res4 + 91: 6 Reserved
> dp + 97: 1 Drop precedence level after policing
> pop-cnt + 98: 2 Number of VLAN tags that must be popped
> cpuq +100: 8 CPU extraction queue mask
> qos-class +108: 3 Classified QoS class
> tag-type +111: 1 Tag information's associated Tag Protocol Identifier (TPID)
> pcp +112: 3 Classified PCP
> dei +115: 1 Classified DEI
> vid +116: 12 Classified VID
>
>
> /Allan
>
Thanks,
-Vladimir
^ permalink raw reply
* [PATCH] firmware: imx: Remove IMX_SC_RPC_SVC_ABORT
From: Leonard Crestez @ 2020-02-20 15:56 UTC (permalink / raw)
To: Shawn Guo, Dong Aisheng
Cc: Fabio Estevam, Franck LENORMAND, linux-imx, kernel,
linux-arm-kernel
This is not used by linux and not supported as part of imx SCU api, it
was added by mistake.
The constant value "9" has since been reassigned in firmware to a
different service.
Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com>
---
include/linux/firmware/imx/ipc.h | 1 -
1 file changed, 1 deletion(-)
diff --git a/include/linux/firmware/imx/ipc.h b/include/linux/firmware/imx/ipc.h
index 6312c8cb084a..891057434858 100644
--- a/include/linux/firmware/imx/ipc.h
+++ b/include/linux/firmware/imx/ipc.h
@@ -23,11 +23,10 @@ enum imx_sc_rpc_svc {
IMX_SC_RPC_SVC_RM = 3,
IMX_SC_RPC_SVC_TIMER = 5,
IMX_SC_RPC_SVC_PAD = 6,
IMX_SC_RPC_SVC_MISC = 7,
IMX_SC_RPC_SVC_IRQ = 8,
- IMX_SC_RPC_SVC_ABORT = 9
};
struct imx_sc_rpc_msg {
uint8_t ver;
uint8_t size;
--
2.17.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [igt-dev] [PATCH i-g-t 9/9] i915: Exercise I915_CONTEXT_PARAM_RINGSIZE
From: Janusz Krzysztofik @ 2020-02-20 15:57 UTC (permalink / raw)
To: Chris Wilson; +Cc: igt-dev, intel-gfx
In-Reply-To: <157529875955.27263.14886639874940144583@skylake-alporthouse-com>
Hi Chris,
On Monday, December 2, 2019 3:59:19 PM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-12-02 14:42:58)
> > Hi Chris,
> >
> > I have a few questions rather than comments. I hope they are worth spending
> > your time.
> >
> > On Wednesday, November 13, 2019 1:52:40 PM CET Chris Wilson wrote:
> > > I915_CONTEXT_PARAM_RINGSIZE specifies how large to create the command
> > > ringbuffer for logical ring contects. This directly affects the number
> >
> > s/contects/contexts/
> >
> > > of batches userspace can submit before blocking waiting for space.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Have you got this patch still queued somewhere? As UMD has accepted the
solution and are ready with changes on their side, I think we need to merge it
soon, and the kernel side as well.
Thanks,
Janusz
> > > ---
> > > tests/Makefile.sources | 3 +
> > > tests/i915/gem_ctx_ringsize.c | 296 ++++++++++++++++++++++++++++++++++
> > > tests/meson.build | 1 +
> > > 3 files changed, 300 insertions(+)
> > > create mode 100644 tests/i915/gem_ctx_ringsize.c
> > >
> > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > index e17d43155..801fc52f3 100644
> > > --- a/tests/Makefile.sources
> > > +++ b/tests/Makefile.sources
> > > @@ -163,6 +163,9 @@ gem_ctx_param_SOURCES = i915/gem_ctx_param.c
> > > TESTS_progs += gem_ctx_persistence
> > > gem_ctx_persistence_SOURCES = i915/gem_ctx_persistence.c
> > >
> > > +TESTS_progs += gem_ctx_ringsize
> > > +gem_ctx_ringsize_SOURCES = i915/gem_ctx_ringsize.c
> > > +
> > > TESTS_progs += gem_ctx_shared
> > > gem_ctx_shared_SOURCES = i915/gem_ctx_shared.c
> > >
> > > diff --git a/tests/i915/gem_ctx_ringsize.c b/tests/i915/gem_ctx_ringsize.c
> > > new file mode 100644
> > > index 000000000..1450e8f0d
> > > --- /dev/null
> > > +++ b/tests/i915/gem_ctx_ringsize.c
> > > @@ -0,0 +1,296 @@
> > > +/*
> > > + * Copyright © 2019 Intel Corporation
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person obtaining a
> > > + * copy of this software and associated documentation files (the "Software"),
> > > + * to deal in the Software without restriction, including without limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to whom the
> > > + * Software is furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice (including the next
> > > + * paragraph) shall be included in all copies or substantial portions of the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > > + * IN THE SOFTWARE.
> > > + */
> > > +
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +#include <inttypes.h>
> > > +#include <sys/ioctl.h>
> > > +#include <sys/types.h>
> > > +#include <unistd.h>
> > > +
> > > +#include "drmtest.h" /* gem_quiescent_gpu()! */
> > > +#include "i915/gem_context.h"
> > > +#include "i915/gem_engine_topology.h"
> > > +#include "ioctl_wrappers.h" /* gem_wait()! */
> > > +#include "sw_sync.h"
> > > +
> > > +#define I915_CONTEXT_PARAM_RINGSIZE 0xc
> >
> > How are we going to handle symbol redefinition conflict which arises as soon
> > as this symbol is also included from kernel headers (e.g. via
> > "i915/gem_engine_topology.h")?
>
> Final version we copy the headers form the kernel. Conflicts remind us
> when we forget.
>
> >
> > > +
> > > +static bool has_ringsize(int i915)
> > > +{
> > > + struct drm_i915_gem_context_param p = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + };
> > > +
> > > + return __gem_context_get_param(i915, &p) == 0;
> > > +}
> > > +
> > > +static void test_idempotent(int i915)
> > > +{
> > > + struct drm_i915_gem_context_param p = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + };
> > > + uint32_t saved;
> > > +
> > > + /*
> > > + * Simple test to verify that we are able to read back the same
> > > + * value as we set.
> > > + */
> > > +
> > > + gem_context_get_param(i915, &p);
> > > + saved = p.value;
> > > +
> > > + for (uint32_t x = 1 << 12; x <= 128 << 12; x <<= 1) {
> >
> > I've noticed you are using two different notations for those minimum/maximum
> > constants. I think that may be confusing. How about defining and using
> > macros?
>
> A range in pages...
>
> > > + p.value = x;
> > > + gem_context_set_param(i915, &p);
> > > + gem_context_get_param(i915, &p);
> > > + igt_assert_eq_u32(p.value, x);
> > > + }
> > > +
> > > + p.value = saved;
> > > + gem_context_set_param(i915, &p);
> > > +}
> > > +
> > > +static void test_invalid(int i915)
> > > +{
> > > + struct drm_i915_gem_context_param p = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + };
> > > + uint64_t invalid[] = {
> > > + 0, 1, 4095, 4097, 8191, 8193,
> > > + /* upper limit may be HW dependent, atm it is 512KiB */
> > > + (512 << 10) - 1, (512 << 10) + 1,
> >
> > Here is an example of that different notation mentioned above.
>
> And here written in KiB to match comments.
>
> >
> > > + -1, -1u
> > > + };
> > > + uint32_t saved;
> > > +
> > > + gem_context_get_param(i915, &p);
> > > + saved = p.value;
> > > +
> > > + for (int i = 0; i < ARRAY_SIZE(invalid); i++) {
> > > + p.value = invalid[i];
> > > + igt_assert_eq(__gem_context_set_param(i915, &p), -EINVAL);
> > > + gem_context_get_param(i915, &p);
> > > + igt_assert_eq_u64(p.value, saved);
> > > + }
> > > +}
> > > +
> > > +static int create_ext_ioctl(int i915,
> > > + struct drm_i915_gem_context_create_ext *arg)
> > > +{
> > > + int err;
> > > +
> > > + err = 0;
> > > + if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, arg)) {
> > > + err = -errno;
> > > + igt_assume(err);
> > > + }
> > > +
> > > + errno = 0;
> > > + return err;
> > > +}
> >
> > This helper looks like pretty standard for me. Why there are no library
> > functions for such generic operations?
>
> Because no one has written that yet.
>
> >
> > > +
> > > +static void test_create(int i915)
> > > +{
> > > + struct drm_i915_gem_context_create_ext_setparam p = {
> > > + .base = {
> > > + .name = I915_CONTEXT_CREATE_EXT_SETPARAM,
> > > + .next_extension = 0, /* end of chain */
> > > + },
> > > + .param = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + .value = 512 << 10,
> > > + }
> > > + };
> > > + struct drm_i915_gem_context_create_ext create = {
> > > + .flags = I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
> > > + .extensions = to_user_pointer(&p),
> > > + };
> > > +
> > > + igt_assert_eq(create_ext_ioctl(i915, &create), 0);
> > > +
> > > + p.param.ctx_id = create.ctx_id;
> > > + p.param.value = 0;
> > > + gem_context_get_param(i915, &p.param);
> > > + igt_assert_eq(p.param.value, 512 << 10);
> > > +
> > > + gem_context_destroy(i915, create.ctx_id);
> > > +}
> > > +
> > > +static void test_clone(int i915)
> > > +{
> > > + struct drm_i915_gem_context_create_ext_setparam p = {
> > > + .base = {
> > > + .name = I915_CONTEXT_CREATE_EXT_SETPARAM,
> > > + .next_extension = 0, /* end of chain */
> > > + },
> > > + .param = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + .value = 512 << 10,
> > > + }
> > > + };
> > > + struct drm_i915_gem_context_create_ext create = {
> > > + .flags = I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
> > > + .extensions = to_user_pointer(&p),
> > > + };
> > > +
> > > + igt_assert_eq(create_ext_ioctl(i915, &create), 0);
> > > +
> > > + p.param.ctx_id = gem_context_clone(i915, create.ctx_id,
> > > + I915_CONTEXT_CLONE_ENGINES, 0);
> > > + igt_assert_neq(p.param.ctx_id, create.ctx_id);
> > > + gem_context_destroy(i915, create.ctx_id);
> > > +
> > > + p.param.value = 0;
> > > + gem_context_get_param(i915, &p.param);
> > > + igt_assert_eq(p.param.value, 512 << 10);
> > > +
> > > + gem_context_destroy(i915, p.param.ctx_id);
> > > +}
> > > +
> > > +static int __execbuf(int i915, struct drm_i915_gem_execbuffer2 *execbuf)
> > > +{
> > > + int err;
> > > +
> > > + err = 0;
> > > + if (ioctl(i915, DRM_IOCTL_I915_GEM_EXECBUFFER2, execbuf))
> > > + err = -errno;
> > > +
> > > + errno = 0;
> > > + return err;
> > > +}
> >
> > The above helper looks pretty the same as lib/ioctlwrappers.c:__gem_execbuf().
> > Does igt_assume(err) found in the latter matter so much that you use your own
> > version?
>
> It's very, very different from that one.
>
> > > +
> > > +static uint32_t __batch_create(int i915, uint32_t offset)
> >
> > This is always called with offset = 0, do we expect other values to be used
> > later?
>
> Why not.
>
> > > +{
> > > + const uint32_t bbe = 0xa << 23;
> > > + uint32_t handle;
> > > +
> > > + handle = gem_create(i915, ALIGN(offset + sizeof(bbe), 4096));
> >
> > Why don't we rely on the driver making the alignment for us?
>
> I'm used to being inside the kernel where it's expected to be correct.
>
> > > + gem_write(i915, handle, offset, &bbe, sizeof(bbe));
> > > +
> > > + return handle;
> > > +}
> > > +
> > > +static uint32_t batch_create(int i915)
> > > +{
> > > + return __batch_create(i915, 0);
> > > +}
> > > +
> > > +static unsigned int measure_inflight(int i915, unsigned int engine)
> > > +{
> > > + IGT_CORK_FENCE(cork);
> > > + struct drm_i915_gem_exec_object2 obj = {
> > > + .handle = batch_create(i915)
> > > + };
> > > + struct drm_i915_gem_execbuffer2 execbuf = {
> > > + .buffers_ptr = to_user_pointer(&obj),
> > > + .buffer_count = 1,
> > > + .flags = engine | I915_EXEC_FENCE_IN,
> > > + .rsvd2 = igt_cork_plug(&cork, i915),
> > > + };
> > > + unsigned int count;
> > > +
> > > + fcntl(i915, F_SETFL, fcntl(i915, F_GETFL) | O_NONBLOCK);
> > > +
> > > + gem_execbuf(i915, &execbuf);
> > > + for (count = 1; __execbuf(i915, &execbuf) == 0; count++)
> > > + ;
> >
> > Shouldn't we check if the reason for the failure is what we expect, i.e.,
> > -EWOULDBLOCK (or -EINTR)? And why don't we put a time constraint on that loop
> > in case O_NONBLOCK handling is not supported (yet)?
>
> Sure. The idea is that O_NONBLOCK is supported, otherwise we don't
> have fast and precise feedback.
>
> > > +static void test_resize(int i915,
> > > + const struct intel_execution_engine2 *e,
> > > + unsigned int flags)
> > > +#define IDLE (1 << 0)
> > > +{
> > > + struct drm_i915_gem_context_param p = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + };
> > > + unsigned int prev[2] = {};
> > > + uint32_t saved;
> > > +
> > > + gem_context_get_param(i915, &p);
> > > + saved = p.value;
> > > +
> > > + gem_quiescent_gpu(i915);
> > > + for (p.value = 1 << 12; p.value <= 128 << 12; p.value <<= 1) {
> > > + unsigned int count;
> > > +
> > > + gem_context_set_param(i915, &p);
> > > +
> > > + count = measure_inflight(i915, e->flags);
> > > + igt_info("%s: %llx -> %d\n", e->name, p.value, count);
> > > + igt_assert(count > 3 * (prev[1] - prev[0]) / 4 + prev[1]);
> >
> > Where does this formula come from? Why not just count == 2 * prev[1] ?
> > What results should we expect in "active" vs. "idle" mode?
>
> I've explained somewhere why it is not 2*prev... And there's a small
> amount of imprecision (+-1 request). In test_resize is the comment:
>
> /*
> * The ringsize directly affects the number of batches we can have
> * inflight -- when we run out of room in the ring, the client is
> * blocked (or if O_NONBLOCK is specified, -EWOULDBLOCK is reported).
> * The kernel throttles the client when they enter the last 4KiB page,
> * so as we double the size of the ring, we nearly double the number
> * of requests we can fit as 2^n-1: i.e 0, 1, 3, 7, 15, 31 pages.
> */
>
> -Chris
>
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply
* [Cluster-devel] [PATCH v7 14/24] btrfs: Convert from readpages to readahead
From: Christoph Hellwig @ 2020-02-20 15:57 UTC (permalink / raw)
To: cluster-devel.redhat.com
In-Reply-To: <20200220155452.GX24185@bombadil.infradead.org>
On Thu, Feb 20, 2020 at 07:54:52AM -0800, Matthew Wilcox wrote:
> On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote:
> > On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote:
> > > btrfs: Convert from readpages to readahead
> > >
> > > Implement the new readahead method in btrfs. Add a readahead_page_batch()
> > > to optimise fetching a batch of pages at once.
> >
> > Shouldn't this readahead_page_batch heper go into a separate patch so
> > that it clearly stands out?
>
> I'll move it into 'Put readahead pages in cache earlier' for v8 (the
> same patch where we add readahead_page())
One argument for keeping it in a patch of its own is that btrfs appears
to be the only user, and Goldwyn has a WIP conversion of btrfs to iomap,
so it might go away pretty soon and we could just revert the commit.
But this starts to get into really minor details, so I'll shut up now :)
^ permalink raw reply
* [Ocfs2-devel] [PATCH v7 14/24] btrfs: Convert from readpages to readahead
From: Christoph Hellwig @ 2020-02-20 15:57 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Christoph Hellwig, Johannes Thumshirn,
linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org,
linux-erofs@lists.ozlabs.org, linux-ext4@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com,
ocfs2-devel@oss.oracle.com, linux-xfs@vger.kernel.org
In-Reply-To: <20200220155452.GX24185@bombadil.infradead.org>
On Thu, Feb 20, 2020 at 07:54:52AM -0800, Matthew Wilcox wrote:
> On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote:
> > On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote:
> > > btrfs: Convert from readpages to readahead
> > >
> > > Implement the new readahead method in btrfs. Add a readahead_page_batch()
> > > to optimise fetching a batch of pages at once.
> >
> > Shouldn't this readahead_page_batch heper go into a separate patch so
> > that it clearly stands out?
>
> I'll move it into 'Put readahead pages in cache earlier' for v8 (the
> same patch where we add readahead_page())
One argument for keeping it in a patch of its own is that btrfs appears
to be the only user, and Goldwyn has a WIP conversion of btrfs to iomap,
so it might go away pretty soon and we could just revert the commit.
But this starts to get into really minor details, so I'll shut up now :)
^ permalink raw reply
* Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
From: Christoph Hellwig @ 2020-02-20 15:57 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Christoph Hellwig, Johannes Thumshirn,
linux-fsdevel@vger.kernel.org, linux-mm@kvack.org,
linux-kernel@vger.kernel.org, linux-btrfs@vger.kernel.org,
linux-erofs@lists.ozlabs.org, linux-ext4@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net, cluster-devel@redhat.com,
ocfs2-devel@oss.oracle.com, linux-xfs@vger.kernel.org
In-Reply-To: <20200220155452.GX24185@bombadil.infradead.org>
On Thu, Feb 20, 2020 at 07:54:52AM -0800, Matthew Wilcox wrote:
> On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote:
> > On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote:
> > > btrfs: Convert from readpages to readahead
> > >
> > > Implement the new readahead method in btrfs. Add a readahead_page_batch()
> > > to optimise fetching a batch of pages at once.
> >
> > Shouldn't this readahead_page_batch heper go into a separate patch so
> > that it clearly stands out?
>
> I'll move it into 'Put readahead pages in cache earlier' for v8 (the
> same patch where we add readahead_page())
One argument for keeping it in a patch of its own is that btrfs appears
to be the only user, and Goldwyn has a WIP conversion of btrfs to iomap,
so it might go away pretty soon and we could just revert the commit.
But this starts to get into really minor details, so I'll shut up now :)
^ permalink raw reply
* Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 9/9] i915: Exercise I915_CONTEXT_PARAM_RINGSIZE
From: Janusz Krzysztofik @ 2020-02-20 15:57 UTC (permalink / raw)
To: Chris Wilson; +Cc: igt-dev, intel-gfx
In-Reply-To: <157529875955.27263.14886639874940144583@skylake-alporthouse-com>
Hi Chris,
On Monday, December 2, 2019 3:59:19 PM CET Chris Wilson wrote:
> Quoting Janusz Krzysztofik (2019-12-02 14:42:58)
> > Hi Chris,
> >
> > I have a few questions rather than comments. I hope they are worth spending
> > your time.
> >
> > On Wednesday, November 13, 2019 1:52:40 PM CET Chris Wilson wrote:
> > > I915_CONTEXT_PARAM_RINGSIZE specifies how large to create the command
> > > ringbuffer for logical ring contects. This directly affects the number
> >
> > s/contects/contexts/
> >
> > > of batches userspace can submit before blocking waiting for space.
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Have you got this patch still queued somewhere? As UMD has accepted the
solution and are ready with changes on their side, I think we need to merge it
soon, and the kernel side as well.
Thanks,
Janusz
> > > ---
> > > tests/Makefile.sources | 3 +
> > > tests/i915/gem_ctx_ringsize.c | 296 ++++++++++++++++++++++++++++++++++
> > > tests/meson.build | 1 +
> > > 3 files changed, 300 insertions(+)
> > > create mode 100644 tests/i915/gem_ctx_ringsize.c
> > >
> > > diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> > > index e17d43155..801fc52f3 100644
> > > --- a/tests/Makefile.sources
> > > +++ b/tests/Makefile.sources
> > > @@ -163,6 +163,9 @@ gem_ctx_param_SOURCES = i915/gem_ctx_param.c
> > > TESTS_progs += gem_ctx_persistence
> > > gem_ctx_persistence_SOURCES = i915/gem_ctx_persistence.c
> > >
> > > +TESTS_progs += gem_ctx_ringsize
> > > +gem_ctx_ringsize_SOURCES = i915/gem_ctx_ringsize.c
> > > +
> > > TESTS_progs += gem_ctx_shared
> > > gem_ctx_shared_SOURCES = i915/gem_ctx_shared.c
> > >
> > > diff --git a/tests/i915/gem_ctx_ringsize.c b/tests/i915/gem_ctx_ringsize.c
> > > new file mode 100644
> > > index 000000000..1450e8f0d
> > > --- /dev/null
> > > +++ b/tests/i915/gem_ctx_ringsize.c
> > > @@ -0,0 +1,296 @@
> > > +/*
> > > + * Copyright © 2019 Intel Corporation
> > > + *
> > > + * Permission is hereby granted, free of charge, to any person obtaining a
> > > + * copy of this software and associated documentation files (the "Software"),
> > > + * to deal in the Software without restriction, including without limitation
> > > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > > + * and/or sell copies of the Software, and to permit persons to whom the
> > > + * Software is furnished to do so, subject to the following conditions:
> > > + *
> > > + * The above copyright notice and this permission notice (including the next
> > > + * paragraph) shall be included in all copies or substantial portions of the
> > > + * Software.
> > > + *
> > > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> > > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> > > + * IN THE SOFTWARE.
> > > + */
> > > +
> > > +#include <errno.h>
> > > +#include <fcntl.h>
> > > +#include <inttypes.h>
> > > +#include <sys/ioctl.h>
> > > +#include <sys/types.h>
> > > +#include <unistd.h>
> > > +
> > > +#include "drmtest.h" /* gem_quiescent_gpu()! */
> > > +#include "i915/gem_context.h"
> > > +#include "i915/gem_engine_topology.h"
> > > +#include "ioctl_wrappers.h" /* gem_wait()! */
> > > +#include "sw_sync.h"
> > > +
> > > +#define I915_CONTEXT_PARAM_RINGSIZE 0xc
> >
> > How are we going to handle symbol redefinition conflict which arises as soon
> > as this symbol is also included from kernel headers (e.g. via
> > "i915/gem_engine_topology.h")?
>
> Final version we copy the headers form the kernel. Conflicts remind us
> when we forget.
>
> >
> > > +
> > > +static bool has_ringsize(int i915)
> > > +{
> > > + struct drm_i915_gem_context_param p = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + };
> > > +
> > > + return __gem_context_get_param(i915, &p) == 0;
> > > +}
> > > +
> > > +static void test_idempotent(int i915)
> > > +{
> > > + struct drm_i915_gem_context_param p = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + };
> > > + uint32_t saved;
> > > +
> > > + /*
> > > + * Simple test to verify that we are able to read back the same
> > > + * value as we set.
> > > + */
> > > +
> > > + gem_context_get_param(i915, &p);
> > > + saved = p.value;
> > > +
> > > + for (uint32_t x = 1 << 12; x <= 128 << 12; x <<= 1) {
> >
> > I've noticed you are using two different notations for those minimum/maximum
> > constants. I think that may be confusing. How about defining and using
> > macros?
>
> A range in pages...
>
> > > + p.value = x;
> > > + gem_context_set_param(i915, &p);
> > > + gem_context_get_param(i915, &p);
> > > + igt_assert_eq_u32(p.value, x);
> > > + }
> > > +
> > > + p.value = saved;
> > > + gem_context_set_param(i915, &p);
> > > +}
> > > +
> > > +static void test_invalid(int i915)
> > > +{
> > > + struct drm_i915_gem_context_param p = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + };
> > > + uint64_t invalid[] = {
> > > + 0, 1, 4095, 4097, 8191, 8193,
> > > + /* upper limit may be HW dependent, atm it is 512KiB */
> > > + (512 << 10) - 1, (512 << 10) + 1,
> >
> > Here is an example of that different notation mentioned above.
>
> And here written in KiB to match comments.
>
> >
> > > + -1, -1u
> > > + };
> > > + uint32_t saved;
> > > +
> > > + gem_context_get_param(i915, &p);
> > > + saved = p.value;
> > > +
> > > + for (int i = 0; i < ARRAY_SIZE(invalid); i++) {
> > > + p.value = invalid[i];
> > > + igt_assert_eq(__gem_context_set_param(i915, &p), -EINVAL);
> > > + gem_context_get_param(i915, &p);
> > > + igt_assert_eq_u64(p.value, saved);
> > > + }
> > > +}
> > > +
> > > +static int create_ext_ioctl(int i915,
> > > + struct drm_i915_gem_context_create_ext *arg)
> > > +{
> > > + int err;
> > > +
> > > + err = 0;
> > > + if (igt_ioctl(i915, DRM_IOCTL_I915_GEM_CONTEXT_CREATE_EXT, arg)) {
> > > + err = -errno;
> > > + igt_assume(err);
> > > + }
> > > +
> > > + errno = 0;
> > > + return err;
> > > +}
> >
> > This helper looks like pretty standard for me. Why there are no library
> > functions for such generic operations?
>
> Because no one has written that yet.
>
> >
> > > +
> > > +static void test_create(int i915)
> > > +{
> > > + struct drm_i915_gem_context_create_ext_setparam p = {
> > > + .base = {
> > > + .name = I915_CONTEXT_CREATE_EXT_SETPARAM,
> > > + .next_extension = 0, /* end of chain */
> > > + },
> > > + .param = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + .value = 512 << 10,
> > > + }
> > > + };
> > > + struct drm_i915_gem_context_create_ext create = {
> > > + .flags = I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
> > > + .extensions = to_user_pointer(&p),
> > > + };
> > > +
> > > + igt_assert_eq(create_ext_ioctl(i915, &create), 0);
> > > +
> > > + p.param.ctx_id = create.ctx_id;
> > > + p.param.value = 0;
> > > + gem_context_get_param(i915, &p.param);
> > > + igt_assert_eq(p.param.value, 512 << 10);
> > > +
> > > + gem_context_destroy(i915, create.ctx_id);
> > > +}
> > > +
> > > +static void test_clone(int i915)
> > > +{
> > > + struct drm_i915_gem_context_create_ext_setparam p = {
> > > + .base = {
> > > + .name = I915_CONTEXT_CREATE_EXT_SETPARAM,
> > > + .next_extension = 0, /* end of chain */
> > > + },
> > > + .param = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + .value = 512 << 10,
> > > + }
> > > + };
> > > + struct drm_i915_gem_context_create_ext create = {
> > > + .flags = I915_CONTEXT_CREATE_FLAGS_USE_EXTENSIONS,
> > > + .extensions = to_user_pointer(&p),
> > > + };
> > > +
> > > + igt_assert_eq(create_ext_ioctl(i915, &create), 0);
> > > +
> > > + p.param.ctx_id = gem_context_clone(i915, create.ctx_id,
> > > + I915_CONTEXT_CLONE_ENGINES, 0);
> > > + igt_assert_neq(p.param.ctx_id, create.ctx_id);
> > > + gem_context_destroy(i915, create.ctx_id);
> > > +
> > > + p.param.value = 0;
> > > + gem_context_get_param(i915, &p.param);
> > > + igt_assert_eq(p.param.value, 512 << 10);
> > > +
> > > + gem_context_destroy(i915, p.param.ctx_id);
> > > +}
> > > +
> > > +static int __execbuf(int i915, struct drm_i915_gem_execbuffer2 *execbuf)
> > > +{
> > > + int err;
> > > +
> > > + err = 0;
> > > + if (ioctl(i915, DRM_IOCTL_I915_GEM_EXECBUFFER2, execbuf))
> > > + err = -errno;
> > > +
> > > + errno = 0;
> > > + return err;
> > > +}
> >
> > The above helper looks pretty the same as lib/ioctlwrappers.c:__gem_execbuf().
> > Does igt_assume(err) found in the latter matter so much that you use your own
> > version?
>
> It's very, very different from that one.
>
> > > +
> > > +static uint32_t __batch_create(int i915, uint32_t offset)
> >
> > This is always called with offset = 0, do we expect other values to be used
> > later?
>
> Why not.
>
> > > +{
> > > + const uint32_t bbe = 0xa << 23;
> > > + uint32_t handle;
> > > +
> > > + handle = gem_create(i915, ALIGN(offset + sizeof(bbe), 4096));
> >
> > Why don't we rely on the driver making the alignment for us?
>
> I'm used to being inside the kernel where it's expected to be correct.
>
> > > + gem_write(i915, handle, offset, &bbe, sizeof(bbe));
> > > +
> > > + return handle;
> > > +}
> > > +
> > > +static uint32_t batch_create(int i915)
> > > +{
> > > + return __batch_create(i915, 0);
> > > +}
> > > +
> > > +static unsigned int measure_inflight(int i915, unsigned int engine)
> > > +{
> > > + IGT_CORK_FENCE(cork);
> > > + struct drm_i915_gem_exec_object2 obj = {
> > > + .handle = batch_create(i915)
> > > + };
> > > + struct drm_i915_gem_execbuffer2 execbuf = {
> > > + .buffers_ptr = to_user_pointer(&obj),
> > > + .buffer_count = 1,
> > > + .flags = engine | I915_EXEC_FENCE_IN,
> > > + .rsvd2 = igt_cork_plug(&cork, i915),
> > > + };
> > > + unsigned int count;
> > > +
> > > + fcntl(i915, F_SETFL, fcntl(i915, F_GETFL) | O_NONBLOCK);
> > > +
> > > + gem_execbuf(i915, &execbuf);
> > > + for (count = 1; __execbuf(i915, &execbuf) == 0; count++)
> > > + ;
> >
> > Shouldn't we check if the reason for the failure is what we expect, i.e.,
> > -EWOULDBLOCK (or -EINTR)? And why don't we put a time constraint on that loop
> > in case O_NONBLOCK handling is not supported (yet)?
>
> Sure. The idea is that O_NONBLOCK is supported, otherwise we don't
> have fast and precise feedback.
>
> > > +static void test_resize(int i915,
> > > + const struct intel_execution_engine2 *e,
> > > + unsigned int flags)
> > > +#define IDLE (1 << 0)
> > > +{
> > > + struct drm_i915_gem_context_param p = {
> > > + .param = I915_CONTEXT_PARAM_RINGSIZE,
> > > + };
> > > + unsigned int prev[2] = {};
> > > + uint32_t saved;
> > > +
> > > + gem_context_get_param(i915, &p);
> > > + saved = p.value;
> > > +
> > > + gem_quiescent_gpu(i915);
> > > + for (p.value = 1 << 12; p.value <= 128 << 12; p.value <<= 1) {
> > > + unsigned int count;
> > > +
> > > + gem_context_set_param(i915, &p);
> > > +
> > > + count = measure_inflight(i915, e->flags);
> > > + igt_info("%s: %llx -> %d\n", e->name, p.value, count);
> > > + igt_assert(count > 3 * (prev[1] - prev[0]) / 4 + prev[1]);
> >
> > Where does this formula come from? Why not just count == 2 * prev[1] ?
> > What results should we expect in "active" vs. "idle" mode?
>
> I've explained somewhere why it is not 2*prev... And there's a small
> amount of imprecision (+-1 request). In test_resize is the comment:
>
> /*
> * The ringsize directly affects the number of batches we can have
> * inflight -- when we run out of room in the ring, the client is
> * blocked (or if O_NONBLOCK is specified, -EWOULDBLOCK is reported).
> * The kernel throttles the client when they enter the last 4KiB page,
> * so as we double the size of the ring, we nearly double the number
> * of requests we can fit as 2^n-1: i.e 0, 1, 3, 7, 15, 31 pages.
> */
>
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply
* Re: [PATCH v7 14/24] btrfs: Convert from readpages to readahead
From: Christoph Hellwig @ 2020-02-20 15:57 UTC (permalink / raw)
To: Matthew Wilcox
Cc: cluster-devel@redhat.com, Johannes Thumshirn,
linux-kernel@vger.kernel.org,
linux-f2fs-devel@lists.sourceforge.net, Christoph Hellwig,
linux-mm@kvack.org, ocfs2-devel@oss.oracle.com,
linux-fsdevel@vger.kernel.org, linux-ext4@vger.kernel.org,
linux-erofs@lists.ozlabs.org, linux-xfs@vger.kernel.org,
linux-btrfs@vger.kernel.org
In-Reply-To: <20200220155452.GX24185@bombadil.infradead.org>
On Thu, Feb 20, 2020 at 07:54:52AM -0800, Matthew Wilcox wrote:
> On Thu, Feb 20, 2020 at 07:46:58AM -0800, Christoph Hellwig wrote:
> > On Thu, Feb 20, 2020 at 05:48:49AM -0800, Matthew Wilcox wrote:
> > > btrfs: Convert from readpages to readahead
> > >
> > > Implement the new readahead method in btrfs. Add a readahead_page_batch()
> > > to optimise fetching a batch of pages at once.
> >
> > Shouldn't this readahead_page_batch heper go into a separate patch so
> > that it clearly stands out?
>
> I'll move it into 'Put readahead pages in cache earlier' for v8 (the
> same patch where we add readahead_page())
One argument for keeping it in a patch of its own is that btrfs appears
to be the only user, and Goldwyn has a WIP conversion of btrfs to iomap,
so it might go away pretty soon and we could just revert the commit.
But this starts to get into really minor details, so I'll shut up now :)
^ permalink raw reply
* Re: [PATCH 0/1] selinux: Add xfs quota command types
From: Darrick J. Wong @ 2020-02-20 15:57 UTC (permalink / raw)
To: Richard Haines; +Cc: sds, paul, linux-xfs, selinux
In-Reply-To: <20200220153234.152426-1-richard_c_haines@btinternet.com>
On Thu, Feb 20, 2020 at 03:32:33PM +0000, Richard Haines wrote:
> Added these quota command types for SELinux checks on XFS quotas. I picked
> those I thought useful. The selinux-testsuite will have tests for these
> permission checks on XFS.
>
> One point to note: XFS does not call dquot_quota_on() to trigger
> security_quota_on(), therefore the 'file quotaon' permission is not tested
> for SELinux
Is that a feature or a bug? :)
(It sounds like a bug to me, but let's see if anyone complains...)
--D
> Richard Haines (1):
> selinux: Add xfs quota command types
>
> security/selinux/hooks.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> --
> 2.24.1
>
^ permalink raw reply
* Re: [Xen-devel] [PATCH] rwlock: allow recursive read locking when already locked in write mode
From: Roger Pau Monné @ 2020-02-20 15:57 UTC (permalink / raw)
To: Jan Beulich
Cc: Jürgen Groß, Stefano Stabellini, Julien Grall, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
xen-devel
In-Reply-To: <9f82c3cb-862d-87bd-7a01-4812be98eedd@suse.com>
On Thu, Feb 20, 2020 at 04:50:22PM +0100, Jan Beulich wrote:
> On 20.02.2020 16:17, Roger Pau Monné wrote:
> > On Thu, Feb 20, 2020 at 04:02:55PM +0100, Jan Beulich wrote:
> >> On 20.02.2020 15:11, Roger Pau Monné wrote:
> >>> On Thu, Feb 20, 2020 at 01:48:54PM +0100, Jan Beulich wrote:
> >>>> Another option is to use the recurse_cpu field of the
> >>>> associated spin lock: The field is used for recursive locks
> >>>> only, and hence the only conflict would be with
> >>>> _spin_is_locked(), which we don't (and in the future then
> >>>> also shouldn't) use on this lock.
> >>>
> >>> I looked into that also, but things get more complicated AFAICT, as it's
> >>> not possible to atomically fetch the state of the lock and the owner
> >>> CPU at the same time. Neither you could set the LOCKED bit and the CPU
> >>> at the same time.
> >>
> >> There's no need to atomically fetch both afaics: The field is
> >> valid only if the LOCKED bit is set. And when reading the
> >> field you only care about the value being equal to
> >> smp_processor_id(), i.e. it is fine to set LOCKED before
> >> updating the CPU field on lock, and to reset the CPU field to
> >> SPINLOCK_NO_CPU (or whatever it's called) before clearing
> >> LOCKED.
> >
> > Yes that would require the usage of a sentinel value as 0 would be a
> > valid processor ID.
> >
> > I would try to refrain from (abu)sing internal spinlock fields, as I
> > think we can solve this just by using the cnts field on the current
> > rwlock implementation.
> >
> > What issue do you have in mind that would prevent storing the CPU
> > write locked in the cnts field?
>
> The reduction of the number of bits used for other purposes.
I think it should be fine for now, as we would support systems with up
to 16384 CPU IDs, and 65536 concurrent read lockers, which mean each
CPU could take the lock up to 4 times recursively. Note that
supporting 16384 CPUs is still very, very far off the radar.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* Re: [igt-dev] [PATCH i-g-t] intel-ci: add a pre-merge blacklist to reduce the testing queue
From: Chris Wilson @ 2020-02-20 15:58 UTC (permalink / raw)
To: Martin Peres, igt-dev
In-Reply-To: <20200220153209.210767-1-martin.peres@linux.intel.com>
Quoting Martin Peres (2020-02-20 15:32:09)
> +###############################################################################
> +# These 2 tests are stressing the re-usability of objects. It does not look
> +# like we have had issues with this outside of the gen7 ppgtt issue, which
> +# does not counterbalance its overall execution time.
> +#
> +# - shard-skl: 2% (~5 minutes)
> +# - shard-kbl: 1% (~1.5 minutes)
> +# - shard-apl: 1.7% (~3 minutes)
> +# - shard-glk: 1% (2.5 minutes)
> +# - shard-icl: 0.5% (1 minute)
> +# - shard-tgl: 0.5% (1 minute)
> +#
> +# Data acquired on 2020-02-20 by Martin Peres
> +###############################################################################
> +igt@gem_exec_reuse@(baggage|contexts)
File a bug; there's no reason why this should be greatly exceeding it's
5s runtime. So presumably we waste too much time in the setup/teardown.
-Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply
* [PATCH RESEND v6 05/16] arc/mm: Use helper fault_signal_pending()
From: Peter Xu @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Peter Xu, Martin Cracauer, Mike Rapoport, Hugh Dickins,
Jerome Glisse, Kirill A . Shutemov, Matthew Wilcox,
Pavel Emelyanov, Brian Geffon, Maya Gokhale, Denis Plotnikov,
Andrea Arcangeli, Johannes Weiner, Dr . David Alan Gilbert,
Linus Torvalds, Mike Kravetz, Marty McFadden, David Hildenbrand,
Bobby Powers, Mel Gorman
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
Let ARC to use the new helper fault_signal_pending() by moving the
signal check out of the retry logic as standalone. This should also
helps to simplify the code a bit.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/arc/mm/fault.c | 34 +++++++++++++---------------------
1 file changed, 13 insertions(+), 21 deletions(-)
diff --git a/arch/arc/mm/fault.c b/arch/arc/mm/fault.c
index fb86bc3e9b35..6eb821a59b49 100644
--- a/arch/arc/mm/fault.c
+++ b/arch/arc/mm/fault.c
@@ -133,29 +133,21 @@ void do_page_fault(unsigned long address, struct pt_regs *regs)
fault = handle_mm_fault(vma, address, flags);
+ /* Quick path to respond to signals */
+ if (fault_signal_pending(fault, regs)) {
+ if (!user_mode(regs))
+ goto no_context;
+ return;
+ }
+
/*
- * Fault retry nuances
+ * Fault retry nuances, mmap_sem already relinquished by core mm
*/
- if (unlikely(fault & VM_FAULT_RETRY)) {
-
- /*
- * If fault needs to be retried, handle any pending signals
- * first (by returning to user mode).
- * mmap_sem already relinquished by core mm for RETRY case
- */
- if (fatal_signal_pending(current)) {
- if (!user_mode(regs))
- goto no_context;
- return;
- }
- /*
- * retry state machine
- */
- if (flags & FAULT_FLAG_ALLOW_RETRY) {
- flags &= ~FAULT_FLAG_ALLOW_RETRY;
- flags |= FAULT_FLAG_TRIED;
- goto retry;
- }
+ if (unlikely((fault & VM_FAULT_RETRY) &&
+ (flags & FAULT_FLAG_ALLOW_RETRY))) {
+ flags &= ~FAULT_FLAG_ALLOW_RETRY;
+ flags |= FAULT_FLAG_TRIED;
+ goto retry;
}
bad_area:
--
2.24.1
^ permalink raw reply related
* [Xen-devel] [PATCH] MAINTAINERS: make Roger VPCI maintainer
From: Wei Liu @ 2020-02-20 15:58 UTC (permalink / raw)
To: Xen Development List
Cc: Stefano Stabellini, Julien Grall, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Jan Beulich,
Roger Pau Monné
Roger has kindly agreed to take on the burden.
Signed-off-by: Wei Liu <wl@xen.org>
---
MAINTAINERS | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 46424c1833..e7d1d5a88a 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -500,6 +500,12 @@ F: xen/include/*/vm_event.h
F: xen/include/asm-x86/hvm/monitor.h
F: xen/include/asm-x86/hvm/vm_event.h
+VPCI
+M: Roger Pau Monné <roger.pau@citrix.com>
+S: Supported
+F: xen/drivers/vpci
+F: xen/include/xen/vpci.h
+
VTPM
M: Daniel De Graaf <dgdegra@tycho.nsa.gov>
M: Quan Xu <quan.xu0@gmail.com>
--
2.20.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related
* [PATCH 1/8] platform/chrome: cros_ec_proto: Report command not supported
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
In-Reply-To: <20200220155859.906647-1-enric.balletbo@collabora.com>
In practice most drivers that use the EC protocol what really care is if
the result was successful or not, hence, we introduced a
cros_ec_cmd_xfer_status() function that converts EC errors to standard
Linux error codes. On some few cases, though, we are interested on know
if the command is supported or not, and in such cases, just ignore the
error. To achieve this, return a -ENOTSUPP error when the command is not
supported.
This will allow us to finish the conversion of all users to use the
cros_ec_cmd_xfer_status() function instead of cros_ec_cmd_xfer() and
make the latest private to the protocol driver, so users of the protocol
are not confused in which function they should use.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_proto.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 3cfa643f1d07..3e745e0fe092 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -553,7 +553,10 @@ EXPORT_SYMBOL(cros_ec_cmd_xfer);
* replied with success status. It's not necessary to check msg->result when
* using this function.
*
- * Return: The number of bytes transferred on success or negative error code.
+ * Return:
+ * >=0 - The number of bytes transferred
+ * -ENOTSUPP - Operation not supported
+ * -EPROTO - Protocol error
*/
int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
struct cros_ec_command *msg)
@@ -563,6 +566,10 @@ int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
ret = cros_ec_cmd_xfer(ec_dev, msg);
if (ret < 0) {
dev_err(ec_dev->dev, "Command xfer error (err:%d)\n", ret);
+ } else if (msg->result == EC_RES_INVALID_VERSION) {
+ dev_dbg(ec_dev->dev, "Command invalid version (err:%d)\n",
+ msg->result);
+ return -ENOTSUPP;
} else if (msg->result != EC_RES_SUCCESS) {
dev_dbg(ec_dev->dev, "Command result (err: %d)\n", msg->result);
return -EPROTO;
--
2.25.0
^ permalink raw reply related
* [PATCH 3/8] platform/chrome: cros_ec_vbc: Use cros_ec_cmd_xfer_status helper
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
In-Reply-To: <20200220155859.906647-1-enric.balletbo@collabora.com>
This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_vbc.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_vbc.c b/drivers/platform/chrome/cros_ec_vbc.c
index 8edae465105c..46482d12cffe 100644
--- a/drivers/platform/chrome/cros_ec_vbc.c
+++ b/drivers/platform/chrome/cros_ec_vbc.c
@@ -40,7 +40,7 @@ static ssize_t vboot_context_read(struct file *filp, struct kobject *kobj,
msg->outsize = para_sz;
msg->insize = resp_sz;
- err = cros_ec_cmd_xfer(ecdev, msg);
+ err = cros_ec_cmd_xfer_status(ecdev, msg);
if (err < 0) {
dev_err(dev, "Error sending read request: %d\n", err);
kfree(msg);
@@ -83,7 +83,7 @@ static ssize_t vboot_context_write(struct file *filp, struct kobject *kobj,
msg->outsize = para_sz;
msg->insize = 0;
- err = cros_ec_cmd_xfer(ecdev, msg);
+ err = cros_ec_cmd_xfer_status(ecdev, msg);
if (err < 0) {
dev_err(dev, "Error sending write request: %d\n", err);
kfree(msg);
--
2.25.0
^ permalink raw reply related
* [PATCH 7/8] platform/chrome: cros_ec: Use cros_ec_cmd_xfer_status helper
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
In-Reply-To: <20200220155859.906647-1-enric.balletbo@collabora.com>
This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/platform/chrome/cros_ec.c b/drivers/platform/chrome/cros_ec.c
index 6fc8f2c3ac51..e179411bdfbe 100644
--- a/drivers/platform/chrome/cros_ec.c
+++ b/drivers/platform/chrome/cros_ec.c
@@ -120,7 +120,7 @@ static int cros_ec_sleep_event(struct cros_ec_device *ec_dev, u8 sleep_event)
buf.msg.command = EC_CMD_HOST_SLEEP_EVENT;
- ret = cros_ec_cmd_xfer(ec_dev, &buf.msg);
+ ret = cros_ec_cmd_xfer_status(ec_dev, &buf.msg);
/* For now, report failure to transition to S0ix with a warning. */
if (ret >= 0 && ec_dev->host_sleep_v1 &&
--
2.25.0
^ permalink raw reply related
* [PATCH 5/8] platform/chrome: cros_ec_sysfs: Use cros_ec_cmd_xfer_status helper
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
In-Reply-To: <20200220155859.906647-1-enric.balletbo@collabora.com>
This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). In this case the change is trivial and the only
reason to do it is because we want to make cros_ec_cmd_xfer() a private
function for the EC protocol and let people only use the
cros_ec_cmd_xfer_status() to return Linux standard error codes.
Looking at the code I am even unsure that makes sense differentiate
these two errors but let's not change the behaviour for now.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_sysfs.c | 36 ++++++++++++-------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_sysfs.c b/drivers/platform/chrome/cros_ec_sysfs.c
index 07dac97ad57c..d45ea5d5bfa4 100644
--- a/drivers/platform/chrome/cros_ec_sysfs.c
+++ b/drivers/platform/chrome/cros_ec_sysfs.c
@@ -149,14 +149,14 @@ static ssize_t version_show(struct device *dev,
/* Get build info. */
msg->command = EC_CMD_GET_BUILD_INFO + ec->cmd_offset;
msg->insize = EC_HOST_PARAM_SIZE;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
- if (ret < 0)
- count += scnprintf(buf + count, PAGE_SIZE - count,
- "Build info: XFER ERROR %d\n", ret);
- else if (msg->result != EC_RES_SUCCESS)
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ if (ret == -EPROTO) {
count += scnprintf(buf + count, PAGE_SIZE - count,
"Build info: EC error %d\n", msg->result);
- else {
+ } else if (ret < 0) {
+ count += scnprintf(buf + count, PAGE_SIZE - count,
+ "Build info: XFER ERROR %d\n", ret);
+ } else {
msg->data[EC_HOST_PARAM_SIZE - 1] = '\0';
count += scnprintf(buf + count, PAGE_SIZE - count,
"Build info: %s\n", msg->data);
@@ -165,14 +165,14 @@ static ssize_t version_show(struct device *dev,
/* Get chip info. */
msg->command = EC_CMD_GET_CHIP_INFO + ec->cmd_offset;
msg->insize = sizeof(*r_chip);
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
- if (ret < 0)
- count += scnprintf(buf + count, PAGE_SIZE - count,
- "Chip info: XFER ERROR %d\n", ret);
- else if (msg->result != EC_RES_SUCCESS)
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ if (ret == -EPROTO) {
count += scnprintf(buf + count, PAGE_SIZE - count,
"Chip info: EC error %d\n", msg->result);
- else {
+ } else if (ret < 0) {
+ count += scnprintf(buf + count, PAGE_SIZE - count,
+ "Chip info: XFER ERROR %d\n", ret);
+ } else {
r_chip = (struct ec_response_get_chip_info *)msg->data;
r_chip->vendor[sizeof(r_chip->vendor) - 1] = '\0';
@@ -189,14 +189,14 @@ static ssize_t version_show(struct device *dev,
/* Get board version */
msg->command = EC_CMD_GET_BOARD_VERSION + ec->cmd_offset;
msg->insize = sizeof(*r_board);
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
- if (ret < 0)
- count += scnprintf(buf + count, PAGE_SIZE - count,
- "Board version: XFER ERROR %d\n", ret);
- else if (msg->result != EC_RES_SUCCESS)
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ if (ret == -EPROTO) {
count += scnprintf(buf + count, PAGE_SIZE - count,
"Board version: EC error %d\n", msg->result);
- else {
+ } else if (ret < 0) {
+ count += scnprintf(buf + count, PAGE_SIZE - count,
+ "Board version: XFER ERROR %d\n", ret);
+ } else {
r_board = (struct ec_response_board_version *)msg->data;
count += scnprintf(buf + count, PAGE_SIZE - count,
--
2.25.0
^ permalink raw reply related
* [PATCH] tcg: gdbstub: Fix single-step issue on arm target
From: Changbin Du @ 2020-02-20 15:58 UTC (permalink / raw)
To: alex.bennee, philmd; +Cc: qemu-devel, Changbin Du
Recently when debugging an arm32 system on qemu, I found sometimes the
single-step command (stepi) is not working. This can be reproduced by
below steps:
1) start qemu-system-arm -s -S .. and wait for gdb connection.
2) start gdb and connect to qemu. In my case, gdb gets a wrong value
(0x60) for PC.
3) After connected, type 'stepi' and expect it will stop at next ins.
But, it has never stopped. This because:
1) We doesn't report ‘vContSupported’ feature to gdb explicitly and gdb
think we do not support it. In this case, gdb use a software breakpoint
to emulate single-step.
2) Since gdb gets a wrong initial value of PC, then gdb inserts a
breakpoint to wrong place (PC+4).
Since we do support ‘vContSupported’ query command, so let's tell gdb that
we support it.
Before this change, gdb send below 'Z0' packet to implement single-step:
gdb_handle_packet: Z0,4,4
After this change, gdb send "vCont;s.." which is expected:
gdb_handle_packet: vCont?
put_packet: vCont;c;C;s;S
gdb_handle_packet: vCont;s:p1.1;c:p1.-1
Signed-off-by: Changbin Du <changbin.du@gmail.com>
---
gdbstub.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gdbstub.c b/gdbstub.c
index ce304ff482..adccd938e2 100644
--- a/gdbstub.c
+++ b/gdbstub.c
@@ -2111,7 +2111,7 @@ static void handle_query_supported(GdbCmdContext *gdb_ctx, void *user_ctx)
gdb_ctx->s->multiprocess = true;
}
- pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), ";multiprocess+");
+ pstrcat(gdb_ctx->str_buf, sizeof(gdb_ctx->str_buf), ";vContSupported+;multiprocess+");
put_packet(gdb_ctx->s, gdb_ctx->str_buf);
}
--
2.25.0
^ permalink raw reply related
* [PATCH 8/8] platform/chrome: cros_ec_proto: Do not export cros_ec_cmd_xfer()
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel
Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani,
Jonathan Cameron, Enrico Granata, Andy Shevchenko, Lee Jones,
Pi-Hsun Shih, Evan Green
In-Reply-To: <20200220155859.906647-1-enric.balletbo@collabora.com>
Now that all the remaining users of cros_ec_cmd_xfer() has been removed,
make this function private to the cros_ec_proto module.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_proto.c | 5 ++---
include/linux/platform_data/cros_ec_proto.h | 3 ---
2 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_proto.c b/drivers/platform/chrome/cros_ec_proto.c
index 3e745e0fe092..11a2db7cd0f7 100644
--- a/drivers/platform/chrome/cros_ec_proto.c
+++ b/drivers/platform/chrome/cros_ec_proto.c
@@ -496,8 +496,8 @@ EXPORT_SYMBOL(cros_ec_query_all);
*
* Return: 0 on success or negative error code.
*/
-int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
- struct cros_ec_command *msg)
+static int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
+ struct cros_ec_command *msg)
{
int ret;
@@ -541,7 +541,6 @@ int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
return ret;
}
-EXPORT_SYMBOL(cros_ec_cmd_xfer);
/**
* cros_ec_cmd_xfer_status() - Send a command to the ChromeOS EC.
diff --git a/include/linux/platform_data/cros_ec_proto.h b/include/linux/platform_data/cros_ec_proto.h
index ba5914770191..1334fedc07cb 100644
--- a/include/linux/platform_data/cros_ec_proto.h
+++ b/include/linux/platform_data/cros_ec_proto.h
@@ -212,9 +212,6 @@ int cros_ec_prepare_tx(struct cros_ec_device *ec_dev,
int cros_ec_check_result(struct cros_ec_device *ec_dev,
struct cros_ec_command *msg);
-int cros_ec_cmd_xfer(struct cros_ec_device *ec_dev,
- struct cros_ec_command *msg);
-
int cros_ec_cmd_xfer_status(struct cros_ec_device *ec_dev,
struct cros_ec_command *msg);
--
2.25.0
^ permalink raw reply related
* [PATCH RESEND v6 06/16] arm64/mm: Use helper fault_signal_pending()
From: Peter Xu @ 2020-02-20 15:59 UTC (permalink / raw)
To: linux-mm, linux-kernel
Cc: Peter Xu, Martin Cracauer, Mike Rapoport, Hugh Dickins,
Jerome Glisse, Kirill A . Shutemov, Matthew Wilcox,
Pavel Emelyanov, Brian Geffon, Maya Gokhale, Denis Plotnikov,
Andrea Arcangeli, Johannes Weiner, Dr . David Alan Gilbert,
Linus Torvalds, Mike Kravetz, Marty McFadden, David Hildenbrand,
Bobby Powers, Mel Gorman
In-Reply-To: <20200220155353.8676-1-peterx@redhat.com>
Let the arm64 fault handling to use the new fault_signal_pending()
helper, by moving the signal handling out of the retry logic.
Signed-off-by: Peter Xu <peterx@redhat.com>
---
arch/arm64/mm/fault.c | 19 +++++++------------
1 file changed, 7 insertions(+), 12 deletions(-)
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 85566d32958f..6f4b69d712b1 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -513,19 +513,14 @@ static int __kprobes do_page_fault(unsigned long addr, unsigned int esr,
fault = __do_page_fault(mm, addr, mm_flags, vm_flags);
major |= fault & VM_FAULT_MAJOR;
- if (fault & VM_FAULT_RETRY) {
- /*
- * If we need to retry but a fatal signal is pending,
- * handle the signal first. We do not need to release
- * the mmap_sem because it would already be released
- * in __lock_page_or_retry in mm/filemap.c.
- */
- if (fatal_signal_pending(current)) {
- if (!user_mode(regs))
- goto no_context;
- return 0;
- }
+ /* Quick path to respond to signals */
+ if (fault_signal_pending(fault, regs)) {
+ if (!user_mode(regs))
+ goto no_context;
+ return 0;
+ }
+ if (fault & VM_FAULT_RETRY) {
/*
* Clear FAULT_FLAG_ALLOW_RETRY to avoid any risk of
* starvation.
--
2.24.1
^ permalink raw reply related
* [PATCH 6/8] platform/chrome: cros_ec_lightbar: Use cros_ec_cmd_xfer_status helper
From: Enric Balletbo i Serra @ 2020-02-20 15:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Collabora Kernel ML, groeck, bleung, dtor, gwendal, pmalani
In-Reply-To: <20200220155859.906647-1-enric.balletbo@collabora.com>
This patch makes use of cros_ec_cmd_xfer_status() instead of
cros_ec_cmd_xfer(). It allows us to remove some redundand code. In this
case, though, we are changing a bit the behaviour because of returning
-EINVAL on protocol error we propagate the error return for
cros_ec_cmd_xfer_status() function, but I think it will be fine, even
more clear as we don't mask the Linux error code.
Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
drivers/platform/chrome/cros_ec_lightbar.c | 50 ++++++----------------
1 file changed, 13 insertions(+), 37 deletions(-)
diff --git a/drivers/platform/chrome/cros_ec_lightbar.c b/drivers/platform/chrome/cros_ec_lightbar.c
index b4c110c5fee0..b59180bff5a3 100644
--- a/drivers/platform/chrome/cros_ec_lightbar.c
+++ b/drivers/platform/chrome/cros_ec_lightbar.c
@@ -116,7 +116,7 @@ static int get_lightbar_version(struct cros_ec_dev *ec,
param = (struct ec_params_lightbar *)msg->data;
param->cmd = LIGHTBAR_CMD_VERSION;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0) {
ret = 0;
goto exit;
@@ -193,15 +193,10 @@ static ssize_t brightness_store(struct device *dev,
if (ret)
goto exit;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto exit;
- if (msg->result != EC_RES_SUCCESS) {
- ret = -EINVAL;
- goto exit;
- }
-
ret = count;
exit:
kfree(msg);
@@ -258,13 +253,10 @@ static ssize_t led_rgb_store(struct device *dev, struct device_attribute *attr,
goto exit;
}
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto exit;
- if (msg->result != EC_RES_SUCCESS)
- goto exit;
-
i = 0;
ok = 1;
}
@@ -305,14 +297,13 @@ static ssize_t sequence_show(struct device *dev,
if (ret)
goto exit;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
- if (ret < 0)
- goto exit;
-
- if (msg->result != EC_RES_SUCCESS) {
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
+ if (ret == -EPROTO) {
ret = scnprintf(buf, PAGE_SIZE,
"ERROR: EC returned %d\n", msg->result);
goto exit;
+ } else if (ret < 0) {
+ goto exit;
}
resp = (struct ec_response_lightbar *)msg->data;
@@ -344,13 +335,10 @@ static int lb_send_empty_cmd(struct cros_ec_dev *ec, uint8_t cmd)
if (ret)
goto error;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto error;
- if (msg->result != EC_RES_SUCCESS) {
- ret = -EINVAL;
- goto error;
- }
+
ret = 0;
error:
kfree(msg);
@@ -377,13 +365,10 @@ static int lb_manual_suspend_ctrl(struct cros_ec_dev *ec, uint8_t enable)
if (ret)
goto error;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto error;
- if (msg->result != EC_RES_SUCCESS) {
- ret = -EINVAL;
- goto error;
- }
+
ret = 0;
error:
kfree(msg);
@@ -425,15 +410,10 @@ static ssize_t sequence_store(struct device *dev, struct device_attribute *attr,
if (ret)
goto exit;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto exit;
- if (msg->result != EC_RES_SUCCESS) {
- ret = -EINVAL;
- goto exit;
- }
-
ret = count;
exit:
kfree(msg);
@@ -487,13 +467,9 @@ static ssize_t program_store(struct device *dev, struct device_attribute *attr,
*/
msg->outsize = count + extra_bytes;
- ret = cros_ec_cmd_xfer(ec->ec_dev, msg);
+ ret = cros_ec_cmd_xfer_status(ec->ec_dev, msg);
if (ret < 0)
goto exit;
- if (msg->result != EC_RES_SUCCESS) {
- ret = -EINVAL;
- goto exit;
- }
ret = count;
exit:
--
2.25.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.