* Re: semantics of rhashtable and sysvipc
From: Linus Torvalds @ 2018-05-23 18:52 UTC (permalink / raw)
To: Davidlohr Bueso
Cc: Thomas Graf, Herbert Xu, Andrew Morton, Manfred Spraul,
guillaume.knispel, Linux API, Linux Kernel Mailing List
In-Reply-To: <20180523183103.cdgv4slom62y56wi@linux-n805>
On Wed, May 23, 2018 at 11:47 AM Davidlohr Bueso <dave@stgolabs.net> wrote:
> Note that even if the allocation was guaranteed, there are still param
validations
> and rhashtable_init() can return -EINVAL.
So?
It's not going to happen, because you're not going to give garbage
parameters.
Why would you add a BUG_ON() for something that cannot happen? You might as
well sprinkle them randomly in every damn place.
And even if somebody screws up the parameters because they are being
stupid, then SO WHAT? rhashtable_init() won't initialize the pointers, and
we'll get a NULL pointer dereference.
And hey, we'll probably get it later during boot, once the system is
actually up and running, and that NULL pointer dereference might even get
logged in the system logs now because the machine booted successfully, and
mnaybe it will even get sent to a distro and debugged.
So at what point was there _any_ advantage in doing a BUG_ON() for a crazy
case?
Really.
Linus
^ permalink raw reply
* Re: semantics of rhashtable and sysvipc
From: Davidlohr Bueso @ 2018-05-23 18:52 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Graf, Herbert Xu, Andrew Morton, Manfred Spraul,
guillaume.knispel, Linux API, Linux Kernel Mailing List
In-Reply-To: <CA+55aFwJtj5ucfx3T-g9VnNgWG7EqHS9+oFMSv8oyv11Yaa2UQ@mail.gmail.com>
On Wed, 23 May 2018, Linus Torvalds wrote:
>On Wed, May 23, 2018 at 11:47 AM Davidlohr Bueso <dave@stgolabs.net> wrote:
>
>> Note that even if the allocation was guaranteed, there are still param
>validations
>> and rhashtable_init() can return -EINVAL.
>
>So?
>
>It's not going to happen, because you're not going to give garbage
>parameters.
Maybe EINVAL could be replaced with WARN_ON(). That would grab the programmer's
attention.
>
>Why would you add a BUG_ON() for something that cannot happen? You might as
>well sprinkle them randomly in every damn place.
Not suggesting this. Before I started the thread, I was actually thinking of
ipc using ENOMEM only for rhashtable_init() filure considering the EINVAL case
will never happen.
>
>And even if somebody screws up the parameters because they are being
>stupid, then SO WHAT? rhashtable_init() won't initialize the pointers, and
>we'll get a NULL pointer dereference.
>
>And hey, we'll probably get it later during boot, once the system is
>actually up and running, and that NULL pointer dereference might even get
>logged in the system logs now because the machine booted successfully, and
>mnaybe it will even get sent to a distro and debugged.
>
>So at what point was there _any_ advantage in doing a BUG_ON() for a crazy
>case?
For the record, I'm not arguing in favor of BUG_ON().
^ permalink raw reply
* Re: semantics of rhashtable and sysvipc
From: Linus Torvalds @ 2018-05-23 18:35 UTC (permalink / raw)
To: Davidlohr Bueso, Thomas Graf, Herbert Xu
Cc: Andrew Morton, Manfred Spraul, guillaume.knispel, Linux API,
Linux Kernel Mailing List
In-Reply-To: <20180523172500.anfvmjtumww65ief@linux-n805>
On Wed, May 23, 2018 at 10:41 AM Davidlohr Bueso <dave@stgolabs.net> wrote:
> The second alternative would be to add a BUG_ON() if the initialization
fails
> and we get rid of all the tables_initialized hack.
I see absolutely no value in an early boot BUG_ON().
Either we know the allocation cannot fail - which is perfectly fine at
bootup, and is a common pattern - or it can fail and we need to handle it.
In neither case is the BUG_ON() appropriate.
So I'm perfectly fine with getting rid of 'tables_initialized'. But no, not
with a BUG_ON().
If you cannot guarantee that the allocation works (using __GFP_NOFAIL is
ok, for example - but it only works with small allocations), then you need
to handle the allocation failure.
I refuse to see more of the shit-for-brains kind of "I can't be bothered to
handle error cases" BUG_ON() stuff.
And I also am not in the least interested in "this cannot possibly happen"
BUG_ON() code.
One option is to make rhashtable_alloc() shrink the allocation and try
again if it fails, and then you *can* do __GFP_NOFAIL eventually.
In fact, it can validly be argued that rhashtable_init() is just buggy
as-is. The whole *point* olf that function is to size things appropriately,
and returning -ENOMEM obviously means that it didn't do its job.
Linus
^ permalink raw reply
* Re: semantics of rhashtable and sysvipc
From: Davidlohr Bueso @ 2018-05-23 18:31 UTC (permalink / raw)
To: Linus Torvalds
Cc: Thomas Graf, Herbert Xu, Andrew Morton, Manfred Spraul,
guillaume.knispel, Linux API, Linux Kernel Mailing List
In-Reply-To: <CA+55aFwAtfiMa22-OFGf1dNR8CNzSKjjLLj5UL-HqgapV7Tf1A@mail.gmail.com>
On Wed, 23 May 2018, Linus Torvalds wrote:
>So I'm perfectly fine with getting rid of 'tables_initialized'. But no, not
>with a BUG_ON().
>
>If you cannot guarantee that the allocation works (using __GFP_NOFAIL is
>ok, for example - but it only works with small allocations), then you need
>to handle the allocation failure.
Note that even if the allocation was guaranteed, there are still param validations
and rhashtable_init() can return -EINVAL.
^ permalink raw reply
* Re: [PATCH v2 3/4] mm: add find_alloc_contig_pages() interface
From: Reinette Chatre @ 2018-05-23 18:07 UTC (permalink / raw)
To: Vlastimil Babka, Mike Kravetz, linux-mm, linux-kernel, linux-api
Cc: Michal Hocko, Christopher Lameter, Guy Shattah, Anshuman Khandual,
Michal Nazarewicz, David Nellans, Laura Abbott, Pavel Machek,
Dave Hansen, Andrew Morton
In-Reply-To: <01793788-1870-858e-2061-a0e6ef3a3171@suse.cz>
Hi Vlastimil,
On 5/23/2018 4:18 AM, Vlastimil Babka wrote:
> On 05/22/2018 06:41 PM, Reinette Chatre wrote:
>> On 5/21/2018 4:48 PM, Mike Kravetz wrote:
>>> I'm guessing that most (?all?) allocations will be order based. The use
>>> cases I am aware of (hugetlbfs, Intel Cache Pseudo-Locking, RDMA) are all
>>> order based. However, as commented in previous version taking arbitrary
>>> nr_pages makes interface more future proof.
>>>
>>
>> I noticed this Cache Pseudo-Locking statement and would like to clarify.
>> I have not been following this thread in detail so I would like to
>> apologize first if my comments are out of context.
>>
>> Currently the Cache Pseudo-Locking allocations are order based because I
>> assumed it was required by the allocator. The contiguous regions needed
>> by Cache Pseudo-Locking will not always be order based - instead it is
>> based on the granularity of the cache allocation. One example is a
>> platform with 55MB L3 cache that can be divided into 20 equal portions.
>> To support Cache Pseudo-Locking on this platform we need to be able to
>> allocate contiguous regions at increments of 2816KB (the size of each
>> portion). In support of this example platform regions needed would thus
>> be 2816KB, 5632KB, 8448KB, etc.
>
> Will there be any alignment requirements for these allocations e.g. for
> minimizing conflict misses?
Two views on the usage of the allocated memory are: On the user space
side, the kernel memory is mapped to userspace (using remap_pfn_range())
and thus need to be page aligned. On the kernel side the memory is
loaded into the cache and it is here where the requirement originates
for it to be contiguous. The memory being contiguous reduces the
likelihood of physical addresses from the allocated memory mapping to
the same cache line and thus cause cache evictions of memory we are
trying to load into the cache.
I hope I answered your question, if not, please let me know which parts
I missed and I will try again.
Reinette
^ permalink raw reply
* Re: semantics of rhashtable and sysvipc
From: Davidlohr Bueso @ 2018-05-23 17:35 UTC (permalink / raw)
To: akpm, torvalds; +Cc: manfred, guillaume.knispel, linux-api, linux-kernel
In-Reply-To: <20180523172500.anfvmjtumww65ief@linux-n805>
On Wed, 23 May 2018, Davidlohr Bueso wrote:
>I see two possible fixes.
I guess a third option would be to make the hashtable static, but I'm not
against using rhashtables so I'm not really considering this.
^ permalink raw reply
* semantics of rhashtable and sysvipc
From: Davidlohr Bueso @ 2018-05-23 17:25 UTC (permalink / raw)
To: akpm, torvalds; +Cc: manfred, guillaume.knispel, linux-api, linux-kernel
Hi,
In sysvipc we have an ids->tables_initialized regarding the rhashtable,
introduced in 0cfb6aee70b (ipc: optimize semget/shmget/msgget for lots of keys).
It's there, specifically, to prevent nil pointer dereferences, from using an
uninitialized api. Considering how rhashtable_init() can fail (probably due to
ENOMEM, if anything), this made the overall ipc initialization capable of
failure as well. That alone is ugly, but fine, however I've spotted a few
issues regarding the semantics of tables_initialized (however unlikely they
may be):
- There is inconsistency in what we return to userspace: ipc_addid() returns
ENOSPC which is certainly _wrong_, while ipc_obtain_object_idr() returns
EINVAL.
- After we started using rhashtables, ipc_findkey() can return nil upon
!tables_initialized, but the caller expects nil for when the ipc structure
isn't found, and can therefore call into ipcget() callbacks.
I see two possible fixes. The first is to return the proper error code
if !tables_initialized, however, I'm not sure how we want to deal with the
EINVAL cases when rhashtable_init() fails. Userspace has no reason to know
about this. The ENOMEM case I guess makes sense ok.
The second alternative would be to add a BUG_ON() if the initialization fails
and we get rid of all the tables_initialized hack. I know Linus isn't fond
of this, and in the past ipc has gotten rid of BUG_ON usage in ipc because
of this. However I mention it because there are other core areas that do this
(or call panic() straightaway). Ie in networking: sock_diag_init() and
netlink_proto_init().
Thoughts?
Thanks,
Davidlohr
^ permalink raw reply
* Re: [PATCH v5 20/28] fpga: dfl: add fpga bridge platform driver for FME
From: Wu Hao @ 2018-05-23 15:28 UTC (permalink / raw)
To: Alan Tull
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer
In-Reply-To: <CANk1AXS3M=GyTtdk=YYMadD22VqDPbB_XRYrnoue6nXrNQqqkQ@mail.gmail.com>
On Wed, May 23, 2018 at 10:15:00AM -0500, Alan Tull wrote:
> On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
>
> Hi Hao,
>
> > This patch adds fpga bridge platform driver for FPGA Management Engine.
> > It implements the enable_set callback for fpga bridge.
> >
> > Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> > Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> > Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> > Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> > Signed-off-by: Wu Hao <hao.wu@intel.com>
> > Acked-by: Alan Tull <atull@kernel.org>
> > Acked-by: Moritz Fischer <mdf@kernel.org>
> > ---
> > v3: rename driver to fpga-dfl-fme-br
> > remove useless dev_dbg in probe function.
> > rebased due to fpga api change.
> > v4: rename to dfl-fme-br and fix SPDX license issue
> > include dfl-fme-pr.h instead of dfl-fme.h
> > add Acked-by from Alan and Moritz
> > v5: rebase due to API changes.
> > defer port and its ops finding when really need.
> > ---
> > drivers/fpga/Kconfig | 6 +++
> > drivers/fpga/Makefile | 1 +
> > drivers/fpga/dfl-fme-br.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 121 insertions(+)
> > create mode 100644 drivers/fpga/dfl-fme-br.c
> >
> > diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> > index 89f76e8..a8f939a 100644
> > --- a/drivers/fpga/Kconfig
> > +++ b/drivers/fpga/Kconfig
> > @@ -156,6 +156,12 @@ config FPGA_DFL_FME_MGR
> > help
> > Say Y to enable FPGA Manager driver for FPGA Management Engine.
> >
> > +config FPGA_DFL_FME_BRIDGE
> > + tristate "FPGA DFL FME Bridge Driver"
> > + depends on FPGA_DFL_FME
> > + help
> > + Say Y to enable FPGA Bridge driver for FPGA Management Engine.
> > +
> > config FPGA_DFL_PCI
> > tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> > depends on PCI && FPGA_DFL
> > diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> > index f82814a..75096e9 100644
> > --- a/drivers/fpga/Makefile
> > +++ b/drivers/fpga/Makefile
> > @@ -32,6 +32,7 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> > obj-$(CONFIG_FPGA_DFL) += dfl.o
> > obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> > obj-$(CONFIG_FPGA_DFL_FME_MGR) += dfl-fme-mgr.o
> > +obj-$(CONFIG_FPGA_DFL_FME_BRIDGE) += dfl-fme-br.o
> >
> > dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
> >
> > diff --git a/drivers/fpga/dfl-fme-br.c b/drivers/fpga/dfl-fme-br.c
> > new file mode 100644
> > index 0000000..5c51b08
> > --- /dev/null
> > +++ b/drivers/fpga/dfl-fme-br.c
> > @@ -0,0 +1,114 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * FPGA Bridge Driver for FPGA Management Engine (FME)
> > + *
> > + * Copyright (C) 2017 Intel Corporation, Inc.
> > + *
> > + * Authors:
> > + * Wu Hao <hao.wu@intel.com>
> > + * Joseph Grecco <joe.grecco@intel.com>
> > + * Enno Luebbers <enno.luebbers@intel.com>
> > + * Tim Whisonant <tim.whisonant@intel.com>
> > + * Ananda Ravuri <ananda.ravuri@intel.com>
> > + * Henry Mitchel <henry.mitchel@intel.com>
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/fpga/fpga-bridge.h>
> > +
> > +#include "dfl.h"
> > +#include "dfl-fme-pr.h"
> > +
> > +struct fme_br_priv {
> > + struct dfl_fme_br_pdata *pdata;
> > + struct dfl_fpga_port_ops *port_ops;
> > + struct platform_device *port_pdev;
> > +};
> > +
> > +static int fme_bridge_enable_set(struct fpga_bridge *bridge, bool enable)
> > +{
> > + struct fme_br_priv *priv = bridge->priv;
> > + struct platform_device *port_pdev;
> > + struct dfl_fpga_port_ops *ops;
> > +
> > + if (!priv->port_pdev) {
> > + port_pdev = dfl_fpga_cdev_find_port(priv->pdata->cdev,
> > + &priv->pdata->port_id,
> > + dfl_fpga_check_port_id);
> > + if (!port_pdev)
> > + return -ENODEV;
> > +
> > + priv->port_pdev = port_pdev;
> > + }
> > +
> > + if (priv->port_pdev && !priv->port_ops) {
> > + ops = dfl_fpga_get_port_ops(priv->port_pdev);
> > + if (!ops || !ops->enable_set)
> > + return -ENOENT;
> > +
> > + priv->port_ops = ops;
> > + }
>
> This is saving some pointers. Is it possible that the port_pdev or
> port_ops could go away?
Hi Alan
Thanks for the comments.
The find_port function will get the port device to prevent that.
You can see put device in remove function. And it's similar for the
port ops. In dfl_fpga_get_port_ops function, it will prevent unexpected
port ops removing by try module get.
>
> Also, the port ops routines probably should be named
> dfl_fpga_port_ops_get/find_port/etc
>
Hm.. as I see there are functions named as get_device(), put_device(), so
I just name it as ..._get_port_ops and ..._put_port_ops in similar way. :)
If you think that could be a better name, it's fine for me to change them.
Thanks
Hao
> Alan
>
> > +
> > + return priv->port_ops->enable_set(priv->port_pdev, enable);
> > +}
> > +
> > +static const struct fpga_bridge_ops fme_bridge_ops = {
> > + .enable_set = fme_bridge_enable_set,
> > +};
> > +
> > +static int fme_br_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct fme_br_priv *priv;
> > + struct fpga_bridge *br;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->pdata = dev_get_platdata(dev);
> > +
> > + br = fpga_bridge_create(dev, "DFL FPGA FME Bridge",
> > + &fme_bridge_ops, priv);
> > + if (!br)
> > + return -ENOMEM;
> > +
> > + platform_set_drvdata(pdev, br);
> > +
> > + ret = fpga_bridge_register(br);
> > + if (ret)
> > + fpga_bridge_free(br);
> > +
> > + return ret;
> > +}
> > +
> > +static int fme_br_remove(struct platform_device *pdev)
> > +{
> > + struct fpga_bridge *br = platform_get_drvdata(pdev);
> > + struct fme_br_priv *priv = br->priv;
> > +
> > + fpga_bridge_unregister(br);
> > +
> > + if (priv->port_pdev)
> > + put_device(&priv->port_pdev->dev);
> > + if (priv->port_ops)
> > + dfl_fpga_put_port_ops(priv->port_ops);
> > +
> > + return 0;
> > +}
> > +
> > +static struct platform_driver fme_br_driver = {
> > + .driver = {
> > + .name = DFL_FPGA_FME_BRIDGE,
> > + },
> > + .probe = fme_br_probe,
> > + .remove = fme_br_remove,
> > +};
> > +
> > +module_platform_driver(fme_br_driver);
> > +
> > +MODULE_DESCRIPTION("FPGA Bridge for DFL FPGA Management Engine");
> > +MODULE_AUTHOR("Intel Corporation");
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:dfl-fme-bridge");
> > --
> > 1.8.3.1
> >
^ permalink raw reply
* Re: [PATCH v5 20/28] fpga: dfl: add fpga bridge platform driver for FME
From: Alan Tull @ 2018-05-23 15:15 UTC (permalink / raw)
To: Wu Hao
Cc: Moritz Fischer, linux-fpga, linux-kernel, linux-api, Kang, Luwei,
Zhang, Yi Z, Tim Whisonant, Enno Luebbers, Shiva Rao,
Christopher Rauer
In-Reply-To: <1525229431-3087-21-git-send-email-hao.wu@intel.com>
On Tue, May 1, 2018 at 9:50 PM, Wu Hao <hao.wu@intel.com> wrote:
Hi Hao,
> This patch adds fpga bridge platform driver for FPGA Management Engine.
> It implements the enable_set callback for fpga bridge.
>
> Signed-off-by: Tim Whisonant <tim.whisonant@intel.com>
> Signed-off-by: Enno Luebbers <enno.luebbers@intel.com>
> Signed-off-by: Shiva Rao <shiva.rao@intel.com>
> Signed-off-by: Christopher Rauer <christopher.rauer@intel.com>
> Signed-off-by: Wu Hao <hao.wu@intel.com>
> Acked-by: Alan Tull <atull@kernel.org>
> Acked-by: Moritz Fischer <mdf@kernel.org>
> ---
> v3: rename driver to fpga-dfl-fme-br
> remove useless dev_dbg in probe function.
> rebased due to fpga api change.
> v4: rename to dfl-fme-br and fix SPDX license issue
> include dfl-fme-pr.h instead of dfl-fme.h
> add Acked-by from Alan and Moritz
> v5: rebase due to API changes.
> defer port and its ops finding when really need.
> ---
> drivers/fpga/Kconfig | 6 +++
> drivers/fpga/Makefile | 1 +
> drivers/fpga/dfl-fme-br.c | 114 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 121 insertions(+)
> create mode 100644 drivers/fpga/dfl-fme-br.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index 89f76e8..a8f939a 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -156,6 +156,12 @@ config FPGA_DFL_FME_MGR
> help
> Say Y to enable FPGA Manager driver for FPGA Management Engine.
>
> +config FPGA_DFL_FME_BRIDGE
> + tristate "FPGA DFL FME Bridge Driver"
> + depends on FPGA_DFL_FME
> + help
> + Say Y to enable FPGA Bridge driver for FPGA Management Engine.
> +
> config FPGA_DFL_PCI
> tristate "FPGA Device Feature List (DFL) PCIe Device Driver"
> depends on PCI && FPGA_DFL
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index f82814a..75096e9 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -32,6 +32,7 @@ obj-$(CONFIG_OF_FPGA_REGION) += of-fpga-region.o
> obj-$(CONFIG_FPGA_DFL) += dfl.o
> obj-$(CONFIG_FPGA_DFL_FME) += dfl-fme.o
> obj-$(CONFIG_FPGA_DFL_FME_MGR) += dfl-fme-mgr.o
> +obj-$(CONFIG_FPGA_DFL_FME_BRIDGE) += dfl-fme-br.o
>
> dfl-fme-objs := dfl-fme-main.o dfl-fme-pr.o
>
> diff --git a/drivers/fpga/dfl-fme-br.c b/drivers/fpga/dfl-fme-br.c
> new file mode 100644
> index 0000000..5c51b08
> --- /dev/null
> +++ b/drivers/fpga/dfl-fme-br.c
> @@ -0,0 +1,114 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FPGA Bridge Driver for FPGA Management Engine (FME)
> + *
> + * Copyright (C) 2017 Intel Corporation, Inc.
> + *
> + * Authors:
> + * Wu Hao <hao.wu@intel.com>
> + * Joseph Grecco <joe.grecco@intel.com>
> + * Enno Luebbers <enno.luebbers@intel.com>
> + * Tim Whisonant <tim.whisonant@intel.com>
> + * Ananda Ravuri <ananda.ravuri@intel.com>
> + * Henry Mitchel <henry.mitchel@intel.com>
> + */
> +
> +#include <linux/module.h>
> +#include <linux/fpga/fpga-bridge.h>
> +
> +#include "dfl.h"
> +#include "dfl-fme-pr.h"
> +
> +struct fme_br_priv {
> + struct dfl_fme_br_pdata *pdata;
> + struct dfl_fpga_port_ops *port_ops;
> + struct platform_device *port_pdev;
> +};
> +
> +static int fme_bridge_enable_set(struct fpga_bridge *bridge, bool enable)
> +{
> + struct fme_br_priv *priv = bridge->priv;
> + struct platform_device *port_pdev;
> + struct dfl_fpga_port_ops *ops;
> +
> + if (!priv->port_pdev) {
> + port_pdev = dfl_fpga_cdev_find_port(priv->pdata->cdev,
> + &priv->pdata->port_id,
> + dfl_fpga_check_port_id);
> + if (!port_pdev)
> + return -ENODEV;
> +
> + priv->port_pdev = port_pdev;
> + }
> +
> + if (priv->port_pdev && !priv->port_ops) {
> + ops = dfl_fpga_get_port_ops(priv->port_pdev);
> + if (!ops || !ops->enable_set)
> + return -ENOENT;
> +
> + priv->port_ops = ops;
> + }
This is saving some pointers. Is it possible that the port_pdev or
port_ops could go away?
Also, the port ops routines probably should be named
dfl_fpga_port_ops_get/find_port/etc
Alan
> +
> + return priv->port_ops->enable_set(priv->port_pdev, enable);
> +}
> +
> +static const struct fpga_bridge_ops fme_bridge_ops = {
> + .enable_set = fme_bridge_enable_set,
> +};
> +
> +static int fme_br_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct fme_br_priv *priv;
> + struct fpga_bridge *br;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->pdata = dev_get_platdata(dev);
> +
> + br = fpga_bridge_create(dev, "DFL FPGA FME Bridge",
> + &fme_bridge_ops, priv);
> + if (!br)
> + return -ENOMEM;
> +
> + platform_set_drvdata(pdev, br);
> +
> + ret = fpga_bridge_register(br);
> + if (ret)
> + fpga_bridge_free(br);
> +
> + return ret;
> +}
> +
> +static int fme_br_remove(struct platform_device *pdev)
> +{
> + struct fpga_bridge *br = platform_get_drvdata(pdev);
> + struct fme_br_priv *priv = br->priv;
> +
> + fpga_bridge_unregister(br);
> +
> + if (priv->port_pdev)
> + put_device(&priv->port_pdev->dev);
> + if (priv->port_ops)
> + dfl_fpga_put_port_ops(priv->port_ops);
> +
> + return 0;
> +}
> +
> +static struct platform_driver fme_br_driver = {
> + .driver = {
> + .name = DFL_FPGA_FME_BRIDGE,
> + },
> + .probe = fme_br_probe,
> + .remove = fme_br_remove,
> +};
> +
> +module_platform_driver(fme_br_driver);
> +
> +MODULE_DESCRIPTION("FPGA Bridge for DFL FPGA Management Engine");
> +MODULE_AUTHOR("Intel Corporation");
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:dfl-fme-bridge");
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH 07/24] arm64: ilp32: add documentation on the ILP32 ABI for ARM64
From: Pavel Machek @ 2018-05-23 14:06 UTC (permalink / raw)
To: Yury Norov
Cc: Catalin Marinas, Arnd Bergmann, linux-arm-kernel, linux-kernel,
linux-doc, linux-arch, linux-api, Adam Borowski, Alexander Graf,
Alexey Klimov, Andreas Schwab, Andrew Pinski, Bamvor Zhangjian,
Chris Metcalf, Christoph Muellner, Dave Martin, David S . Miller,
Florian Weimer, Geert Uytterhoeven, Heiko Carstens,
James Hogan <jame>
In-Reply-To: <20180516081910.10067-8-ynorov@caviumnetworks.com>
[-- Attachment #1: Type: text/plain, Size: 1129 bytes --]
On Wed 2018-05-16 11:18:52, Yury Norov wrote:
> Based on Andrew Pinski's patch-series.
>
> Signed-off-by: Yury Norov <ynorov@caviumnetworks.com>
So Andrew's signoff should be here?
> ---
> Documentation/arm64/ilp32.txt | 45 +++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/arm64/ilp32.txt
>
> diff --git a/Documentation/arm64/ilp32.txt b/Documentation/arm64/ilp32.txt
> new file mode 100644
> index 000000000000..d0fd5109c4b2
> --- /dev/null
> +++ b/Documentation/arm64/ilp32.txt
> @@ -0,0 +1,45 @@
> +ILP32 AARCH64 SYSCALL ABI
> +=========================
> +
> +This document describes the ILP32 syscall ABI and where it differs
> +from the generic compat linux syscall interface.
I was hoping to learn what ILP32 is / what is it good for, but no,
this does not tell me... it would be good to do a short explanation
here, and maybe reference it from cover letter of the series...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH v2 3/4] mm: add find_alloc_contig_pages() interface
From: Vlastimil Babka @ 2018-05-23 11:18 UTC (permalink / raw)
To: Reinette Chatre, Mike Kravetz, linux-mm, linux-kernel, linux-api
Cc: Michal Hocko, Christopher Lameter, Guy Shattah, Anshuman Khandual,
Michal Nazarewicz, David Nellans, Laura Abbott, Pavel Machek,
Dave Hansen, Andrew Morton
In-Reply-To: <c7972da1-a908-7550-7253-9de9a963174c@intel.com>
On 05/22/2018 06:41 PM, Reinette Chatre wrote:
> On 5/21/2018 4:48 PM, Mike Kravetz wrote:
>> I'm guessing that most (?all?) allocations will be order based. The use
>> cases I am aware of (hugetlbfs, Intel Cache Pseudo-Locking, RDMA) are all
>> order based. However, as commented in previous version taking arbitrary
>> nr_pages makes interface more future proof.
>>
>
> I noticed this Cache Pseudo-Locking statement and would like to clarify.
> I have not been following this thread in detail so I would like to
> apologize first if my comments are out of context.
>
> Currently the Cache Pseudo-Locking allocations are order based because I
> assumed it was required by the allocator. The contiguous regions needed
> by Cache Pseudo-Locking will not always be order based - instead it is
> based on the granularity of the cache allocation. One example is a
> platform with 55MB L3 cache that can be divided into 20 equal portions.
> To support Cache Pseudo-Locking on this platform we need to be able to
> allocate contiguous regions at increments of 2816KB (the size of each
> portion). In support of this example platform regions needed would thus
> be 2816KB, 5632KB, 8448KB, etc.
Will there be any alignment requirements for these allocations e.g. for
minimizing conflict misses?
Vlastimil
^ permalink raw reply
* Re: [v8, bpf-next, 4/9] net/wireless/iwlwifi: fix iwlwifi_dev_ucode_error tracepoint
From: Johannes Berg @ 2018-05-23 11:03 UTC (permalink / raw)
To: Alexei Starovoitov, davem-fT/PcQaiUtIeIZ0/mPfg9Q
Cc: daniel-FeC+5ew28dpmcu3hnIyYJQ,
torvalds-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b,
peterz-wEGCiKHe2LqWVfeAwA7xHQ, rostedt-nx8X9YLhiw1AfugRpC6u6w,
mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w,
netdev-u79uwXL29TY76Z2rM5mHXA, kernel-team-b10kYP2dOMg,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-wireless-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20180328190540.370956-5-ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
On Wed, 2018-03-28 at 12:05 -0700, Alexei Starovoitov wrote:
> fix iwlwifi_dev_ucode_error tracepoint to pass pointer to a table
> instead of all 17 arguments by value.
> dvm/main.c and mvm/utils.c have 'struct iwl_error_event_table'
> defined with very similar yet subtly different fields and offsets.
> tracepoint is still common and using definition of 'struct iwl_error_event_table'
> from dvm/commands.h while copying fields.
> Long term this tracepoint probably should be split into two.
It would've been nice to CC the wireless list for wireless related
patches ...
> --- a/drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c
> +++ b/drivers/net/wireless/intel/iwlwifi/iwl-devtrace.c
> @@ -30,6 +30,7 @@
> #ifndef __CHECKER__
> #include "iwl-trans.h"
>
> +#include "dvm/commands.h"
In particular, this breaks the whole driver abstraction.
> +++ b/drivers/net/wireless/intel/iwlwifi/mvm/utils.c
> @@ -549,12 +549,7 @@ static void iwl_mvm_dump_lmac_error_log(struct iwl_mvm *mvm, u32 base)
>
> IWL_ERR(mvm, "Loaded firmware version: %s\n", mvm->fw->fw_version);
>
> - trace_iwlwifi_dev_ucode_error(trans->dev, table.error_id, table.tsf_low,
> - table.data1, table.data2, table.data3,
> - table.blink2, table.ilink1,
> - table.ilink2, table.bcon_time, table.gp1,
> - table.gp2, table.fw_rev_type, table.major,
> - table.minor, table.hw_ver, table.brd_ver);
> + trace_iwlwifi_dev_ucode_error(trans->dev, &table, table.hw_ver, table.brd_ver);
This is also utterly wrong because mvm has - for better or worse - a
different type "struct iwl_error_event_table" in this file ...
This really should never have gotten into the tree.
johannes
^ permalink raw reply
* Re: [PATCH 07/24] hibernate: Disable when the kernel is locked down
From: joeyli @ 2018-05-23 8:46 UTC (permalink / raw)
To: Jiri Kosina
Cc: Pavel Machek, David Howells, Linus Torvalds, linux-man, linux-api,
jmorris, linux-kernel, linux-security-module
In-Reply-To: <nycvar.YFH.7.76.1804261018030.28147@cbobk.fhfr.pm>
Hi experts,
Sorry for I missed this discussion...
On Thu, Apr 26, 2018 at 10:20:29AM +0200, Jiri Kosina wrote:
> On Thu, 26 Apr 2018, Pavel Machek wrote:
>
> > That's not how the crypto needs to work. Talk to Jiri Kosina, ok?
>
> Yeah, Joey Lee (adding to CC) implemented it here:
>
> https://lkml.org/lkml/2015/8/11/47
>
> I think there have been more respins, Joey definitely knows more details
> and status quo.
>
> The design is specifically tailored for secure-boot environments though.
>
I am working on the next version of hibernation encryption and authentication:
https://github.com/joeyli/linux-s4sign/wiki
My plan is:
- Hibernation encryption:
There is a draft patch to encrypt image by ctr(aes). This patch works
with the first version of hibernation verification:
https://github.com/joeyli/linux-s4sign/commit/6a9a0113bb221c036ebd0f6321b7191283fe4929
- Adapt hibernation to key retention service:
- Using the encrypted key to derive encrypt key and auth key to
encrypt and hmac snapshot image. Put the encrypted key in the image
header of snapshot.
- The encrypted key will be encrypted by KMK (kernel master key). Either
trusted key(sealed by TPM) or EFI key (explain in later) can be the KMK.
If there have appropriate UI support in initrd, user key can also be
the KMK.
- Similar with the enrolling EVM key, but more earler:
The systemd and dracut must be changed for enrolling kernel master key
before the swap partition be mounted.
- EFI key:
- A new master key type to key retention service.
- It can be a new option beyond trusted key(TPM) and user key.
- EFI stub generates a random key and stores in EFI boot service
variable:
- This random key in boot variable can be called ERK (EFI Root Key)
- The ERK is secure when secure boot enabled.
- User must aware and enable secure boot by themself if they want.
- ERK can be a secret to encrypt a random number for generate a EFI key
- The EFI key can be used by hibernation encryption/authentication.
- The EFI key can be a master key to generate a encrypted key for EVM.
- Rescue mechanism for ERK:
- The ERK may be regenerated after the old ERK be erased by firmware update
or firmware recovery.
- Current idea is using the public key in first/second trusted keyring
to encrypt the ERK for backup. User can enroll the EFI key with old ERK to
request kernel to re-encrypt the EFI key with new ERK.
Thanks a lot!
Joey Lee
^ permalink raw reply
* YAaioRace (was Re: [PATCH 08/31] aio: implement IOCB_CMD_POLL)
From: Al Viro @ 2018-05-23 1:43 UTC (permalink / raw)
To: Linus Torvalds
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel, Kent Overstreet, Christoph Hellwig
In-Reply-To: <20180523004904.GH30522@ZenIV.linux.org.uk>
On Wed, May 23, 2018 at 01:49:04AM +0100, Al Viro wrote:
> > Looks like we want to call ->ki_cancel() *BEFORE* removing from the list,
> > as well as doing fput() after aio_complete(). The same ordering, BTW, goes
> > for aio_read() et.al.
> >
> > Look:
> > CPU1: io_cancel() grabs ->ctx_lock, finds iocb and removes it from the list.
> > CPU2: aio_rw_complete() on that iocb. Since the sucker is not in the list
> > anymore, we do NOT spin on ->ctx_lock and proceed to free iocb
> > CPU1: pass freed iocb to ->ki_cancel(). BOOM.
>
> BTW, it seems that the mainline is vulnerable to this one. I might be
> missing something, but...
It is, but with a different attack vector - io_cancel(2) won't do it (it
does not remove from the list at all), but io_destroy(2) bloody well will.
IMO, we need this in mainline; unless somebody has a problem with it, to
#fixes it goes:
fix io_destroy()/aio_complete() race
If io_destroy() gets to cancelling everything that can be cancelled and
gets to kiocb_cancel() calling the function driver has left in ->ki_cancel,
it becomes vulnerable to a race with IO completion. At that point req
is already taken off the list and aio_complete() does *NOT* spin until
we (in free_ioctx_users()) releases ->ctx_lock. As the result, it proceeds
to kiocb_free(), freing req just it gets passed to ->ki_cancel().
Fix is simple - remove from the list after the call of kiocb_cancel(). All
instances of ->ki_cancel() already have to cope with the being called with
iocb still on list - that's what happens in io_cancel(2).
Cc: stable@kernel.org
Fixes: 0460fef2a921 "aio: use cancellation list lazily"
Signed-off-by: Al Viro <viro@zeniv.linux.org.uk>
---
diff --git a/fs/aio.c b/fs/aio.c
index 8061d9787e54..49f53516eef0 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -634,9 +634,8 @@ static void free_ioctx_users(struct percpu_ref *ref)
while (!list_empty(&ctx->active_reqs)) {
req = list_first_entry(&ctx->active_reqs,
struct aio_kiocb, ki_list);
-
- list_del_init(&req->ki_list);
kiocb_cancel(req);
+ list_del_init(&req->ki_list);
}
spin_unlock_irq(&ctx->ctx_lock);
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply related
* Re: [PATCH 08/31] aio: implement IOCB_CMD_POLL
From: Al Viro @ 2018-05-23 0:49 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180523004530.GG30522@ZenIV.linux.org.uk>
On Wed, May 23, 2018 at 01:45:30AM +0100, Al Viro wrote:
> Oh, bugger...
>
> wakeup
> removed from queue
> schedule __aio_poll_complete()
>
> cancel
> grab ctx->lock
> remove from list
> work
> aio_complete()
> check if it's in the list
> it isn't, move on to free the sucker
> cancel
> call ->ki_cancel()
> BOOM
>
> Looks like we want to call ->ki_cancel() *BEFORE* removing from the list,
> as well as doing fput() after aio_complete(). The same ordering, BTW, goes
> for aio_read() et.al.
>
> Look:
> CPU1: io_cancel() grabs ->ctx_lock, finds iocb and removes it from the list.
> CPU2: aio_rw_complete() on that iocb. Since the sucker is not in the list
> anymore, we do NOT spin on ->ctx_lock and proceed to free iocb
> CPU1: pass freed iocb to ->ki_cancel(). BOOM.
BTW, it seems that the mainline is vulnerable to this one. I might be
missing something, but...
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH 08/31] aio: implement IOCB_CMD_POLL
From: Al Viro @ 2018-05-23 0:45 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180522220524.GE30522@ZenIV.linux.org.uk>
On Tue, May 22, 2018 at 11:05:24PM +0100, Al Viro wrote:
> > +{
> > + struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, poll);
> > +
> > + fput(req->file);
> > + aio_complete(iocb, mangle_poll(mask), 0);
> > +}
>
> Careful.
>
> > +static int aio_poll_cancel(struct kiocb *iocb)
> > +{
> > + struct aio_kiocb *aiocb = container_of(iocb, struct aio_kiocb, rw);
> > + struct poll_iocb *req = &aiocb->poll;
> > + struct wait_queue_head *head = req->head;
> > + bool found = false;
> > +
> > + spin_lock(&head->lock);
> > + found = __aio_poll_remove(req);
> > + spin_unlock(&head->lock);
>
> What's to guarantee that req->head has not been freed by that point?
> Look: wakeup finds ->ctx_lock held, so it leaves the sucker on the
> list, removes it from queue and schedules the call of __aio_poll_complete().
> Which gets executed just as we hit aio_poll_cancel(), starting with fput().
>
> You really want to do aio_complete() before fput(). That way you know that
> req->wait is alive and well at least until iocb gets removed from the list.
Oh, bugger...
wakeup
removed from queue
schedule __aio_poll_complete()
cancel
grab ctx->lock
remove from list
work
aio_complete()
check if it's in the list
it isn't, move on to free the sucker
cancel
call ->ki_cancel()
BOOM
Looks like we want to call ->ki_cancel() *BEFORE* removing from the list,
as well as doing fput() after aio_complete(). The same ordering, BTW, goes
for aio_read() et.al.
Look:
CPU1: io_cancel() grabs ->ctx_lock, finds iocb and removes it from the list.
CPU2: aio_rw_complete() on that iocb. Since the sucker is not in the list
anymore, we do NOT spin on ->ctx_lock and proceed to free iocb
CPU1: pass freed iocb to ->ki_cancel(). BOOM.
and if we have fput() done first (in aio_rw_complete()) we are vulnerable to
CPU1: io_cancel() grabs ->ctx_lock, finds iocb and removes it from the list.
CPU2: aio_rw_complete() on that iocb. fput() done, opening us to rmmod.
CPU1: call ->ki_cancel(), which points to freed memory now. BOOM.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH v7 0/5] AIO add per-command iopriority
From: Adam Manzanares @ 2018-05-22 22:31 UTC (permalink / raw)
To: Jens Axboe, Al Viro
Cc: linux-fsdevel@vger.kernel.org, bcrl@kvack.org, mingo@kernel.org,
tglx@linutronix.de, kstewart@linuxfoundation.org,
peterz@infradead.org, pombredanne@nexb.com,
gregkh@linuxfoundation.org, bigeasy@linutronix.de,
rgoldwyn@suse.com, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-aio@kvack.org,
linux-api@vger.kernel.org, hch@infradread.org, jmoyer@redhat.com
In-Reply-To: <4b8732e8-1c51-3181-56a1-d03f7b229f63@kernel.dk>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1599 bytes --]
On 5/22/18 11:30 AM, Jens Axboe wrote:
> On 5/22/18 12:30 PM, Al Viro wrote:
>> On Tue, May 22, 2018 at 11:55:04AM -0600, Jens Axboe wrote:
>>> On 5/22/18 11:52 AM, adam.manzanares@wdc.com wrote:
>>>> From: Adam Manzanares <adam.manzanares@wdc.com>
>>>>
>>>> This is the per-I/O equivalent of the ioprio_set system call.
>>>> See the following link for performance implications on a SATA HDD:
>>>> https://lkml.org/lkml/2016/12/6/495
>>>>
>>>> First patch factors ioprio_check_cap function out of ioprio_set system call to
>>>> also be used by the aio ioprio interface.
>>>>
>>>> Second patch converts kiocb ki_hint field to a u16 to avoid kiocb bloat.
>>>>
>>>> Third patch passes ioprio hint from aio iocb to kiocb and initializes kiocb
>>>> ioprio value appropriately when it is not explicitly set.
>>>>
>>>> Fourth patch enables the feature for blkdev.
>>>>
>>>> Fifth patch enables the feature for iomap direct IO
>>>
>>> LGTM, you can add:
>>>
>>> Reviewed-by: Jens Axboe <axboe@kernel.dk>
>>>
>>> Al, are you picking this series up, or should I?
>>
>> Probably better if I do, once I finish reviewing Christoph's patchset -
>> we already have a bunch of stuff around fs/aio.c in this cycle...
>
> Alright, sounds good, thanks Al.
>
I was working on the man page update for this feature and noticed I
could be bit more informative on error if I return the error value
returned by ioprio_check_cap in fs/aio.c.
Should I resend the whole series?N‹§²æìr¸›zǧu©ž²Æ {\b†éì¹»\x1c®&Þ–)îŨ¨Š{ayº\x1dÊÚ&j:+v‰¨’öœ’Šà\x16Šæ¢·¢ú(œ¸§»\x10\b:Çž†Ûiÿü0ÂKÚrJ+ƒö¢£ðèž×¦j)Z†·Ÿ
^ permalink raw reply
* Re: aio poll and a new in-kernel poll API V12
From: Al Viro @ 2018-05-22 22:07 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180522113108.25713-1-hch@lst.de>
On Tue, May 22, 2018 at 01:30:37PM +0200, Christoph Hellwig wrote:
> Hi all,
>
> this series adds support for the IOCB_CMD_POLL operation to poll for the
> readyness of file descriptors using the aio subsystem. The API is based
> on patches that existed in RHAS2.1 and RHEL3, which means it already is
> supported by libaio. To implement the poll support efficiently new
> methods to poll are introduced in struct file_operations: get_poll_head
> and poll_mask. The first one returns a wait_queue_head to wait on
> (lifetime is bound by the file), and the second does a non-blocking
> check for the POLL* events. This allows aio poll to work without
> any additional context switches, unlike epoll.
I can live with that, modulo bug in #8 (see reply to that one). There's
some nitpicking, but that can be dealt with in followups.
^ permalink raw reply
* Re: [PATCH 08/31] aio: implement IOCB_CMD_POLL
From: Al Viro @ 2018-05-22 22:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Avi Kivity, linux-aio, linux-fsdevel, netdev, linux-api,
linux-kernel
In-Reply-To: <20180522113108.25713-9-hch@lst.de>
On Tue, May 22, 2018 at 01:30:45PM +0200, Christoph Hellwig wrote:
> +static inline void __aio_poll_complete(struct poll_iocb *req, __poll_t mask)
> +{
> + struct aio_kiocb *iocb = container_of(req, struct aio_kiocb, poll);
> +
> + fput(req->file);
> + aio_complete(iocb, mangle_poll(mask), 0);
> +}
Careful.
> +static int aio_poll_cancel(struct kiocb *iocb)
> +{
> + struct aio_kiocb *aiocb = container_of(iocb, struct aio_kiocb, rw);
> + struct poll_iocb *req = &aiocb->poll;
> + struct wait_queue_head *head = req->head;
> + bool found = false;
> +
> + spin_lock(&head->lock);
> + found = __aio_poll_remove(req);
> + spin_unlock(&head->lock);
What's to guarantee that req->head has not been freed by that point?
Look: wakeup finds ->ctx_lock held, so it leaves the sucker on the
list, removes it from queue and schedules the call of __aio_poll_complete().
Which gets executed just as we hit aio_poll_cancel(), starting with fput().
You really want to do aio_complete() before fput(). That way you know that
req->wait is alive and well at least until iocb gets removed from the list.
> + req->events = demangle_poll(iocb->aio_buf) | POLLERR | POLLHUP;
EPOLLERR | EPOLLHUP, please. The values are equal to POLLERR and POLLHUP on
all architectures, but let's avoid misannotations.
> + spin_lock_irq(&ctx->ctx_lock);
> + list_add_tail(&aiocb->ki_list, &ctx->active_reqs);
> +
> + spin_lock(&req->head->lock);
> + mask = req->file->f_op->poll_mask(req->file, req->events);
> + if (!mask)
> + __add_wait_queue(req->head, &req->wait);
ITYM
if (!mask) {
__add_wait_queue(req->head, &req->wait);
list_add_tail(&aiocb->ki_list, &ctx->active_reqs);
}
What's the point of exposing it to aio_poll_cancel() when it has
never been on waitqueue?
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH v2 3/4] mm: add find_alloc_contig_pages() interface
From: Mike Kravetz @ 2018-05-22 20:35 UTC (permalink / raw)
To: Reinette Chatre, Vlastimil Babka, linux-mm, linux-kernel,
linux-api
Cc: Michal Hocko, Christopher Lameter, Guy Shattah, Anshuman Khandual,
Michal Nazarewicz, David Nellans, Laura Abbott, Pavel Machek,
Dave Hansen, Andrew Morton
In-Reply-To: <c7972da1-a908-7550-7253-9de9a963174c@intel.com>
On 05/22/2018 09:41 AM, Reinette Chatre wrote:
> On 5/21/2018 4:48 PM, Mike Kravetz wrote:
>> On 05/21/2018 01:54 AM, Vlastimil Babka wrote:
>>> On 05/04/2018 01:29 AM, Mike Kravetz wrote:
>>>> +/**
>>>> + * find_alloc_contig_pages() -- attempt to find and allocate a contiguous
>>>> + * range of pages
>>>> + * @nr_pages: number of pages to find/allocate
>>>> + * @gfp: gfp mask used to limit search as well as during compaction
>>>> + * @nid: target node
>>>> + * @nodemask: mask of other possible nodes
>>>> + *
>>>> + * Pages can be freed with a call to free_contig_pages(), or by manually
>>>> + * calling __free_page() for each page allocated.
>>>> + *
>>>> + * Return: pointer to 'order' pages on success, or NULL if not successful.
>>>> + */
>>>> +struct page *find_alloc_contig_pages(unsigned long nr_pages, gfp_t gfp,
>>>> + int nid, nodemask_t *nodemask)
>>>> +{
>>>> + unsigned long i, alloc_order, order_pages;
>>>> + struct page *pages;
>>>> +
>>>> + /*
>>>> + * Underlying allocators perform page order sized allocations.
>>>> + */
>>>> + alloc_order = get_count_order(nr_pages);
>>>
>>> So if takes arbitrary nr_pages but convert it to order anyway? I think
>>> that's rather suboptimal and wasteful... e.g. a range could be skipped
>>> because some of the pages added by rounding cannot be migrated away.
>>
>> Yes. My idea with this series was to use existing allocators which are
>> all order based. Let me think about how to do allocation for arbitrary
>> number of allocations.
>> - For less than MAX_ORDER size we rely on the buddy allocator, so we are
>> pretty much stuck with order sized allocation. However, allocations of
>> this size are not really interesting as you can call existing routines
>> directly.
>> - For sizes greater than MAX_ORDER, we know that the allocation size will
>> be at least pageblock sized. So, the isolate/migrate scheme can still
>> be used for full pageblocks. We can then use direct migration for the
>> remaining pages. This does complicate things a bit.
>>
>> I'm guessing that most (?all?) allocations will be order based. The use
>> cases I am aware of (hugetlbfs, Intel Cache Pseudo-Locking, RDMA) are all
>> order based. However, as commented in previous version taking arbitrary
>> nr_pages makes interface more future proof.
>>
>
> I noticed this Cache Pseudo-Locking statement and would like to clarify.
> I have not been following this thread in detail so I would like to
> apologize first if my comments are out of context.
>
> Currently the Cache Pseudo-Locking allocations are order based because I
> assumed it was required by the allocator. The contiguous regions needed
> by Cache Pseudo-Locking will not always be order based - instead it is
> based on the granularity of the cache allocation. One example is a
> platform with 55MB L3 cache that can be divided into 20 equal portions.
> To support Cache Pseudo-Locking on this platform we need to be able to
> allocate contiguous regions at increments of 2816KB (the size of each
> portion). In support of this example platform regions needed would thus
> be 2816KB, 5632KB, 8448KB, etc.
Thank you Reinette. I was not aware of these details. Yours is the most
concrete new use case.
This certainly makes more of a case for arbitrary sized allocations.
--
Mike Kravetz
^ permalink raw reply
* Re: [patch v21 2/4] drivers: jtag: Add Aspeed SoC 24xx and 25xx families JTAG master driver
From: Andy Shevchenko @ 2018-05-22 20:21 UTC (permalink / raw)
To: Oleksandr Shamray
Cc: Greg Kroah-Hartman, Arnd Bergmann, Linux Kernel Mailing List,
linux-arm Mailing List, devicetree, openbmc@lists.ozlabs.org,
Joel Stanley, Jiří Pírko, Tobias Klauser,
open list:SERIAL DRIVERS, Vadim Pasternak, system-sw-low-level,
Rob Herring, openocd-devel-owner@lists.sourceforge.net,
linux-api@vger.kernel.org
In-Reply-To: <AM5PR0501MB244981D83DD33BD29FD64924B1940@AM5PR0501MB2449.eurprd05.prod.outlook.com>
On Tue, May 22, 2018 at 6:00 PM, Oleksandr Shamray
<oleksandrs@mellanox.com> wrote:
> Ok. Changed to:
> #define ASPEED_JTAG_IOUT_LEN(len) \
> (ASPEED_JTAG_CTL_ENG_EN | \
> ASPEED_JTAG_CTL_ENG_OUT_EN | \
> ASPEED_JTAG_CTL_INST_LEN(len))
>
> #define ASPEED_JTAG_DOUT_LEN(len) \
> (ASPEED_JTAG_CTL_ENG_EN | \
> ASPEED_JTAG_CTL_ENG_OUT_EN | \
> ASPEED_JTAG_CTL_DATA_LEN(len))
What about
#define _JTAG_OUT_ENABLE \
( _ENG_EN | _ENG_OUT_EN)
#define _IOUT_LEN(len) \
(_ENABLE | _INST_LEN(len))
#define _DOUT_LEN(len) \
...
?
>> > + apb_frq = clk_get_rate(aspeed_jtag->pclk);
>>
>> > + div = (apb_frq % freq == 0) ? (apb_frq / freq) - 1 :
>> > + (apb_frq / freq);
>>
>> Isn't it the same as
>>
>> div = (apb_frq - 1) / freq;
>>
>> ?
> Seems it is same. Thanks.
Though be careful if apb_frq == 0.
In either case the hw will be screwed, but differently.
>> > + if (xfer->direction == JTAG_READ_XFER)
>> > + tdi = UINT_MAX;
>> > + else
>> > + tdi = data[index];
>>
>> > + if (xfer->direction == JTAG_READ_XFER)
>> > + tdi = UINT_MAX;
>> > + else
>> > + tdi = data[index];
>>
>> Take your time to think how the above duplication can be avoided.
>>
>
> In both cases data[] is different, so I should check it twice, but I will
> change it to, macro like:
>
> #define ASPEED_JTAG_GET_TDI(direction, data) \
> (direction == JTAG_READ_XFER) ? UNIT_MAX : data
Perhaps choose better name for data, b/c in the above you are using data[index].
>> > + dev_err(aspeed_jtag->dev, "irq status:%x\n",
>> > + status);
>> Huh, really?! SPAM.
> I will review and delete redundant debug messages.
Just to be sure you got a point. This is interrupt context. Imagine
what might go wrong.
>> > + err = jtag_register(jtag);
>>
>> Perhaps we might have devm_ variant of this. Check how SPI framework
>> deal with a such.
>>
>
> Jtag driver uses miscdevice and related misc_register and misc_deregister
> calls for creation and destruction. There is no device object prior
> to call to misc_register, which could be used in devm_jtag_register.
Same question as per previous patch.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [RFC PATCH ghak32 V2 13/13] debug audit: read container ID of a process
From: Paul Moore @ 2018-05-22 18:59 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, luto-DgEjT+Ai2ygdnm+yROfE0A,
jlayton-H+wXaHxf7aLQT0dZR+AlfA, carlos-H+wXaHxf7aLQT0dZR+AlfA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA, LKML,
dhowells-H+wXaHxf7aLQT0dZR+AlfA,
linux-audit-H+wXaHxf7aLQT0dZR+AlfA, Eric W. Biederman,
simo-H+wXaHxf7aLQT0dZR+AlfA, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA, Eric Paris, Steve Grubb,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn
In-Reply-To: <20180522173541.slcdszumi7q6c4id-bcJWsdo4jJjeVoXN4CMphl7TgLCtbB0G@public.gmane.org>
On Tue, May 22, 2018 at 1:35 PM, Richard Guy Briggs <rgb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> On 2018-05-21 16:06, Paul Moore wrote:
>> On Mon, May 21, 2018 at 3:19 PM, Eric W. Biederman <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org> wrote:
>> > Steve Grubb <sgrubb-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> writes:
>> >> On Friday, March 16, 2018 5:00:40 AM EDT Richard Guy Briggs wrote:
>> >>> Add support for reading the container ID from the proc filesystem.
>> >>
>> >> I think this could be useful in general. Please consider this to be part of
>> >> the full patch set and not something merely used to debug the patches.
>> >
>> > Only with an audit specific name.
>> >
>> > As it is:
>> >
>> > Nacked-by: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org>
>> >
>> > The truth is the containerid name really stinks and is quite confusing
>> > and does not imply that the label applies only to audit. And little
>> > things like this make me extremely uncofortable with it.
>>
>> It also makes the audit container ID (notice how I *always* call it
>> the *audit* container ID? that is not an accident) available for
>> userspace applications to abuse. Perhaps in the future we can look at
>> ways to make this more available to applications, but this patch is
>> not the answer.
>
> Do you have a productive suggestion?
I haven't given it much thought beyond our discussions and until we
get the basic audit container ID support in place (all the other parts
of this patchset) I doubt I'll be giving it much thought.
--
paul moore
www.paul-moore.com
^ permalink raw reply
* Re: [PATCH v7 0/5] AIO add per-command iopriority
From: Jens Axboe @ 2018-05-22 18:30 UTC (permalink / raw)
To: Al Viro
Cc: adam.manzanares, linux-fsdevel, bcrl, mingo, tglx, kstewart,
peterz, pombredanne, gregkh, bigeasy, rgoldwyn, linux-block,
linux-kernel, linux-aio, linux-api, hch, jmoyer
In-Reply-To: <20180522183019.GD30522@ZenIV.linux.org.uk>
On 5/22/18 12:30 PM, Al Viro wrote:
> On Tue, May 22, 2018 at 11:55:04AM -0600, Jens Axboe wrote:
>> On 5/22/18 11:52 AM, adam.manzanares@wdc.com wrote:
>>> From: Adam Manzanares <adam.manzanares@wdc.com>
>>>
>>> This is the per-I/O equivalent of the ioprio_set system call.
>>> See the following link for performance implications on a SATA HDD:
>>> https://lkml.org/lkml/2016/12/6/495
>>>
>>> First patch factors ioprio_check_cap function out of ioprio_set system call to
>>> also be used by the aio ioprio interface.
>>>
>>> Second patch converts kiocb ki_hint field to a u16 to avoid kiocb bloat.
>>>
>>> Third patch passes ioprio hint from aio iocb to kiocb and initializes kiocb
>>> ioprio value appropriately when it is not explicitly set.
>>>
>>> Fourth patch enables the feature for blkdev.
>>>
>>> Fifth patch enables the feature for iomap direct IO
>>
>> LGTM, you can add:
>>
>> Reviewed-by: Jens Axboe <axboe@kernel.dk>
>>
>> Al, are you picking this series up, or should I?
>
> Probably better if I do, once I finish reviewing Christoph's patchset -
> we already have a bunch of stuff around fs/aio.c in this cycle...
Alright, sounds good, thanks Al.
--
Jens Axboe
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH v7 0/5] AIO add per-command iopriority
From: Al Viro @ 2018-05-22 18:30 UTC (permalink / raw)
To: Jens Axboe
Cc: adam.manzanares, linux-fsdevel, bcrl, mingo, tglx, kstewart,
peterz, pombredanne, gregkh, bigeasy, rgoldwyn, linux-block,
linux-kernel, linux-aio, linux-api, hch, jmoyer
In-Reply-To: <da84d129-4dcd-c861-bfc4-62cd425e60e3@kernel.dk>
On Tue, May 22, 2018 at 11:55:04AM -0600, Jens Axboe wrote:
> On 5/22/18 11:52 AM, adam.manzanares@wdc.com wrote:
> > From: Adam Manzanares <adam.manzanares@wdc.com>
> >
> > This is the per-I/O equivalent of the ioprio_set system call.
> > See the following link for performance implications on a SATA HDD:
> > https://lkml.org/lkml/2016/12/6/495
> >
> > First patch factors ioprio_check_cap function out of ioprio_set system call to
> > also be used by the aio ioprio interface.
> >
> > Second patch converts kiocb ki_hint field to a u16 to avoid kiocb bloat.
> >
> > Third patch passes ioprio hint from aio iocb to kiocb and initializes kiocb
> > ioprio value appropriately when it is not explicitly set.
> >
> > Fourth patch enables the feature for blkdev.
> >
> > Fifth patch enables the feature for iomap direct IO
>
> LGTM, you can add:
>
> Reviewed-by: Jens Axboe <axboe@kernel.dk>
>
> Al, are you picking this series up, or should I?
Probably better if I do, once I finish reviewing Christoph's patchset -
we already have a bunch of stuff around fs/aio.c in this cycle...
^ permalink raw reply
* Re: [PATCH 03/14] arm: Add restartable sequences support
From: Mathieu Desnoyers @ 2018-05-22 18:19 UTC (permalink / raw)
To: Will Deacon, Russell King
Cc: Peter Zijlstra, Paul E. McKenney, Boqun Feng, Andy Lutomirski,
Dave Watson, linux-kernel, linux-api, Paul Turner, Andrew Morton,
Thomas Gleixner, Ingo Molnar, H. Peter Anvin, Andrew Hunter,
Andi Kleen, Chris Lameter, Ben Maurer, rostedt, Josh Triplett,
Linus Torvalds, Catalin Marinas
In-Reply-To: <2135166002.2147.1526571001678.JavaMail.zimbra@efficios.com>
----- On May 17, 2018, at 11:30 AM, Mathieu Desnoyers mathieu.desnoyers@efficios.com wrote:
[...]
>
> Or as proposed by Boqun, we can simply call rseq_syscall in a CONFIG_DEBUG_RSEQ
> ifdef. Given that this is a debug option, is it worth it to add the
> current->rseq
> test for NULL in assembly before the call, or do we want to favor simplicity ?
>
Based on advice from Will Deacon, I alternatively tried to add a new TIF_RSEQ thread
flags, but unfortunately bits 1 through 8 are already used, and this is all that fits
in an immediate operand on arm32 for the fast-path thread flag syscall work mask check
in assembly.
So considering that this is a kernel debug option, I took the approach of adding a
call at the very beginning of return from syscall fast and slow paths, which is only
compiled in if CONFIG_DEBUG_RSEQ=y.
Does the following approach make sense ?
arm: Add syscall detection for restartable sequences
Syscalls are not allowed inside restartable sequences, so add a call to
rseq_syscall() at the very beginning of system call exiting path for
CONFIG_DEBUG_RSEQ=y kernel. This could help us to detect whether there
is a syscall issued inside restartable sequences.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
---
diff --git a/arch/arm/kernel/entry-common.S b/arch/arm/kernel/entry-common.S
index 3c4f887..b427ef8 100644
--- a/arch/arm/kernel/entry-common.S
+++ b/arch/arm/kernel/entry-common.S
@@ -39,12 +39,13 @@ saved_pc .req lr
.section .entry.text,"ax",%progbits
.align 5
-#if !(IS_ENABLED(CONFIG_TRACE_IRQFLAGS) || IS_ENABLED(CONFIG_CONTEXT_TRACKING))
+#if !(IS_ENABLED(CONFIG_TRACE_IRQFLAGS) || IS_ENABLED(CONFIG_CONTEXT_TRACKING) || \
+ IS_ENABLED(CONFIG_DEBUG_RSEQ))
/*
* This is the fast syscall return path. We do as little as possible here,
* such as avoiding writing r0 to the stack. We only use this path if we
- * have tracing and context tracking disabled - the overheads from those
- * features make this path too inefficient.
+ * have tracing, context tracking and rseq debug disabled - the overheads
+ * from those features make this path too inefficient.
*/
ret_fast_syscall:
UNWIND(.fnstart )
@@ -71,14 +72,20 @@ fast_work_pending:
/* fall through to work_pending */
#else
/*
- * The "replacement" ret_fast_syscall for when tracing or context tracking
- * is enabled. As we will need to call out to some C functions, we save
- * r0 first to avoid needing to save registers around each C function call.
+ * The "replacement" ret_fast_syscall for when tracing, context tracking,
+ * or rseq debug is enabled. As we will need to call out to some C functions,
+ * we save r0 first to avoid needing to save registers around each C function
+ * call.
*/
ret_fast_syscall:
UNWIND(.fnstart )
UNWIND(.cantunwind )
str r0, [sp, #S_R0 + S_OFF]! @ save returned r0
+#if IS_ENABLED(CONFIG_DEBUG_RSEQ)
+ /* do_rseq_syscall needs interrupts enabled. */
+ mov r0, sp @ 'regs'
+ bl do_rseq_syscall
+#endif
disable_irq_notrace @ disable interrupts
ldr r2, [tsk, #TI_ADDR_LIMIT]
cmp r2, #TASK_SIZE
@@ -113,6 +120,12 @@ ENDPROC(ret_fast_syscall)
*/
ENTRY(ret_to_user)
ret_slow_syscall:
+#if IS_ENABLED(CONFIG_DEBUG_RSEQ)
+ /* do_rseq_syscall needs interrupts enabled. */
+ enable_irq_notrace @ enable interrupts
+ mov r0, sp @ 'regs'
+ bl do_rseq_syscall
+#endif
disable_irq_notrace @ disable interrupts
ENTRY(ret_to_user_from_irq)
ldr r2, [tsk, #TI_ADDR_LIMIT]
diff --git a/arch/arm/kernel/signal.c b/arch/arm/kernel/signal.c
index 5879ab3..f09e9d66 100644
--- a/arch/arm/kernel/signal.c
+++ b/arch/arm/kernel/signal.c
@@ -710,3 +710,10 @@ asmlinkage void addr_limit_check_failed(void)
{
addr_limit_user_check();
}
+
+#ifdef CONFIG_DEBUG_RSEQ
+asmlinkage void do_rseq_syscall(struct pt_regs *regs)
+{
+ rseq_syscall(regs);
+}
+#endif
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply related
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