* Re: [RFC PATCH 9/9] parisc: allocate sys_membarrier system call number
From: Mathieu Desnoyers @ 2015-09-03 15:41 UTC (permalink / raw)
To: Helge Deller
Cc: Andrew Morton, linux-api, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
James E.J. Bottomley, linux-parisc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <trinity-920d5daa-933d-4e50-bd13-713e544e93c4-1441283170751@3capp-gmx-bs04>
----- On Sep 3, 2015, at 8:26 AM, Helge Deller deller-Mmb7MZpHnFY@public.gmane.org wrote:
> Hi Mathieu,
>
>> [ Untested on this architecture. To try it out: fetch linux-next/akpm,
>> apply this patch, build/run a membarrier-enabled kernel, and do make
>> kselftest. ]
>>
>> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org>
>> CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
>> CC: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> CC: "James E.J. Bottomley" <jejb-6jwH94ZQLHl74goWV3ctuw@public.gmane.org>
>> CC: Helge Deller <deller-Mmb7MZpHnFY@public.gmane.org>
>> CC: linux-parisc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> ---
>> arch/parisc/include/uapi/asm/unistd.h | 3 ++-
>> arch/parisc/kernel/syscall_table.S | 1 +
>> 2 files changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/parisc/include/uapi/asm/unistd.h
>> b/arch/parisc/include/uapi/asm/unistd.h
>> index 2e639d7..dadcada 100644
>> --- a/arch/parisc/include/uapi/asm/unistd.h
>> +++ b/arch/parisc/include/uapi/asm/unistd.h
>> @@ -358,8 +358,9 @@
>> #define __NR_memfd_create (__NR_Linux + 340)
>> #define __NR_bpf (__NR_Linux + 341)
>> #define __NR_execveat (__NR_Linux + 342)
>> +#define __NR_membarrier (__NR_Linux + 343)
>>
>> -#define __NR_Linux_syscalls (__NR_execveat + 1)
>> +#define __NR_Linux_syscalls (__NR_membarrier + 1)
>>
>>
>> #define __IGNORE_select /* newselect */
>> diff --git a/arch/parisc/kernel/syscall_table.S
>> b/arch/parisc/kernel/syscall_table.S
>> index 8eefb12..2faa43b 100644
>> --- a/arch/parisc/kernel/syscall_table.S
>> +++ b/arch/parisc/kernel/syscall_table.S
>> @@ -438,6 +438,7 @@
>> ENTRY_SAME(memfd_create) /* 340 */
>> ENTRY_SAME(bpf)
>> ENTRY_COMP(execveat)
>> + ENTRY_COMP(membarrier)
>
> This needs to be ENTRY_SAME(membarrier), since you don't have/need a
> compat_membarrier() function.
Allright, will fix.
>
> After changing to ENTRY_SAME() I did run the kselftest on parisc:
> deller@ls3xx> ./membarrier_test
> membarrier MEMBARRIER_CMD_QUERY syscall available.
> membarrier: MEMBARRIER_CMD_SHARED success.
> membarrier: tests done!
And add your Tested-by tag, thanks!
Mathieu
>
> Helge
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH 2/3] selftests: add membarrier syscall test
From: Mathieu Desnoyers @ 2015-09-03 15:47 UTC (permalink / raw)
To: Michael Ellerman
Cc: Andy Lutomirski, Andrew Morton,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-api, Pranith Kumar
In-Reply-To: <1441272839.26379.2.camel-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org>
----- On Sep 3, 2015, at 5:33 AM, Michael Ellerman mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org wrote:
> On Tue, 2015-09-01 at 11:32 -0700, Andy Lutomirski wrote:
>> On Tue, Sep 1, 2015 at 10:11 AM, Mathieu Desnoyers
>> <mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org> wrote:
>> > Just to make sure I understand: should we expect that
>> > everyone will issue "make headers_install" on their system
>> > before doing a make kselftest ?
>> >
>> > I see that a few selftests (e.g. memfd) are adding the
>> > source tree include paths to the compiler include paths,
>> > which I guess is to ensure that the kselftest will
>> > work even if the system headers are not up to date.
>>
>> It would be really nice if there were a clean way for selftests to
>> include the kernel headers.
>
> What's wrong with make headers_install?
>
> Or do you mean when writing the tests? That we could fix by adding the
> ../../../../usr/include path to CFLAGS in lib.mk. And fixing all the tests that
> overwrite CFLAGS to append to CFLAGS.
>
>> Perhaps make should build the exportable headers somewhere as a dependency of
>> kselftests.
>
> Yeah the top-level kselftest target could do that I think.
>
> Folks who don't want the headers installed can just run the selftests Makefile
> directly.
>
> Does this work for you?
>
> diff --git a/Makefile b/Makefile
> index c361593..c8841d3 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1080,7 +1080,7 @@ headers_check: headers_install
> # Kernel selftest
>
> PHONY += kselftest
> -kselftest:
> +kselftest: headers_install
> $(Q)$(MAKE) -C tools/testing/selftests run_tests
My personal experience is that make headers_install
does not necessarily play well with the distribution
header file hierarchy, which requires some tweaks
to be done by the users (e.g. asm vs x86_64-linux-gnu).
Also, headers_install typically expects a INSTALL_HDR_PATH.
It would be interesting if we could install the kernel
headers into a specific location that is then re-used by
kselftest, so using it without too much manual configuration
does not require to overwrite the distribution header files
to run tests.
Thoughts ?
Thanks,
Mathieu
>
> # ---------------------------------------------------------------------------
>
> cheers
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [RFC PATCH v3 1/3] vfio: platform: add device properties skeleton and user API
From: Eric Auger @ 2015-09-03 16:48 UTC (permalink / raw)
To: Baptiste Reynal, kvmarm-FPEHb7Xf0XXUo1n7N8X6UoWGPAHP3yOg,
iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
alex.williamson-H+wXaHxf7aLQT0dZR+AlfA
Cc: ABI/API, tech-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J, open list,
christoffer.dall-QSEj5FYQhm4dnm+yROfE0A, VFIO DRIVER
In-Reply-To: <1419024032-1269-2-git-send-email-a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
On 12/19/2014 10:20 PM, Antonios Motakis wrote:
> This patch introduced the API to return device properties about
This patch introduces an API that allows to return device node
properties of a device bound to the vfio-platform/vfio-amba driver?
> a PLATFORM device (if described by a device tree or ACPI) and the
> skeleton of the implementation for VFIO_PLATFORM. Information about any
> device node bound by VFIO_PLATFORM should be queried via the introduced
> ioctl VFIO_DEVICE_GET_DEV_PROPERTY.
>
> The user needs to know the name and the data type of the property he is
> accessing.
>
> Signed-off-by: Antonios Motakis <a.motakis-lrHrjnjw1UfHK3s98zE1ajGjJy/sRE9J@public.gmane.org>
> ---
> drivers/vfio/platform/Makefile | 3 +-
> drivers/vfio/platform/properties.c | 61 +++++++++++++++++++++++++++
> drivers/vfio/platform/vfio_platform_common.c | 35 +++++++++++++++
> drivers/vfio/platform/vfio_platform_private.h | 7 +++
> include/uapi/linux/vfio.h | 26 ++++++++++++
> 5 files changed, 131 insertions(+), 1 deletion(-)
> create mode 100644 drivers/vfio/platform/properties.c
>
> diff --git a/drivers/vfio/platform/Makefile b/drivers/vfio/platform/Makefile
> index 81de144..99f3ba1 100644
> --- a/drivers/vfio/platform/Makefile
> +++ b/drivers/vfio/platform/Makefile
> @@ -1,5 +1,6 @@
>
> -vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o
> +vfio-platform-y := vfio_platform.o vfio_platform_common.o vfio_platform_irq.o \
> + devtree.o
devtree.o or properties.o?
>
> obj-$(CONFIG_VFIO_PLATFORM) += vfio-platform.o
>
> diff --git a/drivers/vfio/platform/properties.c b/drivers/vfio/platform/properties.c
> new file mode 100644
> index 0000000..8b90465
> --- /dev/null
> +++ b/drivers/vfio/platform/properties.c
missing copyright
> @@ -0,0 +1,61 @@
> +#include <linux/slab.h>
> +#include <linux/vfio.h>
> +#include <linux/property.h>
> +#include "vfio_platform_private.h"
> +
> +static int dev_property_get_strings(struct device *dev,
> + char *name, unsigned *lenp,
> + void __user *datap, unsigned long datasz)
> +{
> + return -EINVAL;
> +}
> +
> +static int dev_property_get_uint(struct device *dev, char *name,
> + uint32_t type, unsigned *lenp,
> + void __user *datap, unsigned long datasz)
> +{
> + return -EINVAL;
> +}
> +
> +int vfio_platform_dev_properties(struct device *dev,
> + uint32_t type, unsigned *lenp,
> + void __user *datap, unsigned long datasz)
> +{
> + char *name;
> + long namesz;
> + int ret;
> +
> + namesz = strnlen_user(datap, datasz);
> + if (!namesz)
> + return -EFAULT;
> + if (namesz > datasz)
> + return -EINVAL;
> +
> + name = kzalloc(namesz, GFP_KERNEL);
> + if (!name)
> + return -ENOMEM;
> + if (strncpy_from_user(name, datap, namesz) <= 0) {
> + kfree(name);
> + return -EFAULT;
> + }
> +
> + switch (type) {
> + case VFIO_DEV_PROPERTY_TYPE_STRINGS:
> + ret = dev_property_get_strings(dev, name, lenp, datap, datasz);
> + break;
> +
> + case VFIO_DEV_PROPERTY_TYPE_U64:
> + case VFIO_DEV_PROPERTY_TYPE_U32:
> + case VFIO_DEV_PROPERTY_TYPE_U16:
> + case VFIO_DEV_PROPERTY_TYPE_U8:
> + ret = dev_property_get_uint(dev, name, type, lenp,
> + datap, datasz);
> + break;
> +
> + default:
> + ret = -EINVAL;
> + }
> +
> + kfree(name);
> + return ret;
> +}
> diff --git a/drivers/vfio/platform/vfio_platform_common.c b/drivers/vfio/platform/vfio_platform_common.c
> index a532a25..83ad4b74 100644
> --- a/drivers/vfio/platform/vfio_platform_common.c
> +++ b/drivers/vfio/platform/vfio_platform_common.c
> @@ -19,6 +19,7 @@
> #include <linux/slab.h>
> #include <linux/types.h>
> #include <linux/vfio.h>
> +#include <linux/property.h>
>
> #include "vfio_platform_private.h"
>
> @@ -251,6 +252,34 @@ static long vfio_platform_ioctl(void *device_data,
>
> return ret;
>
> + } else if (cmd == VFIO_DEVICE_GET_DEV_PROPERTY) {
> + struct vfio_dev_property info;
> + void __user *datap;
> + unsigned long datasz;
> + int ret;
> +
> + if (!vdev->dev)
> + return -EINVAL;
> +
> + minsz = offsetofend(struct vfio_dev_property, length);
> +
> + if (copy_from_user(&info, (void __user *)arg, minsz))
> + return -EFAULT;
> +
> + if (info.argsz < minsz)
> + return -EINVAL;
> +
> + datap = (void __user *) arg + minsz;
> + datasz = info.argsz - minsz;
> +
> + ret = vfio_platform_dev_properties(vdev->dev, info.type,
> + &info.length, datap, datasz);
> +
> + if (copy_to_user((void __user *)arg, &info, minsz))
> + ret = -EFAULT;
> +
> + return ret;
> +
> } else if (cmd == VFIO_DEVICE_RESET)
> return -EINVAL;
>
> @@ -501,6 +530,12 @@ int vfio_platform_probe_common(struct vfio_platform_device *vdev,
> return ret;
> }
>
> + /* add device properties flag */
> + if (device_property_present(dev, "name")) {
> + vdev->dev = dev;
> + vdev->flags |= VFIO_DEVICE_FLAGS_DEV_PROPERTIES;
> + }
> +
> mutex_init(&vdev->igate);
>
> return 0;
> diff --git a/drivers/vfio/platform/vfio_platform_private.h b/drivers/vfio/platform/vfio_platform_private.h
> index c3d5b4b..fc6b1fb 100644
> --- a/drivers/vfio/platform/vfio_platform_private.h
> +++ b/drivers/vfio/platform/vfio_platform_private.h
> @@ -53,6 +53,7 @@ struct vfio_platform_device {
> u32 num_irqs;
> int refcnt;
> struct mutex igate;
> + struct device *dev;
>
> /*
> * These fields should be filled by the bus specific binder
> @@ -79,4 +80,10 @@ extern int vfio_platform_set_irqs_ioctl(struct vfio_platform_device *vdev,
> unsigned start, unsigned count,
> void *data);
>
> +/* device properties support in devtree.c */
devtree.c to be removed
> +extern int vfio_platform_dev_properties(struct device *dev,
> + uint32_t type, unsigned *lenp,
> + void __user *datap,
> + unsigned long datasz);
> +
> #endif /* VFIO_PLATFORM_PRIVATE_H */
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index 544d3d8..ac50ab3f 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -161,12 +161,38 @@ struct vfio_device_info {
> #define VFIO_DEVICE_FLAGS_PCI (1 << 1) /* vfio-pci device */
> #define VFIO_DEVICE_FLAGS_PLATFORM (1 << 2) /* vfio-platform device */
> #define VFIO_DEVICE_FLAGS_AMBA (1 << 3) /* vfio-amba device */
> +#define VFIO_DEVICE_FLAGS_DEV_PROPERTIES (1 << 4) /* Device properties */
> __u32 num_regions; /* Max region index + 1 */
> __u32 num_irqs; /* Max IRQ index + 1 */
> };
> #define VFIO_DEVICE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 7)
>
> /**
> + * VFIO_DEVICE_GET_DEV_PROPERTY - _IOR(VFIO_TYPE, VFIO_BASE + 16,
> + * struct vfio_devtree_info)
> + *
> + * Retrive a device property, e.g. from a device tree if available.
retrieve
from an HW description (device tree or ACPI)
> + * Caller will initialize data[] with a single string with the requested
needs to initialize data[] with the property name string?
> + * devicetree property name, and type depending on whether a array of strings
an array
> + * or an array of u32 values is expected. On success, data[] will be extended
> + * with the requested information, either as an array of u32, or with a list
> + * of strings sepparated by the NULL terminating character.
separated
> + * Return: 0 on success, -errno on failure.
> + */
> +struct vfio_dev_property {
> + __u32 argsz;
> + __u32 type;
> +#define VFIO_DEV_PROPERTY_TYPE_STRINGS 0
_STRING?
> +#define VFIO_DEV_PROPERTY_TYPE_U8 1
> +#define VFIO_DEV_PROPERTY_TYPE_U16 2
> +#define VFIO_DEV_PROPERTY_TYPE_U32 3
> +#define VFIO_DEV_PROPERTY_TYPE_U64 4
> + __u32 length;
you may comment length value as initialized by the user (size of the
string including the nul char?
> + __u8 data[];
> +};
> +#define VFIO_DEVICE_GET_DEV_PROPERTY _IO(VFIO_TYPE, VFIO_BASE + 17)
17 does not match figure in above comment
> +
> +/**
> * VFIO_DEVICE_GET_REGION_INFO - _IOWR(VFIO_TYPE, VFIO_BASE + 8,
> * struct vfio_region_info)
> *
>
^ permalink raw reply
* Re: [PATCH 2/2] PCI: Add support for Enhanced Allocation devices
From: Sean O. Stalley @ 2015-09-03 18:23 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Yinghai Lu, Rajat Jain, Michael S. Tsirkin,
Rafał Miłecki,
gong.chen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org,
linux-pci-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux Kernel Mailing List, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150903144654.GF829-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org>
On Thu, Sep 03, 2015 at 09:46:54AM -0500, Bjorn Helgaas wrote:
> On Wed, Sep 02, 2015 at 05:29:38PM -0700, Sean O. Stalley wrote:
> > On Wed, Sep 02, 2015 at 04:21:59PM -0500, Bjorn Helgaas wrote:
> > > On Wed, Sep 02, 2015 at 01:01:27PM -0700, Sean O. Stalley wrote:
> > > > On Wed, Sep 02, 2015 at 02:25:50PM -0500, Bjorn Helgaas wrote:
> > > > > On Wed, Sep 2, 2015 at 12:46 PM, Sean O. Stalley <sean.stalley@intel.com> wrote:
> > > > >
> > > > > > Would it be better to modify pci_claim_resource() to support EA instead of adding pci_ea_claim_resource()?
> > > > > > That way, EA entries would be claimed at the same time as traditional BARs.
> > > > >
> > > > > Yes, I think so.
> > > >
> > > > Ok, I'll make it work this way in the next patchset.
> > > >
> > > > > Why wouldn't pci_claim_resource() work as-is for EA? I see that
> > > > > pci_ea_get_parent_resource() defaults to iomem_resource or
> > > > > ioport_resource if we don't find a parent, but I don't understand why
> > > > > that's necessary.
> > > >
> > > > EA resources may (or may not) be in the parent's range[1].
> > > > If the parent doesn't describe this range, we want to default to the top-level resource.
> > > > Other than that case, I think pci_claim_resource would work as-is.
> > > >
> > > > -Sean
> > > >
> > > > [1] From the EA ECN:
> > > > For a bridge function that is permitted to implement EA based on the rules above, it is
> > > > permitted, but not required, for the bridge function to use EA mechanisms to indicate
> > > > resource ranges that are located behind the bridge Function (see Section 6.9.1.2).
> > >
> > > [BTW, in EA ECN 23_Oct_2014_Final, this text is in sec 6.9, not 6.9.1.2]
> > >
> > > I agree that it implies EA resources need not be in the parent's *EA*
> > > range. But I would read it as saying "a bridge can use either the
> > > usual window registers or EA to indicate resources forwarded
> > > downstream."
> > >
> > > What happens in the following case?
> > >
> > > 00:00.0: PCI bridge to [bus 01]
> > > 00:00.0: bridge window [mem 0x80000000-0x8fffffff]
> > > 01:00.0: EA 0: [mem 0x90000000-0x9000ffff]
> > >
> > > The 00:00.0 bridge knows nothing about EA. The 01:00.0 EA device has
> > > a fixed region at 0x90000000. The ECN says:
> > >
> > > System firmware/software must comprehend that such bridge functions
> > > [those that are permitted to implement EA] are not required to
> > > indicate inclusively all resources behind the bridge, and as a
> > > result system firmware/software must make a complete search of all
> > > functions behind the bridge to comprehend the resources used by
> > > those functions.
> >
> > The intention of this line was to indicate that EA regions are not required
> > to be inside of the Base+Limit window.
>
> It would be a lot nicer if the terminology here built on terminology
> used in the original specs. The P2P Bridge spec defines rules for
> when a bridge forwards transactions between its primary and secondary
> interfaces using Command register Enable bits and Base and Limit
> registers. It doesn't say anything about "indicating resources behind
> the bridge."
Thanks for the feedback. I will forward it on the spec-writer.
> > If an EA device is connected below a bridge, that bridge must be aware of EA.
> > It is assumed that the bridge is aware of the fixed EA regions below it,
> > so system software doesn't need to program the window to include them.
>
> Is the requirement that every bridge upstream of an EA device must be
> aware of EA in the ECN somewhere? What does it even mean for a bridge
> to be "aware of EA"? That it has an EA Capability?
Sorry, poor choice of words on my part. Yes, it must be an EA bridge.
Also, I should point out that this patchset doesn't add support for EA bridges.
It only adds support for EA endpoints that are using an EA entries instead of BARs.
> The EA ECN doesn't change anything in the P2P bridge spec, so a bridge
> that forwards EA regions not described by its Base/Limit registers
> sounds like it would be non-conforming.
Your right, It it seems like there would need to be a change to the P2P spec.
I'll investigate...
> The EA ECN *does* say (end of sec 6.9) that Memory and I/O Space
> enables still work, so I assume that if we clear those bits, a bridge
> will not forward EA regions, and an endpoint will not respond to EA
> regions.
Yes, they still work.
> AFAIK, config transaction forwarding is controlled only by the
> Secondary and Subordinate Bus Number registers. So I assume there's
> no way to disable bridge forwarding of an EA Bus number range.
That is how I read it as well.
> > This is part of the reason why EA devices must be permanently connected
> > (to make sure it doesn't end up behind an old bridge).
> > Re-reading the spec, I can see that this requirement isn't explicitly stated.
> >
> > > A bridge was never required to indicate, e.g., via its window
> > > registers, anything about the resources behind it. Software always
> > > had to search behind the bridge and look at all the downstream BARs.
> > > What's new here is that software now has to look for downstream EA
> > > entries in addition to BARs, and the EA entries are at fixed
> > > addresses.
> > >
> > > My question is what the implication is for address routing performed
> > > by the bridge. The EA ECN doesn't mention any changes there, so I
> > > assume it is software's responsibility to reprogram the 00:00.0 mem
> > > window so it includes [mem 0x90000000-0x9000ffff].
> >
> > The Base+Limit window is not required to include EA regions.
> > In the example shown in in Figure 6-1, the bridge above Bus N [...]
> > is permitted to not indicate the resources used by the two functions
> > in “Si component C”
> >
> > Before, all the BAR regions would be inside the window range.
> > The Base+Limit "indicated" the Range of all the BARs behind the bridge.
> > Once the window was set, system software could avoid an address collision
> > with every device on the bus by avoiding the window.
> >
> > BAR-equivalent EA regions aren't required to be inside the Base+Limit window,
> > which is why System firmware/software must search all the functions behind a bus
> > to avoid address collisions.
> >
> > > If software does have to reprogram that window, the normal
> > > pci_claim_resource() should work. If it doesn't have to reprogram the
> > > window, and there's some magical way for 01:00.0 to work even though
> > > we don't route address space to it, I suspect we'll need significantly
> > > more changes than just pci_ea_claim_resource(), because then 00:00.0
> > > is really not a PCI bridge any more.
>
> Assuming the Memory Enable bit of an EA bridge affects the EA regions,
> I think EA resources of devices behind the bridge should appear as
> children of the bridge, not of iomem_resource. I guess that means the
> bridge should claim the EA regions it forwards.
>
> Bjorn
Those bits should affect the EA regions.
I'll make the EA resources children of the bridge in the next patchset.
Thanks for reviewing,
Sean
^ permalink raw reply
* Re: [PATCH 2/3] selftests: add membarrier syscall test
From: Michael Ellerman @ 2015-09-04 3:36 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: Andy Lutomirski, Andrew Morton,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-api, Pranith Kumar
In-Reply-To: <1734164155.35899.1441295233847.JavaMail.zimbra-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org>
On Thu, 2015-09-03 at 15:47 +0000, Mathieu Desnoyers wrote:
> ----- On Sep 3, 2015, at 5:33 AM, Michael Ellerman mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org wrote:
>
> > On Tue, 2015-09-01 at 11:32 -0700, Andy Lutomirski wrote:
> >> On Tue, Sep 1, 2015 at 10:11 AM, Mathieu Desnoyers
> >> <mathieu.desnoyers-vg+e7yoeK/dWk0Htik3J/w@public.gmane.org> wrote:
> >> > Just to make sure I understand: should we expect that
> >> > everyone will issue "make headers_install" on their system
> >> > before doing a make kselftest ?
> >> >
> >> > I see that a few selftests (e.g. memfd) are adding the
> >> > source tree include paths to the compiler include paths,
> >> > which I guess is to ensure that the kselftest will
> >> > work even if the system headers are not up to date.
> >>
> >> It would be really nice if there were a clean way for selftests to
> >> include the kernel headers.
> >
> > What's wrong with make headers_install?
> >
> > Or do you mean when writing the tests? That we could fix by adding the
> > ../../../../usr/include path to CFLAGS in lib.mk. And fixing all the tests that
> > overwrite CFLAGS to append to CFLAGS.
> >
> >> Perhaps make should build the exportable headers somewhere as a dependency of
> >> kselftests.
> >
> > Yeah the top-level kselftest target could do that I think.
> >
> > Folks who don't want the headers installed can just run the selftests Makefile
> > directly.
> >
> > Does this work for you?
> >
> > diff --git a/Makefile b/Makefile
> > index c361593..c8841d3 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1080,7 +1080,7 @@ headers_check: headers_install
> > # Kernel selftest
> >
> > PHONY += kselftest
> > -kselftest:
> > +kselftest: headers_install
> > $(Q)$(MAKE) -C tools/testing/selftests run_tests
>
> My personal experience is that make headers_install does not necessarily play
> well with the distribution header file hierarchy, which requires some tweaks
> to be done by the users (e.g. asm vs x86_64-linux-gnu).
OK, I've never had issues. What exactly are you doing and how is it going wrong?
> Also, headers_install typically expects a INSTALL_HDR_PATH.
You can specify it, but the default is just usr/, ie. in the kernel directory,
that is what I was proposing. (Actually it's $(objtree)/usr).
> It would be interesting if we could install the kernel headers into a
> specific location that is then re-used by kselftest, so using it without too
> much manual configuration does not require to overwrite the distribution
> header files to run tests.
I think we can do that now, ie:
$ ls /usr/include/linux/membarrier.h
ls: cannot access /usr/include/linux/membarrier.h: No such file or directory
$ cd linux-next
$ make mrproper
$ make headers_install
...
$ ls usr/include/linux/membarrier.h
usr/include/linux/membarrier.h
$ make -C tools/testing/selftests TARGETS=membarrier
make: Entering directory '/home/michael/work/topics/selftests/linux-next/tools/testing/selftests'
for TARGET in membarrier; do \
make -C $TARGET; \
done;
make[1]: Entering directory '/home/michael/work/topics/selftests/linux-next/tools/testing/selftests/membarrier'
gcc -g -I../../../../usr/include/ membarrier_test.c -o membarrier_test
make[1]: Leaving directory '/home/michael/work/topics/selftests/linux-next/tools/testing/selftests/membarrier'
make: Leaving directory '/home/michael/work/topics/selftests/linux-next/tools/testing/selftests'
$ ./tools/testing/selftests/membarrier/membarrier_test
membarrier MEMBARRIER_CMD_QUERY failed. Function not implemented.
$
So that seems to be working for me. Are you doing some different work flow, or
am I just missing something?
I guess it probably doesn't work if you're using O=.. ?
cheers
^ permalink raw reply
* Re: [PATCH 3/3] kselftest/inotify: add inotify_update_watch(2) test-cases
From: Michael Ellerman @ 2015-09-04 6:42 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, John McCutchan, Robert Love,
Eric Paris, Andrew Morton, Al Viro,
linux-api-u79uwXL29TY76Z2rM5mHXA, Shuah Khan
In-Reply-To: <1441293027-3363-4-git-send-email-dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On Thu, 2015-09-03 at 17:10 +0200, David Herrmann wrote:
> diff --git a/tools/testing/selftests/inotify/Makefile b/tools/testing/selftests/inotify/Makefile
> new file mode 100644
> index 0000000..c205abb
> --- /dev/null
> +++ b/tools/testing/selftests/inotify/Makefile
> @@ -0,0 +1,14 @@
> +CC = $(CROSS_COMPILE)gcc
You don't need this, it's in lib.mk already.
> +CFLAGS += -I../../../../include/uapi/
> +CFLAGS += -I../../../../include/
Please don't include the unexported kernel headers in userspace programs.
It will sometimes work, but often not.
> +CFLAGS += -I../../../../usr/include/
This is good, it allows the test to build against the exported kernel headers, in
their default install location (usr/include). To install them run 'make
headers_install' in the top-level of the kernel tree.
It is sufficient to get the new syscall numbers you need, and is usually safe.
> +all:
> + $(CC) $(CFLAGS) test_inotify.c -o test_inotify
> +
> +TEST_PROGS := test_inotify
You don't need to spell out the all rule, this is sufficient:
TEST_PROGS := test_inotify
all: $(TEST_PROGS)
> +
> +include ../lib.mk
> +
> +clean:
> + $(RM) test_inotify test_file
You should either add a leading "-", or make this rm -f, so that when the files
don't exist rm doesn't give you an error.
> diff --git a/tools/testing/selftests/inotify/test_inotify.c b/tools/testing/selftests/inotify/test_inotify.c
> new file mode 100644
> index 0000000..094420a
> --- /dev/null
> +++ b/tools/testing/selftests/inotify/test_inotify.c
> @@ -0,0 +1,105 @@
> +/*
> + * inotify tests
> + */
> +
> +#define _GNU_SOURCE
> +#define __EXPORTED_HEADERS__
Please don't do that.
> +
> +#include <assert.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <limits.h>
> +#include <linux/unistd.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/inotify.h>
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +
> +#define TEST_FILE "./test_file"
> +
> +static int sys_inotify_update_watch(int fd, uint32_t wd, uint32_t mask)
> +{
> + return syscall(__NR_inotify_update_watch, fd, wd, mask);
> +}
> +
> +static int read_inotify(int fd, struct inotify_event **out)
> +{
> + union {
> + struct inotify_event event;
> + char buffer[sizeof(struct inotify_event) + NAME_MAX + 1];
> + } buf;
> + int r;
> +
> + r = read(fd, &buf, sizeof(buf));
> + if (r < 0)
> + return r;
> +
> + assert(r >= sizeof(struct inotify_event) && r <= sizeof(buf));
> + *out = &buf.event;
> + return r;
> +}
> +
> +/* test inotify_update_watch(2) */
> +static void test_update(void)
> +{
> + struct inotify_event *event;
> + int ifd, wd, fd, r;
> +
> + ifd = inotify_init1(IN_CLOEXEC | IN_NONBLOCK);
> + assert(ifd >= 0);
> +
> + unlink(TEST_FILE);
> +
> + /* try adding watch on non-existant file */
> + wd = inotify_add_watch(ifd, TEST_FILE, IN_IGNORED);
> + assert(wd < 0 && errno == ENOENT);
> +
> + /* create file */
> + fd = open(TEST_FILE, O_CREAT | O_CLOEXEC | O_EXCL, S_IRUSR);
> + assert(fd >= 0);
> +
> + /* add watch descriptor without IN_CLOSE */
> + wd = inotify_add_watch(ifd, TEST_FILE, IN_IGNORED);
> + assert(wd > 0);
> +
> + /* close file */
> + close(fd);
> +
> + /* should not have gotten any event */
> + r = read_inotify(ifd, &event);
> + assert(r < 0 && errno == EAGAIN);
> +
> + /* reopen file */
> + fd = open(TEST_FILE, O_CLOEXEC | O_EXCL);
> + assert(fd >= 0);
> +
> + /* invalid flags must result in EINVAL */
> + r = sys_inotify_update_watch(ifd, wd, -1);
> + assert(r < 0 && errno == EINVAL);
> +
> + /* update watch descriptor with IN_CLOSE */
> + r = sys_inotify_update_watch(ifd, wd, IN_MASK_ADD | IN_CLOSE);
> + assert(r >= 0);
> +
> + /* close file */
> + close(fd);
> +
> + /* now we should have seen the event */
> + r = read_inotify(ifd, &event);
> + assert(r >= 0);
> + assert(event->wd == wd);
> + assert(event->mask & IN_CLOSE);
> +
> + unlink(TEST_FILE);
> + close(ifd);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + test_update();
> + return 0;
> +}
You may as well just drop test_update() and put the whole test in main() ?
cheers
^ permalink raw reply
* Re: [RFC PATCH 09/14] ring_buffer: Initialize completions statically in the benchmark
From: Petr Mladek @ 2015-09-04 9:31 UTC (permalink / raw)
To: Steven Rostedt
Cc: Andrew Morton, Oleg Nesterov, Tejun Heo, Ingo Molnar,
Peter Zijlstra, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm-Bw31MaZKKs3YtjvyW6yDsg, Vlastimil Babka,
live-patching-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150803143109.0b13925b-f9ZlEuEWxVcJvu8Pb33WZ0EMvNT87kid@public.gmane.org>
On Mon 2015-08-03 14:31:09, Steven Rostedt wrote:
> On Tue, 28 Jul 2015 16:39:26 +0200
> Petr Mladek <pmladek-IBi9RG/b67k@public.gmane.org> wrote:
>
> > It looks strange to initialize the completions repeatedly.
> >
> > This patch uses static initialization. It simplifies the code
> > and even helps to get rid of two memory barriers.
>
> There was a reason I did it this way and did not use static
> initializers. But I can't recall why I did that. :-/
>
> I'll have to think about this some more.
Heh, the parallel programming is a real fun. I tried to understand
the code in more details and sometimes felt like Duane Dibbley.
Anyway, I found few possible races related to the completions.
One scenario was opened by my previous fix b44754d8262d3aab8429
("ring_buffer: Allow to exit the ring buffer benchmark immediately")
The races can be fixed by the patch below. I still do not see any
scenario where the extra initialization of the two completions
is needed but I am not brave enough to remove it after all ;-)
>From ad75428b1e5e5127bf7dc6062f880ece11dbdbbf Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek-IBi9RG/b67k@public.gmane.org>
Date: Fri, 28 Aug 2015 15:59:00 +0200
Subject: [PATCH 1/2] ring_buffer: Do no not complete benchmark reader too
early
It seems that complete(&read_done) might be called too early
in some situations.
1st scenario:
-------------
CPU0 CPU1
ring_buffer_producer_thread()
wake_up_process(consumer);
wait_for_completion(&read_start);
ring_buffer_consumer_thread()
complete(&read_start);
ring_buffer_producer()
# producing data in
# the do-while cycle
ring_buffer_consumer();
# reading data
# got error
# set kill_test = 1;
set_current_state(
TASK_INTERRUPTIBLE);
if (reader_finish) # false
schedule();
# producer still in the middle of
# do-while cycle
if (consumer && !(cnt % wakeup_interval))
wake_up_process(consumer);
# spurious wakeup
while (!reader_finish &&
!kill_test)
# leaving because
# kill_test == 1
reader_finish = 0;
complete(&read_done);
1st BANG: We might access uninitialized "read_done" if this is the
the first round.
# producer finally leaving
# the do-while cycle because kill_test == 1;
if (consumer) {
reader_finish = 1;
wake_up_process(consumer);
wait_for_completion(&read_done);
2nd BANG: This will never complete because consumer already did
the completion.
2nd scenario:
-------------
CPU0 CPU1
ring_buffer_producer_thread()
wake_up_process(consumer);
wait_for_completion(&read_start);
ring_buffer_consumer_thread()
complete(&read_start);
ring_buffer_producer()
# CPU3 removes the module <--- difference from
# and stops producer <--- the 1st scenario
if (kthread_should_stop())
kill_test = 1;
ring_buffer_consumer();
while (!reader_finish &&
!kill_test)
# kill_test == 1 => we never go
# into the top level while()
reader_finish = 0;
complete(&read_done);
# producer still in the middle of
# do-while cycle
if (consumer && !(cnt % wakeup_interval))
wake_up_process(consumer);
# spurious wakeup
while (!reader_finish &&
!kill_test)
# leaving because kill_test == 1
reader_finish = 0;
complete(&read_done);
BANG: We are in the same "bang" situations as in the 1st scenario.
Root of the problem:
--------------------
ring_buffer_consumer() must complete "read_done" only when "reader_finish"
variable is set. It must not be skipped because of other conditions.
Note that we still must keep the check for "reader_finish" in a loop
because there might be the spurious wakeup as described in the
above scenarios..
Solution:
----------
The top level cycle in ring_buffer_consumer() will finish only when
"reader_finish" is set. The data will be read in "while-do" cycle
so that they are not read after an error (kill_test == 1) and
the spurious wake up.
In addition, "reader_finish" is manipulated by the producer thread.
Therefore we add READ_ONCE() to make sure that the fresh value is
read in each cycle. Also we add the corresponding barrier
to synchronize the sleep check.
Next we set back TASK_RUNNING state for the situation when we
did not sleep.
Just from paranoid reasons, we initialize both completions statically.
It should be more safe if there is other race that we do not know of.
As a side effect we could remove the memory barrier from
ring_buffer_producer_thread(). IMHO, this was the reason of
the barrier. ring_buffer_reset() uses spin locks that should
provide the needed memory barrier for using the buffer.
Signed-off-by: Petr Mladek <pmladek-IBi9RG/b67k@public.gmane.org>
---
kernel/trace/ring_buffer_benchmark.c | 31 ++++++++++++++++++++++---------
1 file changed, 22 insertions(+), 9 deletions(-)
diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
index a1503a027ee2..045e0a24c2a0 100644
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -24,8 +24,8 @@ struct rb_page {
static int wakeup_interval = 100;
static int reader_finish;
-static struct completion read_start;
-static struct completion read_done;
+static DECLARE_COMPLETION(read_start);
+static DECLARE_COMPLETION(read_done);
static struct ring_buffer *buffer;
static struct task_struct *producer;
@@ -178,10 +178,14 @@ static void ring_buffer_consumer(void)
read_events ^= 1;
read = 0;
- while (!reader_finish && !kill_test) {
- int found;
+ /*
+ * Always wait until we are asked to finish and the producer
+ * is ready to wait for the completion.
+ */
+ while (!READ_ONCE(reader_finish)) {
+ int found = 1;
- do {
+ while (found && !kill_test) {
int cpu;
found = 0;
@@ -195,17 +199,29 @@ static void ring_buffer_consumer(void)
if (kill_test)
break;
+
if (stat == EVENT_FOUND)
found = 1;
+
}
- } while (found && !kill_test);
+ }
+ /*
+ * Sleep a bit. Producer with wake up us when some more data
+ * are available or when we should finish reading.
+ */
set_current_state(TASK_INTERRUPTIBLE);
+ /*
+ * Make sure that we read the updated finish variable
+ * before producer tries to wakeup us.
+ */
+ smp_rmb();
if (reader_finish)
break;
schedule();
}
+ __set_current_state(TASK_RUNNING);
reader_finish = 0;
complete(&read_done);
}
@@ -389,13 +405,10 @@ static int ring_buffer_consumer_thread(void *arg)
static int ring_buffer_producer_thread(void *arg)
{
- init_completion(&read_start);
-
while (!kthread_should_stop() && !kill_test) {
ring_buffer_reset(buffer);
if (consumer) {
- smp_wmb();
wake_up_process(consumer);
wait_for_completion(&read_start);
}
--
1.8.5.6
^ permalink raw reply related
* Re: [RFC PATCH 10/14] ring_buffer: Fix more races when terminating the producer in the benchmark
From: Petr Mladek @ 2015-09-04 9:38 UTC (permalink / raw)
To: Steven Rostedt
Cc: Andrew Morton, Oleg Nesterov, Tejun Heo, Ingo Molnar,
Peter Zijlstra, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm, Vlastimil Babka, live-patching, linux-api, linux-kernel
In-Reply-To: <20150803143323.426ea2fc@gandalf.local.home>
On Mon 2015-08-03 14:33:23, Steven Rostedt wrote:
> On Tue, 28 Jul 2015 16:39:27 +0200
> Petr Mladek <pmladek@suse.com> wrote:
>
> > @@ -384,7 +389,7 @@ static int ring_buffer_consumer_thread(void *arg)
> >
> > static int ring_buffer_producer_thread(void *arg)
> > {
> > - while (!kthread_should_stop() && !kill_test) {
> > + while (!break_test()) {
> > ring_buffer_reset(buffer);
> >
> > if (consumer) {
> > @@ -393,11 +398,15 @@ static int ring_buffer_producer_thread(void *arg)
> > }
> >
> > ring_buffer_producer();
> > - if (kill_test)
> > + if (break_test())
> > goto out_kill;
> >
> > trace_printk("Sleeping for 10 secs\n");
> > set_current_state(TASK_INTERRUPTIBLE);
> > + if (break_test()) {
> > + __set_current_state(TASK_RUNNING);
>
> Move the setting of the current state to after the out_kill label.
Please, find below the updated version of this patch.
I also reverted some changes in the consumer code. It never stays
in a loop for too long and it must stay in ring_buffer_producer()
until "reader_finish" variable is set.
>From 7f5b1a5b8cf8248245897b55ffc51a6d74c8e15b Mon Sep 17 00:00:00 2001
From: Petr Mladek <pmladek@suse.com>
Date: Fri, 19 Jun 2015 14:38:36 +0200
Subject: [PATCH 2/2] ring_buffer: Fix more races when terminating the producer
in the benchmark
The commit b44754d8262d3aab8 ("ring_buffer: Allow to exit the ring
buffer benchmark immediately") added a hack into ring_buffer_producer()
that set @kill_test when kthread_should_stop() returned true. It improved
the situation a lot. It stopped the kthread in most cases because
the producer spent most of the time in the patched while cycle.
But there are still few possible races when kthread_should_stop()
is set outside of the cycle. Then we do not set @kill_test and
some other checks pass.
This patch adds a better fix. It renames @test_kill/TEST_KILL() into
a better descriptive @test_error/TEST_ERROR(). Also it introduces
break_test() function that checks for both @test_error and
kthread_should_stop().
The new function is used in the producer when the check for @test_error
is not enough. It is not used in the consumer because its state
is manipulated by the producer via the "reader_finish" variable.
Also we add a missing check into ring_buffer_producer_thread()
between setting TASK_INTERRUPTIBLE and calling schedule_timeout().
Otherwise, we might miss a wakeup from kthread_stop().
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
kernel/trace/ring_buffer_benchmark.c | 54 +++++++++++++++++++-----------------
1 file changed, 29 insertions(+), 25 deletions(-)
diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
index 045e0a24c2a0..d1bfe4399e96 100644
--- a/kernel/trace/ring_buffer_benchmark.c
+++ b/kernel/trace/ring_buffer_benchmark.c
@@ -60,12 +60,12 @@ MODULE_PARM_DESC(consumer_fifo, "fifo prio for consumer");
static int read_events;
-static int kill_test;
+static int test_error;
-#define KILL_TEST() \
+#define TEST_ERROR() \
do { \
- if (!kill_test) { \
- kill_test = 1; \
+ if (!test_error) { \
+ test_error = 1; \
WARN_ON(1); \
} \
} while (0)
@@ -75,6 +75,11 @@ enum event_status {
EVENT_DROPPED,
};
+static bool break_test(void)
+{
+ return test_error || kthread_should_stop();
+}
+
static enum event_status read_event(int cpu)
{
struct ring_buffer_event *event;
@@ -87,7 +92,7 @@ static enum event_status read_event(int cpu)
entry = ring_buffer_event_data(event);
if (*entry != cpu) {
- KILL_TEST();
+ TEST_ERROR();
return EVENT_DROPPED;
}
@@ -115,10 +120,10 @@ static enum event_status read_page(int cpu)
rpage = bpage;
/* The commit may have missed event flags set, clear them */
commit = local_read(&rpage->commit) & 0xfffff;
- for (i = 0; i < commit && !kill_test; i += inc) {
+ for (i = 0; i < commit && !test_error ; i += inc) {
if (i >= (PAGE_SIZE - offsetof(struct rb_page, data))) {
- KILL_TEST();
+ TEST_ERROR();
break;
}
@@ -128,7 +133,7 @@ static enum event_status read_page(int cpu)
case RINGBUF_TYPE_PADDING:
/* failed writes may be discarded events */
if (!event->time_delta)
- KILL_TEST();
+ TEST_ERROR();
inc = event->array[0] + 4;
break;
case RINGBUF_TYPE_TIME_EXTEND:
@@ -137,12 +142,12 @@ static enum event_status read_page(int cpu)
case 0:
entry = ring_buffer_event_data(event);
if (*entry != cpu) {
- KILL_TEST();
+ TEST_ERROR();
break;
}
read++;
if (!event->array[0]) {
- KILL_TEST();
+ TEST_ERROR();
break;
}
inc = event->array[0] + 4;
@@ -150,17 +155,17 @@ static enum event_status read_page(int cpu)
default:
entry = ring_buffer_event_data(event);
if (*entry != cpu) {
- KILL_TEST();
+ TEST_ERROR();
break;
}
read++;
inc = ((event->type_len + 1) * 4);
}
- if (kill_test)
+ if (test_error)
break;
if (inc <= 0) {
- KILL_TEST();
+ TEST_ERROR();
break;
}
}
@@ -185,7 +190,7 @@ static void ring_buffer_consumer(void)
while (!READ_ONCE(reader_finish)) {
int found = 1;
- while (found && !kill_test) {
+ while (found && !test_error) {
int cpu;
found = 0;
@@ -197,7 +202,7 @@ static void ring_buffer_consumer(void)
else
stat = read_page(cpu);
- if (kill_test)
+ if (test_error)
break;
if (stat == EVENT_FOUND)
@@ -279,10 +284,7 @@ static void ring_buffer_producer(void)
if (cnt % wakeup_interval)
cond_resched();
#endif
- if (kthread_should_stop())
- kill_test = 1;
-
- } while (ktime_before(end_time, timeout) && !kill_test);
+ } while (ktime_before(end_time, timeout) && !break_test());
trace_printk("End ring buffer hammer\n");
if (consumer) {
@@ -303,7 +305,7 @@ static void ring_buffer_producer(void)
entries = ring_buffer_entries(buffer);
overruns = ring_buffer_overruns(buffer);
- if (kill_test && !kthread_should_stop())
+ if (test_error)
trace_printk("ERROR!\n");
if (!disable_reader) {
@@ -384,15 +386,14 @@ static void wait_to_die(void)
static int ring_buffer_consumer_thread(void *arg)
{
- while (!kthread_should_stop() && !kill_test) {
+ while (!break_test()) {
complete(&read_start);
ring_buffer_consumer();
set_current_state(TASK_INTERRUPTIBLE);
- if (kthread_should_stop() || kill_test)
+ if (break_test())
break;
-
schedule();
}
__set_current_state(TASK_RUNNING);
@@ -405,7 +406,7 @@ static int ring_buffer_consumer_thread(void *arg)
static int ring_buffer_producer_thread(void *arg)
{
- while (!kthread_should_stop() && !kill_test) {
+ while (!break_test()) {
ring_buffer_reset(buffer);
if (consumer) {
@@ -414,15 +415,18 @@ static int ring_buffer_producer_thread(void *arg)
}
ring_buffer_producer();
- if (kill_test)
+ if (break_test())
goto out_kill;
trace_printk("Sleeping for 10 secs\n");
set_current_state(TASK_INTERRUPTIBLE);
+ if (break_test())
+ goto out_kill;
schedule_timeout(HZ * SLEEP_TIME);
}
out_kill:
+ __set_current_state(TASK_RUNNING);
if (!kthread_should_stop())
wait_to_die();
--
1.8.5.6
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply related
* Re: [RFC PATCH 09/14] ring_buffer: Initialize completions statically in the benchmark
From: Steven Rostedt @ 2015-09-04 13:15 UTC (permalink / raw)
To: Petr Mladek
Cc: Andrew Morton, Oleg Nesterov, Tejun Heo, Ingo Molnar,
Peter Zijlstra, Paul E. McKenney, Josh Triplett, Thomas Gleixner,
Linus Torvalds, Jiri Kosina, Borislav Petkov, Michal Hocko,
linux-mm, Vlastimil Babka, live-patching, linux-api, linux-kernel
In-Reply-To: <20150904093126.GH22739@pathway.suse.cz>
On Fri, 4 Sep 2015 11:31:26 +0200
Petr Mladek <pmladek@suse.com> wrote:
> 1st scenario:
> -------------
>
> CPU0 CPU1
>
> ring_buffer_producer_thread()
> wake_up_process(consumer);
> wait_for_completion(&read_start);
>
> ring_buffer_consumer_thread()
> complete(&read_start);
>
> ring_buffer_producer()
> # producing data in
> # the do-while cycle
>
> ring_buffer_consumer();
> # reading data
> # got error
So you're saying the error condition can cause this race? OK, I'll
admit that. Although, I don't think it's that big of a bug because the
error condition will also trigger a WARN_ON() and it means the ring
buffer code is broken, which also means the kernel is broken. Things
that go wrong after that is just tough luck.
But I'm not saying we couldn't fix it either.
> # set kill_test = 1;
> set_current_state(
> TASK_INTERRUPTIBLE);
> if (reader_finish) # false
> schedule();
>
> # producer still in the middle of
> # do-while cycle
> if (consumer && !(cnt % wakeup_interval))
> wake_up_process(consumer);
>
> # spurious wakeup
> while (!reader_finish &&
> !kill_test)
> # leaving because
> # kill_test == 1
> reader_finish = 0;
> complete(&read_done);
>
> 1st BANG: We might access uninitialized "read_done" if this is the
> the first round.
>
> # producer finally leaving
> # the do-while cycle because kill_test == 1;
>
> if (consumer) {
> reader_finish = 1;
> wake_up_process(consumer);
> wait_for_completion(&read_done);
>
> 2nd BANG: This will never complete because consumer already did
> the completion.
>
> 2nd scenario:
> -------------
>
> CPU0 CPU1
>
> ring_buffer_producer_thread()
> wake_up_process(consumer);
> wait_for_completion(&read_start);
>
> ring_buffer_consumer_thread()
> complete(&read_start);
>
> ring_buffer_producer()
> # CPU3 removes the module <--- difference from
> # and stops producer <--- the 1st scenario
> if (kthread_should_stop())
> kill_test = 1;
>
> ring_buffer_consumer();
> while (!reader_finish &&
> !kill_test)
> # kill_test == 1 => we never go
> # into the top level while()
> reader_finish = 0;
> complete(&read_done);
>
> # producer still in the middle of
> # do-while cycle
> if (consumer && !(cnt % wakeup_interval))
> wake_up_process(consumer);
>
> # spurious wakeup
> while (!reader_finish &&
> !kill_test)
> # leaving because kill_test == 1
> reader_finish = 0;
> complete(&read_done);
>
> BANG: We are in the same "bang" situations as in the 1st scenario.
This scenario I believe is a true bug, because it can happen on a
kernel that is not broken.
>
> Root of the problem:
> --------------------
>
> ring_buffer_consumer() must complete "read_done" only when "reader_finish"
> variable is set. It must not be skipped because of other conditions.
"It must not be skipped due to other conditions."
>
> Note that we still must keep the check for "reader_finish" in a loop
> because there might be the spurious wakeup as described in the
"might be spurious wakeups"
> above scenarios..
>
> Solution:
> ----------
>
> The top level cycle in ring_buffer_consumer() will finish only when
> "reader_finish" is set. The data will be read in "while-do" cycle
> so that they are not read after an error (kill_test == 1) and
> the spurious wake up.
"or a spurious wake up"
>
> In addition, "reader_finish" is manipulated by the producer thread.
> Therefore we add READ_ONCE() to make sure that the fresh value is
> read in each cycle. Also we add the corresponding barrier
> to synchronize the sleep check.
>
> Next we set back TASK_RUNNING state for the situation when we
> did not sleep.
"Next we set the state back to TASK_RUNNING for the situation where we
did not sleep"
>
> Just from paranoid reasons, we initialize both completions statically.
> It should be more safe if there is other race that we do not know of.
"This is safer, in case there are other races that we are unaware of."
>
> As a side effect we could remove the memory barrier from
> ring_buffer_producer_thread(). IMHO, this was the reason of
"the reason for"
> the barrier. ring_buffer_reset() uses spin locks that should
> provide the needed memory barrier for using the buffer.
>
> Signed-off-by: Petr Mladek <pmladek@suse.com>
> ---
> kernel/trace/ring_buffer_benchmark.c | 31 ++++++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 9 deletions(-)
>
> diff --git a/kernel/trace/ring_buffer_benchmark.c b/kernel/trace/ring_buffer_benchmark.c
> index a1503a027ee2..045e0a24c2a0 100644
> --- a/kernel/trace/ring_buffer_benchmark.c
> +++ b/kernel/trace/ring_buffer_benchmark.c
> @@ -24,8 +24,8 @@ struct rb_page {
> static int wakeup_interval = 100;
>
> static int reader_finish;
> -static struct completion read_start;
> -static struct completion read_done;
> +static DECLARE_COMPLETION(read_start);
> +static DECLARE_COMPLETION(read_done);
>
> static struct ring_buffer *buffer;
> static struct task_struct *producer;
> @@ -178,10 +178,14 @@ static void ring_buffer_consumer(void)
> read_events ^= 1;
>
> read = 0;
> - while (!reader_finish && !kill_test) {
> - int found;
> + /*
> + * Always wait until we are asked to finish and the producer
> + * is ready to wait for the completion.
"Continue running until the producer specifically asks to stop and is
ready for the completion."
> + */
> + while (!READ_ONCE(reader_finish)) {
> + int found = 1;
>
> - do {
> + while (found && !kill_test) {
> int cpu;
>
> found = 0;
> @@ -195,17 +199,29 @@ static void ring_buffer_consumer(void)
>
> if (kill_test)
> break;
> +
> if (stat == EVENT_FOUND)
> found = 1;
> +
> }
> - } while (found && !kill_test);
> + }
>
> + /*
> + * Sleep a bit. Producer with wake up us when some more data
> + * are available or when we should finish reading.
"Wait till the producer wakes us up when there is more data available or
when the producer wants us to finish reading"
> + */
> set_current_state(TASK_INTERRUPTIBLE);
> + /*
> + * Make sure that we read the updated finish variable
> + * before producer tries to wakeup us.
> + */
> + smp_rmb();
The above is unneeded. Look at the definition of set_current_state().
> if (reader_finish)
> break;
>
> schedule();
> }
> + __set_current_state(TASK_RUNNING);
> reader_finish = 0;
> complete(&read_done);
> }
> @@ -389,13 +405,10 @@ static int ring_buffer_consumer_thread(void *arg)
>
> static int ring_buffer_producer_thread(void *arg)
> {
> - init_completion(&read_start);
> -
> while (!kthread_should_stop() && !kill_test) {
> ring_buffer_reset(buffer);
>
> if (consumer) {
> - smp_wmb();
> wake_up_process(consumer);
> wait_for_completion(&read_start);
> }
Please make the above changes and submit the patch again as a separate
patch.
Thanks!
-- Steve
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [PATCH 1/1] Corsair Vengeance K90 driver
From: Jiri Kosina @ 2015-09-04 13:27 UTC (permalink / raw)
To: Clément Vuchener; +Cc: linux-api, linux-kernel, linux-input
In-Reply-To: <55E1BBC8.9010303@gmail.com>
On Sat, 29 Aug 2015, Clément Vuchener wrote:
This is missing changelog and Signed-off-by: line. I know that you have
provided that in your cover letter, but cover letter never make it to the
actual GIT commits.
So please fix that and resend the patch, thanks.
> ---
> Documentation/ABI/testing/sysfs-class-k90_profile | 55 ++
> .../ABI/testing/sysfs-driver-hid-corsair-k90 | 15 +
> drivers/hid/Kconfig | 10 +
> drivers/hid/Makefile | 1 +
> drivers/hid/hid-core.c | 1 +
> drivers/hid/hid-corsair-k90.c | 690
> +++++++++++++++++++++
> drivers/hid/hid-ids.h | 3 +
> 7 files changed, 775 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-class-k90_profile
> create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
> create mode 100644 drivers/hid/hid-corsair-k90.c
>
> diff --git a/Documentation/ABI/testing/sysfs-class-k90_profile
> b/Documentation/ABI/testing/sysfs-class-k90_profile
> new file mode 100644
> index 0000000..3275c16
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-class-k90_profile
> @@ -0,0 +1,55 @@
> +What: /sys/class/k90_profile/<profile>/profile_number
> +Date: August 2015
> +KernelVersion: 4.2
> +Contact: Clement Vuchener <clement.vuchener@gmail.com>
> +Description: Get the number of the profile.
> +
> +What: /sys/class/k90_profile/<profile>/bindings
> +Date: August 2015
> +KernelVersion: 4.2
> +Contact: Clement Vuchener <clement.vuchener@gmail.com>
> +Description: Write bindings to the keyboard on-board profile.
> + The data structure is:
> + - number of bindings in this structure (1 byte)
> + - size of this data structure (2 bytes big endian)
> + - size of the macro data written to
> + /sys/class/k90_profile/<profile>/data (2 bytes big endian)
> + - array of bindings referencing the data from
> + /sys/class/k90_profile/<profile>/data, each containing:
> + * 0x10 for a key usage, 0x20 for a macro (1 byte)
> + * offset of the key usage/macro data (2 bytes big endian)
> + * length of the key usage/macro data (2 bytes big endian)
> +
This looks like violation of one-value-per-attribute rule for sysfs ABI.
Could you please think about it once again with this aspect on mind?
[ ... snip ... ]
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index e6fce23..f0d9125 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1807,6 +1807,7 @@ static const struct hid_device_id
> hid_have_special_driver[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
> USB_DEVICE_ID_CHICONY_WIRELESS) },
> { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
> USB_DEVICE_ID_CHICONY_WIRELESS2) },
> { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
> { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS,
> USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
> { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
> { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
> USB_DEVICE_ID_CYPRESS_BARCODE_1) },
Your mail client is corrupting long lines, making tha patch application
impossible. Please fix that for your following submissions.
> diff --git a/drivers/hid/hid-corsair-k90.c b/drivers/hid/hid-corsair-k90.c
> new file mode 100644
> index 0000000..67c1095
> --- /dev/null
> +++ b/drivers/hid/hid-corsair-k90.c
> @@ -0,0 +1,690 @@
> +/*
> + * HID driver for Corsair Vengeance K90 Keyboard
Usually we try to be a little bit more generic, and name the driver
according to the vendor (and fold all the vedor-specific stuff into the
one driver).
So my suggestion would be to name the driver hid-corsair, keeping the
possibility for adding more devices later open.
[ ... snip ... ]
> +static int k90_init_special_functions(struct hid_device *dev)
> +{
> + int ret, i;
> + struct usb_interface *usbif = to_usb_interface(dev->dev.parent);
> + struct usb_device *usbdev = interface_to_usbdev(usbif);
> + char data[8];
> + struct k90_drvdata *drvdata =
> + kzalloc(sizeof(struct k90_drvdata), GFP_KERNEL);
> + size_t name_sz;
> + char *name;
> + struct k90_led *led;
> +
> + if (!drvdata) {
> + ret = -ENOMEM;
> + goto fail_drvdata;
> + }
> + hid_set_drvdata(dev, drvdata);
> +
> + /* Get current status */
> + ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
> + K90_REQUEST_STATUS,
> + USB_DIR_IN | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE, 0, 0, data, 8,
> + USB_CTRL_SET_TIMEOUT);
So you apparently also depend on USB ...
> + if (ret < 0) {
> + hid_warn(dev, "Failed to get K90 initial state (error %d).\n",
> + ret);
> + drvdata->backlight.brightness = 0;
> + drvdata->current_profile = 1;
> + } else {
> + drvdata->backlight.brightness = data[4];
> + drvdata->current_profile = data[7];
> + }
> + /* Get current mode */
> + ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
> + K90_REQUEST_GET_MODE,
> + USB_DIR_IN | USB_TYPE_VENDOR |
> + USB_RECIP_DEVICE, 0, 0, data, 2,
> + USB_CTRL_SET_TIMEOUT);
> + if (ret < 0)
> + hid_warn(dev, "Failed to get K90 initial mode (error %d).\n",
> + ret);
> + else {
> + switch (data[0]) {
> + case K90_MACRO_MODE_HW:
> + drvdata->macro_mode = 1;
> + break;
> + case K90_MACRO_MODE_SW:
> + drvdata->macro_mode = 0;
> + break;
> + default:
> + hid_warn(dev, "K90 in unknown mode: %02x.\n",
> + data[0]);
> + }
> + }
> +
> + /* Init LED device for backlight */
> + name_sz =
> + strlen(dev_name(&dev->dev)) + sizeof(K90_BACKLIGHT_LED_SUFFIX);
> + name = devm_kzalloc(&dev->dev, name_sz, GFP_KERNEL);
> + if (!name) {
> + ret = -ENOMEM;
> + goto fail_backlight;
> + }
> + snprintf(name, name_sz, "%s" K90_BACKLIGHT_LED_SUFFIX,
> + dev_name(&dev->dev));
> + led = &drvdata->backlight;
> + led->cdev.name = name;
> + led->cdev.max_brightness = 3;
> + led->cdev.brightness_set = k90_brightness_set;
> + led->cdev.brightness_get = k90_brightness_get;
> + INIT_WORK(&led->work, k90_backlight_work);
> + ret = led_classdev_register(&dev->dev, &led->cdev);
... and also on LED subsystem, but this dependency is not expressed in
Kconfig.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 1/1] Corsair Vengeance K90 driver
From: Clément Vuchener @ 2015-09-04 15:18 UTC (permalink / raw)
To: Jiri Kosina
Cc: linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.LNX.2.00.1509041518580.30132-ztGlSCb7Y1iN3ZZ/Hiejyg@public.gmane.org>
On 09/04/2015 03:27 PM, Jiri Kosina wrote:
> On Sat, 29 Aug 2015, Clément Vuchener wrote:
>
>
> This is missing changelog and Signed-off-by: line. I know that you have
> provided that in your cover letter, but cover letter never make it to the
> actual GIT commits.
>
> So please fix that and resend the patch, thanks.
>
>> ---
>> Documentation/ABI/testing/sysfs-class-k90_profile | 55 ++
>> .../ABI/testing/sysfs-driver-hid-corsair-k90 | 15 +
>> drivers/hid/Kconfig | 10 +
>> drivers/hid/Makefile | 1 +
>> drivers/hid/hid-core.c | 1 +
>> drivers/hid/hid-corsair-k90.c | 690
>> +++++++++++++++++++++
>> drivers/hid/hid-ids.h | 3 +
>> 7 files changed, 775 insertions(+)
>> create mode 100644 Documentation/ABI/testing/sysfs-class-k90_profile
>> create mode 100644 Documentation/ABI/testing/sysfs-driver-hid-corsair-k90
>> create mode 100644 drivers/hid/hid-corsair-k90.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-class-k90_profile
>> b/Documentation/ABI/testing/sysfs-class-k90_profile
>> new file mode 100644
>> index 0000000..3275c16
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-class-k90_profile
>> @@ -0,0 +1,55 @@
>> +What: /sys/class/k90_profile/<profile>/profile_number
>> +Date: August 2015
>> +KernelVersion: 4.2
>> +Contact: Clement Vuchener <clement.vuchener-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> +Description: Get the number of the profile.
>> +
>> +What: /sys/class/k90_profile/<profile>/bindings
>> +Date: August 2015
>> +KernelVersion: 4.2
>> +Contact: Clement Vuchener <clement.vuchener-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> +Description: Write bindings to the keyboard on-board profile.
>> + The data structure is:
>> + - number of bindings in this structure (1 byte)
>> + - size of this data structure (2 bytes big endian)
>> + - size of the macro data written to
>> + /sys/class/k90_profile/<profile>/data (2 bytes big endian)
>> + - array of bindings referencing the data from
>> + /sys/class/k90_profile/<profile>/data, each containing:
>> + * 0x10 for a key usage, 0x20 for a macro (1 byte)
>> + * offset of the key usage/macro data (2 bytes big endian)
>> + * length of the key usage/macro data (2 bytes big endian)
>> +
> This looks like violation of one-value-per-attribute rule for sysfs ABI.
> Could you please think about it once again with this aspect on mind?
Per key attributes would be nice but I don't think I can do that. The profile must be written all at once and I don't know any way to read it from the hardware (the windows driver I studied does not do it). So writing one value only would erase all the others.
I think I will remove this part from the driver. The same thing can easily be done in user space through libusb and writing profile should be exceptional enough that it is not a problem to detach the driver while doing it. That part of the driver is not really useful with the current ABI.
>
> [ ... snip ... ]
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index e6fce23..f0d9125 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1807,6 +1807,7 @@ static const struct hid_device_id
>> hid_have_special_driver[] = {
>> { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
>> USB_DEVICE_ID_CHICONY_WIRELESS) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
>> USB_DEVICE_ID_CHICONY_WIRELESS2) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY, USB_DEVICE_ID_CHICONY_AK1D) },
>> + { HID_USB_DEVICE(USB_VENDOR_ID_CORSAIR, USB_DEVICE_ID_CORSAIR_K90) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS,
>> USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_CP2112) },
>> { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
>> USB_DEVICE_ID_CYPRESS_BARCODE_1) },
> Your mail client is corrupting long lines, making tha patch application
> impossible. Please fix that for your following submissions.
>
>> diff --git a/drivers/hid/hid-corsair-k90.c b/drivers/hid/hid-corsair-k90.c
>> new file mode 100644
>> index 0000000..67c1095
>> --- /dev/null
>> +++ b/drivers/hid/hid-corsair-k90.c
>> @@ -0,0 +1,690 @@
>> +/*
>> + * HID driver for Corsair Vengeance K90 Keyboard
> Usually we try to be a little bit more generic, and name the driver
> according to the vendor (and fold all the vedor-specific stuff into the
> one driver).
>
> So my suggestion would be to name the driver hid-corsair, keeping the
> possibility for adding more devices later open.
>
> [ ... snip ... ]
>> +static int k90_init_special_functions(struct hid_device *dev)
>> +{
>> + int ret, i;
>> + struct usb_interface *usbif = to_usb_interface(dev->dev.parent);
>> + struct usb_device *usbdev = interface_to_usbdev(usbif);
>> + char data[8];
>> + struct k90_drvdata *drvdata =
>> + kzalloc(sizeof(struct k90_drvdata), GFP_KERNEL);
>> + size_t name_sz;
>> + char *name;
>> + struct k90_led *led;
>> +
>> + if (!drvdata) {
>> + ret = -ENOMEM;
>> + goto fail_drvdata;
>> + }
>> + hid_set_drvdata(dev, drvdata);
>> +
>> + /* Get current status */
>> + ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
>> + K90_REQUEST_STATUS,
>> + USB_DIR_IN | USB_TYPE_VENDOR |
>> + USB_RECIP_DEVICE, 0, 0, data, 8,
>> + USB_CTRL_SET_TIMEOUT);
> So you apparently also depend on USB ...
>
>> + if (ret < 0) {
>> + hid_warn(dev, "Failed to get K90 initial state (error %d).\n",
>> + ret);
>> + drvdata->backlight.brightness = 0;
>> + drvdata->current_profile = 1;
>> + } else {
>> + drvdata->backlight.brightness = data[4];
>> + drvdata->current_profile = data[7];
>> + }
>> + /* Get current mode */
>> + ret = usb_control_msg(usbdev, usb_rcvctrlpipe(usbdev, 0),
>> + K90_REQUEST_GET_MODE,
>> + USB_DIR_IN | USB_TYPE_VENDOR |
>> + USB_RECIP_DEVICE, 0, 0, data, 2,
>> + USB_CTRL_SET_TIMEOUT);
>> + if (ret < 0)
>> + hid_warn(dev, "Failed to get K90 initial mode (error %d).\n",
>> + ret);
>> + else {
>> + switch (data[0]) {
>> + case K90_MACRO_MODE_HW:
>> + drvdata->macro_mode = 1;
>> + break;
>> + case K90_MACRO_MODE_SW:
>> + drvdata->macro_mode = 0;
>> + break;
>> + default:
>> + hid_warn(dev, "K90 in unknown mode: %02x.\n",
>> + data[0]);
>> + }
>> + }
>> +
>> + /* Init LED device for backlight */
>> + name_sz =
>> + strlen(dev_name(&dev->dev)) + sizeof(K90_BACKLIGHT_LED_SUFFIX);
>> + name = devm_kzalloc(&dev->dev, name_sz, GFP_KERNEL);
>> + if (!name) {
>> + ret = -ENOMEM;
>> + goto fail_backlight;
>> + }
>> + snprintf(name, name_sz, "%s" K90_BACKLIGHT_LED_SUFFIX,
>> + dev_name(&dev->dev));
>> + led = &drvdata->backlight;
>> + led->cdev.name = name;
>> + led->cdev.max_brightness = 3;
>> + led->cdev.brightness_set = k90_brightness_set;
>> + led->cdev.brightness_get = k90_brightness_get;
>> + INIT_WORK(&led->work, k90_backlight_work);
>> + ret = led_classdev_register(&dev->dev, &led->cdev);
> ... and also on LED subsystem, but this dependency is not expressed in
> Kconfig.
>
> Thanks,
>
^ permalink raw reply
* [PATCH v1 0/8] VFS: In-kernel copy system call
From: Anna Schumaker @ 2015-09-04 20:16 UTC (permalink / raw)
To: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-btrfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, zab-ugsP4Wv/S6ZeoWH0uzbU5w,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, clm-b10kYP2dOMg,
darrick.wong-QHcLZuEGTsvQT0dZR+AlfA,
mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
andros-HgOvQuBEEgTQT0dZR+AlfA, hch-wEGCiKHe2LqWVfeAwA7xHQ
Copy system calls came up during Plumbers a couple of weeks ago, because
several filesystems (including NFS and XFS) are currently working on copy
acceleration implementations. We haven't heard from Zach Brown in a while,
so I volunteered to push his patches upstream so individual filesystems
don't need to keep writing their own ioctls.
The first three patches are a simple reposting of Zach's patches from several
months ago, with one minor error code fix. The remaining patches add in a
fallback mechanism when filesystems don't provide a copy function. This is
especially useful when doing a server-side copy on NFS (using the new COPY
operation in NFS v4.2). This fallback can be disabled by passing the flag
COPY_REFLINK to the system call.
The last patch is a man page patch documenting this new system call,
including an example program.
I tested the fallback option by using /dev/urandom to generate files of
varying sizes and copying them. I compared the time to copy against that
of `cp` just to see if there is a noticable difference. I found that
runtimes are roughly the same, but in-kernel copy tends to use less of
the cpu. Values in the tables below are averages across multiple trials.
/usr/bin/cp | 512 MB | 1024 MB | 1536 MB | 2048 MB
-------------|-----------|-----------|-----------|-----------
user | 0.00s | 0.00s | 0.00s | 0.00s
system | 0.32s | 0.52s | 1.04s | 1.04s
cpu | 73% | 69% | 62% | 62%
total | 0.446 | 0.757 | 1.197 | 1.667
VFS copy | 512 MB | 1024 MB | 1536 MB | 2048 MB
-------------|-----------|-----------|-----------|-----------
user | 0.00s | 0.00s | 0.00s | 0.00s
system | 0.33s | 0.49s | 0.76s | 0.99s
cpu | 77% | 62% | 60% | 59%
total | 0.422 | 0.777 | 1.267 | 1.655
Questions? Comments? Thoughts?
Anna
Anna Schumaker (5):
btrfs: Add mountpoint checking during btrfs_copy_file_range
vfs: Remove copy_file_range mountpoint checks
vfs: Copy should check len after file open mode
vfs: Copy should use file_out rather than file_in
vfs: Fall back on splice if no copy function defined
Zach Brown (3):
vfs: add copy_file_range syscall and vfs helper
x86: add sys_copy_file_range to syscall tables
btrfs: add .copy_file_range file operation
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
fs/btrfs/ctree.h | 3 +
fs/btrfs/file.c | 1 +
fs/btrfs/ioctl.c | 95 ++++++++++++++----------
fs/read_write.c | 132 +++++++++++++++++++++++++++++++++
include/linux/copy.h | 6 ++
include/linux/fs.h | 3 +
include/uapi/asm-generic/unistd.h | 4 +-
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/copy.h | 6 ++
kernel/sys_ni.c | 1 +
12 files changed, 214 insertions(+), 40 deletions(-)
create mode 100644 include/linux/copy.h
create mode 100644 include/uapi/linux/copy.h
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* [PATCH v1 1/9] vfs: add copy_file_range syscall and vfs helper
From: Anna Schumaker @ 2015-09-04 20:16 UTC (permalink / raw)
To: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-btrfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, zab-ugsP4Wv/S6ZeoWH0uzbU5w,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, clm-b10kYP2dOMg,
darrick.wong-QHcLZuEGTsvQT0dZR+AlfA,
mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
andros-HgOvQuBEEgTQT0dZR+AlfA, hch-wEGCiKHe2LqWVfeAwA7xHQ
In-Reply-To: <1441397823-1203-1-git-send-email-Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
From: Zach Brown <zab-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Add a copy_file_range() system call for offloading copies between
regular files.
This gives an interface to underlying layers of the storage stack which
can copy without reading and writing all the data. There are a few
candidates that should support copy offloading in the nearer term:
- btrfs shares extent references with its clone ioctl
- NFS has patches to add a COPY command which copies on the server
- SCSI has a family of XCOPY commands which copy in the device
This system call avoids the complexity of also accelerating the creation
of the destination file by operating on an existing destination file
descriptor, not a path.
Currently the high level vfs entry point limits copy offloading to files
on the same mount and super (and not in the same file). This can be
relaxed if we get implementations which can copy between file systems
safely.
Signed-off-by: Zach Brown <zab-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[Anna Schumaker: Change -EINVAL to -EBADF during file verification]
Signed-off-by: Anna Schumaker <Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
---
fs/read_write.c | 129 ++++++++++++++++++++++++++++++++++++++
include/linux/fs.h | 3 +
include/uapi/asm-generic/unistd.h | 4 +-
kernel/sys_ni.c | 1 +
4 files changed, 136 insertions(+), 1 deletion(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 819ef3f..82c4933 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -16,6 +16,7 @@
#include <linux/pagemap.h>
#include <linux/splice.h>
#include <linux/compat.h>
+#include <linux/mount.h>
#include "internal.h"
#include <asm/uaccess.h>
@@ -1327,3 +1328,131 @@ COMPAT_SYSCALL_DEFINE4(sendfile64, int, out_fd, int, in_fd,
return do_sendfile(out_fd, in_fd, NULL, count, 0);
}
#endif
+
+/*
+ * copy_file_range() differs from regular file read and write in that it
+ * specifically allows return partial success. When it does so is up to
+ * the copy_file_range method.
+ */
+ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
+ struct file *file_out, loff_t pos_out,
+ size_t len, int flags)
+{
+ struct inode *inode_in;
+ struct inode *inode_out;
+ ssize_t ret;
+
+ if (flags)
+ return -EINVAL;
+
+ if (len == 0)
+ return 0;
+
+ /* copy_file_range allows full ssize_t len, ignoring MAX_RW_COUNT */
+ ret = rw_verify_area(READ, file_in, &pos_in, len);
+ if (ret >= 0)
+ ret = rw_verify_area(WRITE, file_out, &pos_out, len);
+ if (ret < 0)
+ return ret;
+
+ if (!(file_in->f_mode & FMODE_READ) ||
+ !(file_out->f_mode & FMODE_WRITE) ||
+ (file_out->f_flags & O_APPEND) ||
+ !file_in->f_op || !file_in->f_op->copy_file_range)
+ return -EBADF;
+
+ inode_in = file_inode(file_in);
+ inode_out = file_inode(file_out);
+
+ /* make sure offsets don't wrap and the input is inside i_size */
+ if (pos_in + len < pos_in || pos_out + len < pos_out ||
+ pos_in + len > i_size_read(inode_in))
+ return -EINVAL;
+
+ /* this could be relaxed once a method supports cross-fs copies */
+ if (inode_in->i_sb != inode_out->i_sb ||
+ file_in->f_path.mnt != file_out->f_path.mnt)
+ return -EXDEV;
+
+ /* forbid ranges in the same file */
+ if (inode_in == inode_out)
+ return -EINVAL;
+
+ ret = mnt_want_write_file(file_out);
+ if (ret)
+ return ret;
+
+ ret = file_in->f_op->copy_file_range(file_in, pos_in, file_out, pos_out,
+ len, flags);
+ if (ret > 0) {
+ fsnotify_access(file_in);
+ add_rchar(current, ret);
+ fsnotify_modify(file_out);
+ add_wchar(current, ret);
+ }
+ inc_syscr(current);
+ inc_syscw(current);
+
+ mnt_drop_write_file(file_out);
+
+ return ret;
+}
+EXPORT_SYMBOL(vfs_copy_file_range);
+
+SYSCALL_DEFINE6(copy_file_range, int, fd_in, loff_t __user *, off_in,
+ int, fd_out, loff_t __user *, off_out,
+ size_t, len, unsigned int, flags)
+{
+ loff_t pos_in;
+ loff_t pos_out;
+ struct fd f_in;
+ struct fd f_out;
+ ssize_t ret;
+
+ f_in = fdget(fd_in);
+ f_out = fdget(fd_out);
+ if (!f_in.file || !f_out.file) {
+ ret = -EBADF;
+ goto out;
+ }
+
+ ret = -EFAULT;
+ if (off_in) {
+ if (copy_from_user(&pos_in, off_in, sizeof(loff_t)))
+ goto out;
+ } else {
+ pos_in = f_in.file->f_pos;
+ }
+
+ if (off_out) {
+ if (copy_from_user(&pos_out, off_out, sizeof(loff_t)))
+ goto out;
+ } else {
+ pos_out = f_out.file->f_pos;
+ }
+
+ ret = vfs_copy_file_range(f_in.file, pos_in, f_out.file, pos_out, len,
+ flags);
+ if (ret > 0) {
+ pos_in += ret;
+ pos_out += ret;
+
+ if (off_in) {
+ if (copy_to_user(off_in, &pos_in, sizeof(loff_t)))
+ ret = -EFAULT;
+ } else {
+ f_in.file->f_pos = pos_in;
+ }
+
+ if (off_out) {
+ if (copy_to_user(off_out, &pos_out, sizeof(loff_t)))
+ ret = -EFAULT;
+ } else {
+ f_out.file->f_pos = pos_out;
+ }
+ }
+out:
+ fdput(f_in);
+ fdput(f_out);
+ return ret;
+}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index cc008c3..c97aed8 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1631,6 +1631,7 @@ struct file_operations {
#ifndef CONFIG_MMU
unsigned (*mmap_capabilities)(struct file *);
#endif
+ ssize_t (*copy_file_range)(struct file *, loff_t, struct file *, loff_t, size_t, int);
};
struct inode_operations {
@@ -1684,6 +1685,8 @@ extern ssize_t vfs_readv(struct file *, const struct iovec __user *,
unsigned long, loff_t *);
extern ssize_t vfs_writev(struct file *, const struct iovec __user *,
unsigned long, loff_t *);
+extern ssize_t vfs_copy_file_range(struct file *, loff_t , struct file *,
+ loff_t, size_t, int);
struct super_operations {
struct inode *(*alloc_inode)(struct super_block *sb);
diff --git a/include/uapi/asm-generic/unistd.h b/include/uapi/asm-generic/unistd.h
index e016bd9..2b60f0c 100644
--- a/include/uapi/asm-generic/unistd.h
+++ b/include/uapi/asm-generic/unistd.h
@@ -709,9 +709,11 @@ __SYSCALL(__NR_memfd_create, sys_memfd_create)
__SYSCALL(__NR_bpf, sys_bpf)
#define __NR_execveat 281
__SC_COMP(__NR_execveat, sys_execveat, compat_sys_execveat)
+#define __NR_copy_file_range 282
+__SYSCALL(__NR_copy_file_range, sys_copy_file_range)
#undef __NR_syscalls
-#define __NR_syscalls 282
+#define __NR_syscalls 283
/*
* All syscalls below here should go away really,
diff --git a/kernel/sys_ni.c b/kernel/sys_ni.c
index 7995ef5..4e01cd9 100644
--- a/kernel/sys_ni.c
+++ b/kernel/sys_ni.c
@@ -173,6 +173,7 @@ cond_syscall(sys_setfsuid);
cond_syscall(sys_setfsgid);
cond_syscall(sys_capget);
cond_syscall(sys_capset);
+cond_syscall(sys_copy_file_range);
/* arch-specific weak syscall entries */
cond_syscall(sys_pciconfig_read);
--
2.5.1
^ permalink raw reply related
* [PATCH v1 2/8] x86: add sys_copy_file_range to syscall tables
From: Anna Schumaker @ 2015-09-04 20:16 UTC (permalink / raw)
To: linux-nfs, linux-btrfs, linux-fsdevel, linux-api, zab, viro, clm,
darrick.wong, mtk.manpages, andros, hch
In-Reply-To: <1441397823-1203-1-git-send-email-Anna.Schumaker@Netapp.com>
From: Zach Brown <zab@redhat.com>
Add sys_copy_file_range to the x86 syscall tables.
Signed-off-by: Zach Brown <zab@redhat.com>
Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
arch/x86/entry/syscalls/syscall_32.tbl | 1 +
arch/x86/entry/syscalls/syscall_64.tbl | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/x86/entry/syscalls/syscall_32.tbl b/arch/x86/entry/syscalls/syscall_32.tbl
index ef8187f..2f5e1e0 100644
--- a/arch/x86/entry/syscalls/syscall_32.tbl
+++ b/arch/x86/entry/syscalls/syscall_32.tbl
@@ -365,3 +365,4 @@
356 i386 memfd_create sys_memfd_create
357 i386 bpf sys_bpf
358 i386 execveat sys_execveat stub32_execveat
+359 i386 copy_file_range sys_copy_file_range
diff --git a/arch/x86/entry/syscalls/syscall_64.tbl b/arch/x86/entry/syscalls/syscall_64.tbl
index 9ef32d5..b2101de 100644
--- a/arch/x86/entry/syscalls/syscall_64.tbl
+++ b/arch/x86/entry/syscalls/syscall_64.tbl
@@ -329,6 +329,7 @@
320 common kexec_file_load sys_kexec_file_load
321 common bpf sys_bpf
322 64 execveat stub_execveat
+323 common copy_file_range sys_copy_file_range
#
# x32-specific system call numbers start at 512 to avoid cache impact
--
2.5.1
^ permalink raw reply related
* [PATCH v1 3/8] btrfs: add .copy_file_range file operation
From: Anna Schumaker @ 2015-09-04 20:16 UTC (permalink / raw)
To: linux-nfs, linux-btrfs, linux-fsdevel, linux-api, zab, viro, clm,
darrick.wong, mtk.manpages, andros, hch
In-Reply-To: <1441397823-1203-1-git-send-email-Anna.Schumaker@Netapp.com>
From: Zach Brown <zab@redhat.com>
This rearranges the existing COPY_RANGE ioctl implementation so that the
.copy_file_range file operation can call the core loop that copies file
data extent items.
The extent copying loop is lifted up into its own function. It retains
the core btrfs error checks that should be shared.
Signed-off-by: Zach Brown <zab@redhat.com>
Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
fs/btrfs/ctree.h | 3 ++
fs/btrfs/file.c | 1 +
fs/btrfs/ioctl.c | 91 ++++++++++++++++++++++++++++++++------------------------
3 files changed, 56 insertions(+), 39 deletions(-)
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index aac314e..e09d4e2 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -4000,6 +4000,9 @@ int btrfs_dirty_pages(struct btrfs_root *root, struct inode *inode,
loff_t pos, size_t write_bytes,
struct extent_state **cached);
int btrfs_fdatawrite_range(struct inode *inode, loff_t start, loff_t end);
+ssize_t btrfs_copy_file_range(struct file *file_in, loff_t pos_in,
+ struct file *file_out, loff_t pos_out,
+ size_t len, int flags);
/* tree-defrag.c */
int btrfs_defrag_leaves(struct btrfs_trans_handle *trans,
diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index b823fac..b05449c 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -2816,6 +2816,7 @@ const struct file_operations btrfs_file_operations = {
#ifdef CONFIG_COMPAT
.compat_ioctl = btrfs_ioctl,
#endif
+ .copy_file_range = btrfs_copy_file_range,
};
void btrfs_auto_defrag_exit(void)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 0770c91..62ae286 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3719,17 +3719,16 @@ out:
return ret;
}
-static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,
- u64 off, u64 olen, u64 destoff)
+static noinline int btrfs_clone_files(struct file *file, struct file *file_src,
+ u64 off, u64 olen, u64 destoff)
{
struct inode *inode = file_inode(file);
+ struct inode *src = file_inode(file_src);
struct btrfs_root *root = BTRFS_I(inode)->root;
- struct fd src_file;
- struct inode *src;
int ret;
u64 len = olen;
u64 bs = root->fs_info->sb->s_blocksize;
- int same_inode = 0;
+ int same_inode = src == inode;
/*
* TODO:
@@ -3742,49 +3741,20 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,
* be either compressed or non-compressed.
*/
- /* the destination must be opened for writing */
- if (!(file->f_mode & FMODE_WRITE) || (file->f_flags & O_APPEND))
- return -EINVAL;
-
if (btrfs_root_readonly(root))
return -EROFS;
- ret = mnt_want_write_file(file);
- if (ret)
- return ret;
-
- src_file = fdget(srcfd);
- if (!src_file.file) {
- ret = -EBADF;
- goto out_drop_write;
- }
-
- ret = -EXDEV;
- if (src_file.file->f_path.mnt != file->f_path.mnt)
- goto out_fput;
-
- src = file_inode(src_file.file);
-
- ret = -EINVAL;
- if (src == inode)
- same_inode = 1;
-
- /* the src must be open for reading */
- if (!(src_file.file->f_mode & FMODE_READ))
- goto out_fput;
+ if (file_src->f_path.mnt != file->f_path.mnt ||
+ src->i_sb != inode->i_sb)
+ return -EXDEV;
/* don't make the dst file partly checksummed */
if ((BTRFS_I(src)->flags & BTRFS_INODE_NODATASUM) !=
(BTRFS_I(inode)->flags & BTRFS_INODE_NODATASUM))
- goto out_fput;
+ return -EINVAL;
- ret = -EISDIR;
if (S_ISDIR(src->i_mode) || S_ISDIR(inode->i_mode))
- goto out_fput;
-
- ret = -EXDEV;
- if (src->i_sb != inode->i_sb)
- goto out_fput;
+ return -EISDIR;
if (!same_inode) {
if (inode < src) {
@@ -3877,6 +3847,49 @@ out_unlock:
} else {
mutex_unlock(&src->i_mutex);
}
+ return ret;
+}
+
+ssize_t btrfs_copy_file_range(struct file *file_in, loff_t pos_in,
+ struct file *file_out, loff_t pos_out,
+ size_t len, int flags)
+{
+ ssize_t ret;
+
+ ret = btrfs_clone_files(file_out, file_in, pos_in, len, pos_out);
+ if (ret == 0)
+ ret = len;
+ return ret;
+}
+
+static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd,
+ u64 off, u64 olen, u64 destoff)
+{
+ struct fd src_file;
+ int ret;
+
+ /* the destination must be opened for writing */
+ if (!(file->f_mode & FMODE_WRITE) || (file->f_flags & O_APPEND))
+ return -EINVAL;
+
+ ret = mnt_want_write_file(file);
+ if (ret)
+ return ret;
+
+ src_file = fdget(srcfd);
+ if (!src_file.file) {
+ ret = -EBADF;
+ goto out_drop_write;
+ }
+
+ /* the src must be open for reading */
+ if (!(src_file.file->f_mode & FMODE_READ)) {
+ ret = -EINVAL;
+ goto out_fput;
+ }
+
+ ret = btrfs_clone_files(file, src_file.file, off, olen, destoff);
+
out_fput:
fdput(src_file);
out_drop_write:
--
2.5.1
^ permalink raw reply related
* [PATCH v1 4/8] btrfs: Add mountpoint checking during btrfs_copy_file_range
From: Anna Schumaker @ 2015-09-04 20:16 UTC (permalink / raw)
To: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-btrfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, zab-ugsP4Wv/S6ZeoWH0uzbU5w,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, clm-b10kYP2dOMg,
darrick.wong-QHcLZuEGTsvQT0dZR+AlfA,
mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
andros-HgOvQuBEEgTQT0dZR+AlfA, hch-wEGCiKHe2LqWVfeAwA7xHQ
In-Reply-To: <1441397823-1203-1-git-send-email-Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
We need to verify that both the source and the destination files are
part of the same filesystem, otherwise we can't create a reflink.
Signed-off-by: Anna Schumaker <Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
---
fs/btrfs/ioctl.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
index 62ae286..9c0c955 100644
--- a/fs/btrfs/ioctl.c
+++ b/fs/btrfs/ioctl.c
@@ -3856,6 +3856,10 @@ ssize_t btrfs_copy_file_range(struct file *file_in, loff_t pos_in,
{
ssize_t ret;
+ if (inode_in->i_sb != inode_out->i_sb ||
+ file_in->f_path.mnt != file_out->f_path.mnt)
+ return -EXDEV;
+
ret = btrfs_clone_files(file_out, file_in, pos_in, len, pos_out);
if (ret == 0)
ret = len;
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v1 5/8] vfs: Remove copy_file_range mountpoint checks
From: Anna Schumaker @ 2015-09-04 20:16 UTC (permalink / raw)
To: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-btrfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, zab-ugsP4Wv/S6ZeoWH0uzbU5w,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, clm-b10kYP2dOMg,
darrick.wong-QHcLZuEGTsvQT0dZR+AlfA,
mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
andros-HgOvQuBEEgTQT0dZR+AlfA, hch-wEGCiKHe2LqWVfeAwA7xHQ
In-Reply-To: <1441397823-1203-1-git-send-email-Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
I still want to do an in-kernel copy even if the files are on different
mountpoints, and NFS has a "server to server" copy that expects two
files on different mountpoints. Let's have individual filesystems
implement this check instead.
Signed-off-by: Anna Schumaker <Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
---
fs/read_write.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 82c4933..922ca58 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1369,11 +1369,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
pos_in + len > i_size_read(inode_in))
return -EINVAL;
- /* this could be relaxed once a method supports cross-fs copies */
- if (inode_in->i_sb != inode_out->i_sb ||
- file_in->f_path.mnt != file_out->f_path.mnt)
- return -EXDEV;
-
/* forbid ranges in the same file */
if (inode_in == inode_out)
return -EINVAL;
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v1 6/8] vfs: Copy should check len after file open mode
From: Anna Schumaker @ 2015-09-04 20:17 UTC (permalink / raw)
To: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-btrfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, zab-ugsP4Wv/S6ZeoWH0uzbU5w,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, clm-b10kYP2dOMg,
darrick.wong-QHcLZuEGTsvQT0dZR+AlfA,
mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
andros-HgOvQuBEEgTQT0dZR+AlfA, hch-wEGCiKHe2LqWVfeAwA7xHQ
In-Reply-To: <1441397823-1203-1-git-send-email-Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
I don't think it makes sense to report that a copy succeeded if the
files aren't open properly.
Signed-off-by: Anna Schumaker <Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
---
fs/read_write.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 922ca58..59637ed 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1345,9 +1345,6 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
if (flags)
return -EINVAL;
- if (len == 0)
- return 0;
-
/* copy_file_range allows full ssize_t len, ignoring MAX_RW_COUNT */
ret = rw_verify_area(READ, file_in, &pos_in, len);
if (ret >= 0)
@@ -1373,6 +1370,9 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
if (inode_in == inode_out)
return -EINVAL;
+ if (len == 0)
+ return 0;
+
ret = mnt_want_write_file(file_out);
if (ret)
return ret;
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v1 7/8] vfs: Copy should use file_out rather than file_in
From: Anna Schumaker @ 2015-09-04 20:17 UTC (permalink / raw)
To: linux-nfs, linux-btrfs, linux-fsdevel, linux-api, zab, viro, clm,
darrick.wong, mtk.manpages, andros, hch
In-Reply-To: <1441397823-1203-1-git-send-email-Anna.Schumaker@Netapp.com>
The way to think about this is that the destination filesystem reads the
data from the source file and processes it accordingly. This is
especially important to avoid an infinate loop when doing a "server to
server" copy on NFS.
Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
---
fs/read_write.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/fs/read_write.c b/fs/read_write.c
index 59637ed..9dafb7f 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -1355,7 +1355,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
if (!(file_in->f_mode & FMODE_READ) ||
!(file_out->f_mode & FMODE_WRITE) ||
(file_out->f_flags & O_APPEND) ||
- !file_in->f_op || !file_in->f_op->copy_file_range)
+ !file_out->f_op || !file_out->f_op->copy_file_range)
return -EBADF;
inode_in = file_inode(file_in);
@@ -1377,8 +1377,8 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
if (ret)
return ret;
- ret = file_in->f_op->copy_file_range(file_in, pos_in, file_out, pos_out,
- len, flags);
+ ret = file_out->f_op->copy_file_range(file_in, pos_in, file_out, pos_out,
+ len, flags);
if (ret > 0) {
fsnotify_access(file_in);
add_rchar(current, ret);
--
2.5.1
^ permalink raw reply related
* [PATCH v1 8/8] vfs: Fall back on splice if no copy function defined
From: Anna Schumaker @ 2015-09-04 20:17 UTC (permalink / raw)
To: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-btrfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, zab-ugsP4Wv/S6ZeoWH0uzbU5w,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, clm-b10kYP2dOMg,
darrick.wong-QHcLZuEGTsvQT0dZR+AlfA,
mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
andros-HgOvQuBEEgTQT0dZR+AlfA, hch-wEGCiKHe2LqWVfeAwA7xHQ
In-Reply-To: <1441397823-1203-1-git-send-email-Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
The NFS server will need a fallback for filesystems that don't have any
kind of copy acceleration yet, and it should be generally useful to have
an in-kernel copy to avoid lots of switches between kernel and
user space. Users who only want a reflink can pass the COPY_REFLINK
flag to disable the fallback.
Signed-off-by: Anna Schumaker <Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
---
fs/read_write.c | 16 ++++++++++++----
include/linux/copy.h | 6 ++++++
include/uapi/linux/Kbuild | 1 +
include/uapi/linux/copy.h | 6 ++++++
4 files changed, 25 insertions(+), 4 deletions(-)
create mode 100644 include/linux/copy.h
create mode 100644 include/uapi/linux/copy.h
diff --git a/fs/read_write.c b/fs/read_write.c
index 9dafb7f..bd7e7e2 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -7,6 +7,7 @@
#include <linux/slab.h>
#include <linux/stat.h>
#include <linux/fcntl.h>
+#include <linux/copy.h>
#include <linux/file.h>
#include <linux/uio.h>
#include <linux/fsnotify.h>
@@ -1342,7 +1343,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
struct inode *inode_out;
ssize_t ret;
- if (flags)
+ if (!((flags == 0) || (flags == COPY_REFLINK)))
return -EINVAL;
/* copy_file_range allows full ssize_t len, ignoring MAX_RW_COUNT */
@@ -1355,7 +1356,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
if (!(file_in->f_mode & FMODE_READ) ||
!(file_out->f_mode & FMODE_WRITE) ||
(file_out->f_flags & O_APPEND) ||
- !file_out->f_op || !file_out->f_op->copy_file_range)
+ !file_in->f_op)
return -EBADF;
inode_in = file_inode(file_in);
@@ -1377,8 +1378,15 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
if (ret)
return ret;
- ret = file_out->f_op->copy_file_range(file_in, pos_in, file_out, pos_out,
- len, flags);
+ ret = -EOPNOTSUPP;
+ if (file_out->f_op->copy_file_range)
+ ret = file_out->f_op->copy_file_range(file_in, pos_in, file_out,
+ pos_out, len, flags);
+ if ((ret < 0) && !(flags & COPY_REFLINK)) {
+ file_start_write(file_out);
+ ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out, len, 0);
+ file_end_write(file_out);
+ }
if (ret > 0) {
fsnotify_access(file_in);
add_rchar(current, ret);
diff --git a/include/linux/copy.h b/include/linux/copy.h
new file mode 100644
index 0000000..fd54543
--- /dev/null
+++ b/include/linux/copy.h
@@ -0,0 +1,6 @@
+#ifndef _LINUX_COPY_H
+#define _LINUX_COPY_H
+
+#include <uapi/linux/copy.h>
+
+#endif /* _LINUX_COPY_H */
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 1ff9942..b6109f3 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -90,6 +90,7 @@ header-y += coda_psdev.h
header-y += coff.h
header-y += connector.h
header-y += const.h
+header-y += copy.h
header-y += cramfs_fs.h
header-y += cuda.h
header-y += cyclades.h
diff --git a/include/uapi/linux/copy.h b/include/uapi/linux/copy.h
new file mode 100644
index 0000000..68f3d65
--- /dev/null
+++ b/include/uapi/linux/copy.h
@@ -0,0 +1,6 @@
+#ifndef _UAPI_LINUX_COPY_H
+#define _UAPI_LINUX_COPY_H
+
+#define COPY_REFLINK 1 /* only perform a reflink */
+
+#endif /* _UAPI_LINUX_COPY_H */
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* [PATCH v1 9/8] copy_file_range.2: New page documenting copy_file_range()
From: Anna Schumaker @ 2015-09-04 20:17 UTC (permalink / raw)
To: linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-btrfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, zab-ugsP4Wv/S6ZeoWH0uzbU5w,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, clm-b10kYP2dOMg,
darrick.wong-QHcLZuEGTsvQT0dZR+AlfA,
mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
andros-HgOvQuBEEgTQT0dZR+AlfA, hch-wEGCiKHe2LqWVfeAwA7xHQ
In-Reply-To: <1441397823-1203-1-git-send-email-Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
copy_file_range() is a new system call for copying ranges of data
completely in the kernel. This gives filesystems an opportunity to
implement some kind of "copy acceleration", such as reflinks or
server-side-copy (in the case of NFS).
Signed-off-by: Anna Schumaker <Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
---
man2/copy_file_range.2 | 168 +++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 168 insertions(+)
create mode 100644 man2/copy_file_range.2
diff --git a/man2/copy_file_range.2 b/man2/copy_file_range.2
new file mode 100644
index 0000000..4a4cb73
--- /dev/null
+++ b/man2/copy_file_range.2
@@ -0,0 +1,168 @@
+.\"This manpage is Copyright (C) 2015 Anna Schumaker <Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
+.TH COPY 2 2015-8-31 "Linux" "Linux Programmer's Manual"
+.SH NAME
+copy_file_range \- Copy a range of data from one file to another
+.SH SYNOPSIS
+.nf
+.B #include <linux/copy.h>
+.B #include <sys/syscall.h>
+.B #include <unistd.h>
+
+.BI "ssize_t syscall(__NR_copy_file_range, int " fd_in ", loff_t * " off_in ",
+.BI " int " fd_out ", loff_t * " off_out ", size_t " len ",
+.BI " unsigned int " flags );
+.fi
+.SH DESCRIPTION
+The
+.BR copy_file_range ()
+system call performs an in-kernel copy between two file descriptors
+without all that tedious mucking about in userspace.
+It copies up to
+.I len
+bytes of data from file descriptor
+.I fd_in
+to file descriptor
+.I fd_out
+at
+.IR off_out .
+The file descriptors must not refer to the same file.
+
+The following semantics apply for
+.IR fd_in ,
+and similar statements apply to
+.IR off_out :
+.IP * 3
+If
+.I off_in
+is NULL, then bytes are read from
+.I fd_in
+starting from the current file offset and the current
+file offset is adjusted appropriately.
+.IP *
+If
+.I off_in
+is not NULL, then
+.I off_in
+must point to a buffer that specifies the starting
+offset where bytes from
+.I fd_in
+will be read. The current file offset of
+.I fd_in
+is not changed, but
+.I off_in
+is adjusted appropriately.
+.PP
+The default behavior of
+.BR copy_file_range ()
+is filesystem specific, and might result in creating a
+copy-on-write reflink.
+In the event that a given filesystem does not implement
+any form of copy acceleration, the kernel will perform
+a deep copy of the requested range by reading bytes from
+.I fd_in
+and writing them to
+.IR fd_out .
+
+Currently, Linux only supports the following flag:
+.TP 1.9i
+.B COPY_REFLINK
+Only perform the copy if the filesystem can do it as a reflink.
+Do not fall back on performing a deep copy.
+.SH RETURN VALUE
+Upon successful completion,
+.BR copy_file_range ()
+will return the number of bytes copied between files.
+This could be less than the length originally requested.
+
+On error,
+.BR copy_file_range ()
+returns \-1 and
+.I errno
+is set to indicate the error.
+.SH ERRORS
+.TP
+.B EBADF
+One or more file descriptors are not valid,
+or do not have proper read-write mode.
+.TP
+.B EINVAL
+Requested range extends beyond the end of the file;
+.I flags
+argument is set to an invalid value.
+.TP
+.B EOPNOTSUPP
+.B COPY_REFLINK
+was specified in
+.IR flags ,
+but the target filesystem does not support reflinks.
+.TP
+.B EXDEV
+Target filesystem doesn't support cross-filesystem copies.
+.SH VERSIONS
+The
+.BR copy_file_range ()
+system call first appeared in Linux 4.3.
+.SH CONFORMING TO
+The
+.BR copy_file_range ()
+system call is a nonstandard Linux extension.
+.SH EXAMPLE
+.nf
+
+#define _GNU_SOURCE
+#include <fcntl.h>
+#include <linux/copy.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/stat.h>
+#include <sys/syscall.h>
+#include <unistd.h>
+
+
+int main(int argc, char **argv)
+{
+ int fd_in, fd_out;
+ struct stat stat;
+ loff_t len, ret;
+
+ if (argc != 3) {
+ fprintf(stderr, "Usage: %s <pathname> <pathname>\n", argv[0]);
+ exit(EXIT_FAILURE);
+ }
+
+ fd_in = open(argv[1], O_RDONLY);
+ if (fd_in == -1) {
+ perror("open (argv[1])");
+ exit(EXIT_FAILURE);
+ }
+
+ if (fstat(fd_in, &stat) == -1) {
+ perror("fstat");
+ exit(EXIT_FAILURE);
+ }
+ len = stat.st_size;
+
+ fd_out = open(argv[2], O_WRONLY | O_CREAT, 0644);
+ if (fd_out == -1) {
+ perror("open (argv[2])");
+ exit(EXIT_FAILURE);
+ }
+
+ do {
+ ret = syscall(__NR_copy_file_range, fd_in, NULL,
+ fd_out, NULL, len, 0);
+ if (ret == -1) {
+ perror("copy_file_range");
+ exit(EXIT_FAILURE);
+ }
+
+ len -= ret;
+ } while (len > 0);
+
+ close(fd_in);
+ close(fd_out);
+ exit(EXIT_SUCCESS);
+}
+.fi
+.SH SEE ALSO
+.BR splice (2)
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PATCH 5/6] seccomp: add a way to attach a filter via eBPF fd
From: Kees Cook @ 2015-09-04 20:41 UTC (permalink / raw)
To: Tycho Andersen, Linux API
Cc: Alexei Starovoitov, Will Drewry, Oleg Nesterov, Andy Lutomirski,
Pavel Emelyanov, Serge E. Hallyn, Daniel Borkmann, LKML,
Network Development
In-Reply-To: <1441382664-17437-6-git-send-email-tycho.andersen-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
On Fri, Sep 4, 2015 at 9:04 AM, Tycho Andersen
<tycho.andersen-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> This is the final bit needed to support seccomp filters created via the bpf
> syscall.
>
> One concern with this patch is exactly what the interface should look like
> for users, since seccomp()'s second argument is a pointer, we could ask
> people to pass a pointer to the fd, but implies we might write to it which
> seems impolite. Right now we cast the pointer (and force the user to cast
> it), which generates ugly warnings. I'm not sure what the right answer is
> here.
>
> Signed-off-by: Tycho Andersen <tycho.andersen-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> CC: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> CC: Will Drewry <wad-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> CC: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> CC: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> CC: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> CC: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
> ---
> include/linux/seccomp.h | 3 +-
> include/uapi/linux/seccomp.h | 1 +
> kernel/seccomp.c | 70 ++++++++++++++++++++++++++++++++++++--------
> 3 files changed, 61 insertions(+), 13 deletions(-)
>
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index d1a86ed..a725dd5 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -3,7 +3,8 @@
>
> #include <uapi/linux/seccomp.h>
>
> -#define SECCOMP_FILTER_FLAG_MASK (SECCOMP_FILTER_FLAG_TSYNC)
> +#define SECCOMP_FILTER_FLAG_MASK (\
> + SECCOMP_FILTER_FLAG_TSYNC | SECCOMP_FILTER_FLAG_EBPF)
>
> #ifdef CONFIG_SECCOMP
>
> diff --git a/include/uapi/linux/seccomp.h b/include/uapi/linux/seccomp.h
> index 0f238a4..c29a423 100644
> --- a/include/uapi/linux/seccomp.h
> +++ b/include/uapi/linux/seccomp.h
> @@ -16,6 +16,7 @@
>
> /* Valid flags for SECCOMP_SET_MODE_FILTER */
> #define SECCOMP_FILTER_FLAG_TSYNC 1
> +#define SECCOMP_FILTER_FLAG_EBPF (1 << 1)
>
> /*
> * All BPF programs must return a 32-bit value.
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index a2c5b32..9c6bea6 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -355,17 +355,6 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
>
> BUG_ON(INT_MAX / fprog->len < sizeof(struct sock_filter));
>
> - /*
> - * Installing a seccomp filter requires that the task has
> - * CAP_SYS_ADMIN in its namespace or be running with no_new_privs.
> - * This avoids scenarios where unprivileged tasks can affect the
> - * behavior of privileged children.
> - */
> - if (!task_no_new_privs(current) &&
> - security_capable_noaudit(current_cred(), current_user_ns(),
> - CAP_SYS_ADMIN) != 0)
> - return ERR_PTR(-EACCES);
> -
> /* Allocate a new seccomp_filter */
> sfilter = kzalloc(sizeof(*sfilter), GFP_KERNEL | __GFP_NOWARN);
> if (!sfilter)
> @@ -509,6 +498,48 @@ static void seccomp_send_sigsys(int syscall, int reason)
> info.si_syscall = syscall;
> force_sig_info(SIGSYS, &info, current);
> }
> +
> +#ifdef CONFIG_BPF_SYSCALL
> +static struct seccomp_filter *seccomp_prepare_ebpf(const char __user *filter)
> +{
> + /* XXX: this cast generates a warning. should we make people pass in
> + * &fd, or is there some nicer way of doing this?
> + */
> + u32 fd = (u32) filter;
I think this is probably the right way to do it, modulo getting the
warning fixed. Let me invoke the great linux-api subscribers to get
some more opinions.
tl;dr: adding SECCOMP_FILTER_FLAG_EBPF to the flags changes the
pointer argument into an fd argument. Is this sane, should it be a
pointer to an fd, or should it not be a flag at all, creating a new
seccomp command instead (SECCOMP_MODE_FILTER_EBPF)?
-Kees
> + struct seccomp_filter *ret;
> + struct bpf_prog *prog;
> +
> + prog = bpf_prog_get(fd);
> + if (IS_ERR(prog))
> + return (struct seccomp_filter *) prog;
> +
> + if (prog->type != BPF_PROG_TYPE_SECCOMP) {
> + bpf_prog_put(prog);
> + return ERR_PTR(-EINVAL);
> + }
> +
> + ret = kzalloc(sizeof(*ret), GFP_KERNEL | __GFP_NOWARN);
> + if (!ret) {
> + bpf_prog_put(prog);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + ret->prog = prog;
> + atomic_set(&ret->usage, 1);
> +
> + /* Intentionally don't bpf_prog_put() here, because the underlying prog
> + * is refcounted too and we're holding a reference from the struct
> + * seccomp_filter object.
> + */
> +
> + return ret;
> +}
> +#else
> +static struct seccomp_filter *seccomp_prepare_ebpf(const char __user *filter)
> +{
> + return ERR_PTR(-EINVAL);
> +}
> +#endif
> #endif /* CONFIG_SECCOMP_FILTER */
>
> /*
> @@ -775,8 +806,23 @@ static long seccomp_set_mode_filter(unsigned int flags,
> if (flags & ~SECCOMP_FILTER_FLAG_MASK)
> return -EINVAL;
>
> + /*
> + * Installing a seccomp filter requires that the task has
> + * CAP_SYS_ADMIN in its namespace or be running with no_new_privs.
> + * This avoids scenarios where unprivileged tasks can affect the
> + * behavior of privileged children.
> + */
> + if (!task_no_new_privs(current) &&
> + security_capable_noaudit(current_cred(), current_user_ns(),
> + CAP_SYS_ADMIN) != 0)
> + return -EACCES;
> +
> /* Prepare the new filter before holding any locks. */
> - prepared = seccomp_prepare_user_filter(filter);
> + if (flags & SECCOMP_FILTER_FLAG_EBPF)
> + prepared = seccomp_prepare_ebpf(filter);
> + else
> + prepared = seccomp_prepare_user_filter(filter);
> +
> if (IS_ERR(prepared))
> return PTR_ERR(prepared);
>
> --
> 2.1.4
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH v1 3/8] btrfs: add .copy_file_range file operation
From: Josef Bacik @ 2015-09-04 21:02 UTC (permalink / raw)
To: Anna Schumaker, linux-nfs-u79uwXL29TY76Z2rM5mHXA,
linux-btrfs-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA, zab-ugsP4Wv/S6ZeoWH0uzbU5w,
viro-RmSDqhL/yNMiFSDQTTA3OLVCufUGDwFn, clm-b10kYP2dOMg,
darrick.wong-QHcLZuEGTsvQT0dZR+AlfA,
mtk.manpages-Re5JQEeQqe8AvxtiuMwx3w,
andros-HgOvQuBEEgTQT0dZR+AlfA, hch-wEGCiKHe2LqWVfeAwA7xHQ
In-Reply-To: <1441397823-1203-4-git-send-email-Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
On 09/04/2015 04:16 PM, Anna Schumaker wrote:
> From: Zach Brown <zab-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> This rearranges the existing COPY_RANGE ioctl implementation so that the
> .copy_file_range file operation can call the core loop that copies file
> data extent items.
>
> The extent copying loop is lifted up into its own function. It retains
> the core btrfs error checks that should be shared.
>
> Signed-off-by: Zach Brown <zab-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Anna Schumaker <Anna.Schumaker-ZwjVKphTwtPQT0dZR+AlfA@public.gmane.org>
This bit looks fine to me,
Reviewed-by: Josef Bacik <jbacik-b10kYP2dOMg@public.gmane.org>
Thanks,
Josef
^ permalink raw reply
* Re: [PATCH v1 8/8] vfs: Fall back on splice if no copy function defined
From: Darrick J. Wong @ 2015-09-04 21:08 UTC (permalink / raw)
To: Anna Schumaker
Cc: linux-nfs, linux-btrfs, linux-fsdevel, linux-api, zab, viro, clm,
mtk.manpages, andros, hch
In-Reply-To: <1441397823-1203-9-git-send-email-Anna.Schumaker@Netapp.com>
On Fri, Sep 04, 2015 at 04:17:02PM -0400, Anna Schumaker wrote:
> The NFS server will need a fallback for filesystems that don't have any
> kind of copy acceleration yet, and it should be generally useful to have
> an in-kernel copy to avoid lots of switches between kernel and
> user space. Users who only want a reflink can pass the COPY_REFLINK
> flag to disable the fallback.
>
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
> fs/read_write.c | 16 ++++++++++++----
> include/linux/copy.h | 6 ++++++
> include/uapi/linux/Kbuild | 1 +
> include/uapi/linux/copy.h | 6 ++++++
> 4 files changed, 25 insertions(+), 4 deletions(-)
> create mode 100644 include/linux/copy.h
> create mode 100644 include/uapi/linux/copy.h
>
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 9dafb7f..bd7e7e2 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -7,6 +7,7 @@
> #include <linux/slab.h>
> #include <linux/stat.h>
> #include <linux/fcntl.h>
> +#include <linux/copy.h>
> #include <linux/file.h>
> #include <linux/uio.h>
> #include <linux/fsnotify.h>
> @@ -1342,7 +1343,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> struct inode *inode_out;
> ssize_t ret;
>
> - if (flags)
> + if (!((flags == 0) || (flags == COPY_REFLINK)))
I think it's a good idea for the copy_file_range flags to have the name of
the syscall in them at the beginning, both to try to avoid namespace collisions
and also to make it clear where the flag comes from. So, could we prefix the
values with "COPY_FILE_RANGE_" instead of just "COPY_"?
Also... I'm confused by what the structure of check implies about 'flags' --
are each of 'flags's values supposed to be mutually exclusive, or is it a
straight bitset like flags arguments tend to be?
Say we want to add dedupe and ponies as potential behavioral variants. Then
we'd end up with something like:
/* raw flags */
#define COPY_FILE_RANGE_SHARE_BLOCKS (1)
#define COPY_FILE_RANGE_CHECK_SAME (2)
#define COPY_FILE_RANGE_PONIES (4)
/* syntactic sugar */
#define COPY_FILE_RANGE_DEDUPE (COPY_FILE_RANGE_SHARE_BLOCKS | \
COPY_FILE_RANGE_CHECK_SAME)
#define COPY_FILE_RANGE_ALL (COPY_FILE_RANGE_SHARE_BLOCKS | \
COPY_FILE_RANGE_CHECK_SAME | \
COPY_FILE_RANGE_PONIES)
and in copy_file_range():
if (flags & ~COPY_FILE_RANGE_ALL)
return -EINVAL;
if ((flags & COPY_FILE_RANGE_CHECK_SAME) &&
!(flags & COPY_FILE_RANGE_SHARE_BLOCKS))
return -EINVAL; /* or is it return 0? */
if (flags & COPY_FILE_RANGE_PONIES)
panic();
if (flags & COPY_FILE_RANGE_CHECK_SAME)
check_same_contents(...);
err = vfs_copy_range(...);
if (err < 0 && !(flags & COPY_FILE_RANGE_SHARE_BLOCKS))
err = splice(...);
One hard part is figuring out just what actions we want userland to be able to
ask for. I can think of three: "share blocks via remapping" (i.e. reflink),
"share blocks via remapping but only if they're identical" (i.e. dedupe), and
"classic copy via pagecache". I lean towards giving each variant its own bit
and only allowing through the cases that make sense, rather than giving each
valid combination its own unique number, but maybe I misinterpreted the intent
behind the code.
(I guess there could also be 'do it with hardware assist' but that's digging
myself a pretty deep hole.)
--D
> return -EINVAL;
>
> /* copy_file_range allows full ssize_t len, ignoring MAX_RW_COUNT */
> @@ -1355,7 +1356,7 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> if (!(file_in->f_mode & FMODE_READ) ||
> !(file_out->f_mode & FMODE_WRITE) ||
> (file_out->f_flags & O_APPEND) ||
> - !file_out->f_op || !file_out->f_op->copy_file_range)
> + !file_in->f_op)
> return -EBADF;
>
> inode_in = file_inode(file_in);
> @@ -1377,8 +1378,15 @@ ssize_t vfs_copy_file_range(struct file *file_in, loff_t pos_in,
> if (ret)
> return ret;
>
> - ret = file_out->f_op->copy_file_range(file_in, pos_in, file_out, pos_out,
> - len, flags);
> + ret = -EOPNOTSUPP;
> + if (file_out->f_op->copy_file_range)
> + ret = file_out->f_op->copy_file_range(file_in, pos_in, file_out,
> + pos_out, len, flags);
> + if ((ret < 0) && !(flags & COPY_REFLINK)) {
> + file_start_write(file_out);
> + ret = do_splice_direct(file_in, &pos_in, file_out, &pos_out, len, 0);
> + file_end_write(file_out);
> + }
> if (ret > 0) {
> fsnotify_access(file_in);
> add_rchar(current, ret);
> diff --git a/include/linux/copy.h b/include/linux/copy.h
> new file mode 100644
> index 0000000..fd54543
> --- /dev/null
> +++ b/include/linux/copy.h
> @@ -0,0 +1,6 @@
> +#ifndef _LINUX_COPY_H
> +#define _LINUX_COPY_H
> +
> +#include <uapi/linux/copy.h>
> +
> +#endif /* _LINUX_COPY_H */
> diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
> index 1ff9942..b6109f3 100644
> --- a/include/uapi/linux/Kbuild
> +++ b/include/uapi/linux/Kbuild
> @@ -90,6 +90,7 @@ header-y += coda_psdev.h
> header-y += coff.h
> header-y += connector.h
> header-y += const.h
> +header-y += copy.h
> header-y += cramfs_fs.h
> header-y += cuda.h
> header-y += cyclades.h
> diff --git a/include/uapi/linux/copy.h b/include/uapi/linux/copy.h
> new file mode 100644
> index 0000000..68f3d65
> --- /dev/null
> +++ b/include/uapi/linux/copy.h
> @@ -0,0 +1,6 @@
> +#ifndef _UAPI_LINUX_COPY_H
> +#define _UAPI_LINUX_COPY_H
> +
> +#define COPY_REFLINK 1 /* only perform a reflink */
> +
> +#endif /* _UAPI_LINUX_COPY_H */
> --
> 2.5.1
>
^ permalink raw reply
* Re: [PATCH v1 9/8] copy_file_range.2: New page documenting copy_file_range()
From: Darrick J. Wong @ 2015-09-04 21:38 UTC (permalink / raw)
To: Anna Schumaker
Cc: linux-nfs, linux-btrfs, linux-fsdevel, linux-api, zab, viro, clm,
mtk.manpages, andros, hch
In-Reply-To: <1441397823-1203-10-git-send-email-Anna.Schumaker@Netapp.com>
On Fri, Sep 04, 2015 at 04:17:03PM -0400, Anna Schumaker wrote:
> copy_file_range() is a new system call for copying ranges of data
> completely in the kernel. This gives filesystems an opportunity to
> implement some kind of "copy acceleration", such as reflinks or
> server-side-copy (in the case of NFS).
>
> Signed-off-by: Anna Schumaker <Anna.Schumaker@Netapp.com>
> ---
> man2/copy_file_range.2 | 168 +++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 168 insertions(+)
> create mode 100644 man2/copy_file_range.2
>
> diff --git a/man2/copy_file_range.2 b/man2/copy_file_range.2
> new file mode 100644
> index 0000000..4a4cb73
> --- /dev/null
> +++ b/man2/copy_file_range.2
> @@ -0,0 +1,168 @@
> +.\"This manpage is Copyright (C) 2015 Anna Schumaker <Anna.Schumaker@Netapp.com>
> +.TH COPY 2 2015-8-31 "Linux" "Linux Programmer's Manual"
> +.SH NAME
> +copy_file_range \- Copy a range of data from one file to another
> +.SH SYNOPSIS
> +.nf
> +.B #include <linux/copy.h>
> +.B #include <sys/syscall.h>
> +.B #include <unistd.h>
> +
> +.BI "ssize_t syscall(__NR_copy_file_range, int " fd_in ", loff_t * " off_in ",
> +.BI " int " fd_out ", loff_t * " off_out ", size_t " len ",
> +.BI " unsigned int " flags );
> +.fi
> +.SH DESCRIPTION
> +The
> +.BR copy_file_range ()
> +system call performs an in-kernel copy between two file descriptors
> +without all that tedious mucking about in userspace.
;)
> +It copies up to
> +.I len
> +bytes of data from file descriptor
> +.I fd_in
> +to file descriptor
> +.I fd_out
> +at
> +.IR off_out .
> +The file descriptors must not refer to the same file.
Why? btrfs (and XFS) reflink can handle the case of a file sharing blocks
with itself.
> +
> +The following semantics apply for
> +.IR fd_in ,
> +and similar statements apply to
> +.IR off_out :
> +.IP * 3
> +If
> +.I off_in
> +is NULL, then bytes are read from
> +.I fd_in
> +starting from the current file offset and the current
> +file offset is adjusted appropriately.
> +.IP *
> +If
> +.I off_in
> +is not NULL, then
> +.I off_in
> +must point to a buffer that specifies the starting
> +offset where bytes from
> +.I fd_in
> +will be read. The current file offset of
> +.I fd_in
> +is not changed, but
> +.I off_in
> +is adjusted appropriately.
> +.PP
> +The default behavior of
> +.BR copy_file_range ()
> +is filesystem specific, and might result in creating a
> +copy-on-write reflink.
> +In the event that a given filesystem does not implement
> +any form of copy acceleration, the kernel will perform
> +a deep copy of the requested range by reading bytes from
I wonder if it's wise to allow deep copies -- what happens if len == 1T?
Will this syscall just block for a really long time?
> +.I fd_in
> +and writing them to
> +.IR fd_out .
"...if COPY_REFLINK is not set in flags."
> +
> +Currently, Linux only supports the following flag:
> +.TP 1.9i
> +.B COPY_REFLINK
> +Only perform the copy if the filesystem can do it as a reflink.
> +Do not fall back on performing a deep copy.
> +.SH RETURN VALUE
> +Upon successful completion,
> +.BR copy_file_range ()
> +will return the number of bytes copied between files.
> +This could be less than the length originally requested.
> +
> +On error,
> +.BR copy_file_range ()
> +returns \-1 and
> +.I errno
> +is set to indicate the error.
> +.SH ERRORS
> +.TP
> +.B EBADF
> +One or more file descriptors are not valid,
> +or do not have proper read-write mode.
"or fd_out is not opened for writing"?
> +.TP
> +.B EINVAL
> +Requested range extends beyond the end of the file;
> +.I flags
> +argument is set to an invalid value.
> +.TP
> +.B EOPNOTSUPP
> +.B COPY_REFLINK
> +was specified in
> +.IR flags ,
> +but the target filesystem does not support reflinks.
> +.TP
> +.B EXDEV
> +Target filesystem doesn't support cross-filesystem copies.
> +.SH VERSIONS
Perhaps this ought to list a few more errors (EIO, ENOSPC, ENOSYS, EPERM...)
that can be returned? (I was looking at the fallocate manpage.)
--D
> +The
> +.BR copy_file_range ()
> +system call first appeared in Linux 4.3.
> +.SH CONFORMING TO
> +The
> +.BR copy_file_range ()
> +system call is a nonstandard Linux extension.
> +.SH EXAMPLE
> +.nf
> +
> +#define _GNU_SOURCE
> +#include <fcntl.h>
> +#include <linux/copy.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <sys/stat.h>
> +#include <sys/syscall.h>
> +#include <unistd.h>
> +
> +
> +int main(int argc, char **argv)
> +{
> + int fd_in, fd_out;
> + struct stat stat;
> + loff_t len, ret;
> +
> + if (argc != 3) {
> + fprintf(stderr, "Usage: %s <pathname> <pathname>\n", argv[0]);
> + exit(EXIT_FAILURE);
> + }
> +
> + fd_in = open(argv[1], O_RDONLY);
> + if (fd_in == -1) {
> + perror("open (argv[1])");
> + exit(EXIT_FAILURE);
> + }
> +
> + if (fstat(fd_in, &stat) == -1) {
> + perror("fstat");
> + exit(EXIT_FAILURE);
> + }
> + len = stat.st_size;
> +
> + fd_out = open(argv[2], O_WRONLY | O_CREAT, 0644);
> + if (fd_out == -1) {
> + perror("open (argv[2])");
> + exit(EXIT_FAILURE);
> + }
> +
> + do {
> + ret = syscall(__NR_copy_file_range, fd_in, NULL,
> + fd_out, NULL, len, 0);
> + if (ret == -1) {
> + perror("copy_file_range");
> + exit(EXIT_FAILURE);
> + }
> +
> + len -= ret;
> + } while (len > 0);
> +
> + close(fd_in);
> + close(fd_out);
> + exit(EXIT_SUCCESS);
> +}
> +.fi
> +.SH SEE ALSO
> +.BR splice (2)
> --
> 2.5.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox