* Re: [PATCH 01/86] pci: export pci_ids.h
From: Michael S. Tsirkin @ 2015-03-30 11:19 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Bjorn Helgaas,
Jonathan Corbet, David S. Miller, Hans Verkuil,
Mauro Carvalho Chehab, Alexei Starovoitov, stephen hemminger,
Masahiro Yamada, Andy Shevchenko, Andy Lutomirski,
Rasmus Villemoes, Stephane Eranian, Huang Rui, Peter Neubauer,
linux-pci-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150330105707.GA17979-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Mon, Mar 30, 2015 at 12:57:07PM +0200, Greg KH wrote:
> On Mon, Mar 30, 2015 at 12:46:36PM +0200, Michael S. Tsirkin wrote:
> > Sorry about keeping this thread alive, I'm just trying
> > to wrap my head around what you consider a sane API.
> >
> > Linux used not to export headers automatically, generally.
> > It used to be "just use libc". Why is this header different?
>
> You are now exporting things from the kernel that were not being
> exported in the past. When you do that, you are now guaranteeing that
> this api will be present for forever in the future.
We already guarantee this API, that's the point.
Look at this sysfs file:
/sys/bus/pci/devices/0000\:00\:00.0/class
It's a hex number. The values? They are in pci_ids.h
And so everyone who uses this file carries a copy
of the header.
> > These constants are required to use the interfaces
> > linux does export to userspace, including VFIO and sysfs.
> > No one maintains them really, though libpci has a copy
> > of these.
>
> Also libudev, which takes them from the "master" copy that libpci pulls
> from.
OK, so which is the master copy for this:
#define PCI_BASE_CLASS_NETWORK 0x02
> > Some people believe headers are copyrighteable, for them, licensing is
> > also problematic: libpci is under plain GPL, Linux needs the exception
> > for user programs. Does not this mean that we can't depend on libpci to
> > provide parts of linux/userspace interface?
>
> This makes no sense at all. If you have legal questions, ask a lawyer,
> not a programmer. Would you ask me medical questions as well?
>
> > > > Wouldn't the same thing apply to pci_regs.h?
> > > > Why does it make sense to export PCI_CLASS_REVISION
> > > > so I know where to read the class value from
> > > > configuration, but not what the values are?
> > >
> > > That's just due to history, we made the mistake to export those a long
> > > time ago, I wouldn't recommend doing any more, and again, instead rely
> > > on the programs that properly maintain that for you.
> >
> > So add libpci dependency just for the headers? No one wants to do this.
> > Case in point, Linux doesn't want to depend on libpci headers either, right?
>
> The kernel is self-contained, and has to be, don't be foolish.
I agree it is pretty contained but it's not 100%, and it does not have
to be. kernel build depends on perl, bash, make, etc etc.
We don't want a library header dependency because it's easier not to
have dependencies, not because we can't have it.
Same applies to others really.
> > I don't think class IDs or vendor IDs ever did or can change.
>
> Then you haven't tracked what has happened over time. They do change,
> they are sometimes incorrect, they are extended, and stuff is modified.
Sounds like a good reason to avoid duplication, have
provider of the interface document that constants used.
> > New stuff is added, we add it as we add Linux support.
> > Isn't this a good reason to include this? Will push
> > vendors to work with the community instead of
> > around it using userspace drivers.
>
> This has nothing to do with "pushing vendors" to do anything.
>
> Again, if you have a userspace program that needs these headers, then
> use one of the _two_ different packages that userspace already provides
> that has them, don't make the kernel become the third provider of the
> same header information, that's major duplication of effort for no
> reason at all.
>
> thanks,
>
> greg k-h
Why there much more that two packages, I can find
at least 5 copies in the wild. Why? I think it's because it's part of
linux ABI that doesn't have matching headers. People are asked to build
their own, so of course they copy each other.
Once linux exports these headers everyone can stop
duplicating each other and others, and just use linux headers too.
--
MST
^ permalink raw reply
* [PATCH v2] pci: export class IDs from pci_ids.h
From: Michael S. Tsirkin @ 2015-03-30 11:33 UTC (permalink / raw)
To: linux-kernel
Cc: Bjorn Helgaas, Jonathan Corbet, David S. Miller, Hans Verkuil,
Mauro Carvalho Chehab, Alexei Starovoitov, stephen hemminger,
Masahiro Yamada, linux-pci, linux-doc, linux-api, Greg KH
The basic class ID macros in pci_ids.h are pretty useful for userspace
using the pci sysfs interface, and they aren't fundamentally different
from the constants in pci_regs.h - both are defined in the
pci spec.
At the moment userspace is forced to duplicate these macros
(e.g. QEMU does this, so do seabios, gpxe, and others), it is better to
expose them in /usr/include/linux/pci_ids.h so everyone can just include
this header.
vendor/device IDs are left in include/linux/pci_ids.h -
this list isn't complete anyway, and there's no plan
to maintain all vendor IDs there.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Greg, I apologise if this looks like I'm still pushing
this after your objections, I just feel v1 was too far
off the mark, and so lead the discussion astray.
I hope this one is less controversial, in particular, it
only exports 137 LOC.
Thanks for your time.
include/linux/pci_ids.h | 130 +---------------------------------------
include/uapi/linux/pci_ids.h | 137 +++++++++++++++++++++++++++++++++++++++++++
Documentation/PCI/pci.txt | 2 +-
include/uapi/linux/Kbuild | 1 +
4 files changed, 141 insertions(+), 129 deletions(-)
create mode 100644 include/uapi/linux/pci_ids.h
diff --git a/include/linux/pci_ids.h b/include/linux/pci_ids.h
index e63c02a..9f39259 100644
--- a/include/linux/pci_ids.h
+++ b/include/linux/pci_ids.h
@@ -1,5 +1,5 @@
/*
- * PCI Class, Vendor and Device IDs
+ * PCI Vendor and Device IDs
*
* Please keep sorted.
*
@@ -9,133 +9,7 @@
#ifndef _LINUX_PCI_IDS_H
#define _LINUX_PCI_IDS_H
-/* Device classes and subclasses */
-
-#define PCI_CLASS_NOT_DEFINED 0x0000
-#define PCI_CLASS_NOT_DEFINED_VGA 0x0001
-
-#define PCI_BASE_CLASS_STORAGE 0x01
-#define PCI_CLASS_STORAGE_SCSI 0x0100
-#define PCI_CLASS_STORAGE_IDE 0x0101
-#define PCI_CLASS_STORAGE_FLOPPY 0x0102
-#define PCI_CLASS_STORAGE_IPI 0x0103
-#define PCI_CLASS_STORAGE_RAID 0x0104
-#define PCI_CLASS_STORAGE_SATA 0x0106
-#define PCI_CLASS_STORAGE_SATA_AHCI 0x010601
-#define PCI_CLASS_STORAGE_SAS 0x0107
-#define PCI_CLASS_STORAGE_OTHER 0x0180
-
-#define PCI_BASE_CLASS_NETWORK 0x02
-#define PCI_CLASS_NETWORK_ETHERNET 0x0200
-#define PCI_CLASS_NETWORK_TOKEN_RING 0x0201
-#define PCI_CLASS_NETWORK_FDDI 0x0202
-#define PCI_CLASS_NETWORK_ATM 0x0203
-#define PCI_CLASS_NETWORK_OTHER 0x0280
-
-#define PCI_BASE_CLASS_DISPLAY 0x03
-#define PCI_CLASS_DISPLAY_VGA 0x0300
-#define PCI_CLASS_DISPLAY_XGA 0x0301
-#define PCI_CLASS_DISPLAY_3D 0x0302
-#define PCI_CLASS_DISPLAY_OTHER 0x0380
-
-#define PCI_BASE_CLASS_MULTIMEDIA 0x04
-#define PCI_CLASS_MULTIMEDIA_VIDEO 0x0400
-#define PCI_CLASS_MULTIMEDIA_AUDIO 0x0401
-#define PCI_CLASS_MULTIMEDIA_PHONE 0x0402
-#define PCI_CLASS_MULTIMEDIA_OTHER 0x0480
-
-#define PCI_BASE_CLASS_MEMORY 0x05
-#define PCI_CLASS_MEMORY_RAM 0x0500
-#define PCI_CLASS_MEMORY_FLASH 0x0501
-#define PCI_CLASS_MEMORY_OTHER 0x0580
-
-#define PCI_BASE_CLASS_BRIDGE 0x06
-#define PCI_CLASS_BRIDGE_HOST 0x0600
-#define PCI_CLASS_BRIDGE_ISA 0x0601
-#define PCI_CLASS_BRIDGE_EISA 0x0602
-#define PCI_CLASS_BRIDGE_MC 0x0603
-#define PCI_CLASS_BRIDGE_PCI 0x0604
-#define PCI_CLASS_BRIDGE_PCMCIA 0x0605
-#define PCI_CLASS_BRIDGE_NUBUS 0x0606
-#define PCI_CLASS_BRIDGE_CARDBUS 0x0607
-#define PCI_CLASS_BRIDGE_RACEWAY 0x0608
-#define PCI_CLASS_BRIDGE_OTHER 0x0680
-
-#define PCI_BASE_CLASS_COMMUNICATION 0x07
-#define PCI_CLASS_COMMUNICATION_SERIAL 0x0700
-#define PCI_CLASS_COMMUNICATION_PARALLEL 0x0701
-#define PCI_CLASS_COMMUNICATION_MULTISERIAL 0x0702
-#define PCI_CLASS_COMMUNICATION_MODEM 0x0703
-#define PCI_CLASS_COMMUNICATION_OTHER 0x0780
-
-#define PCI_BASE_CLASS_SYSTEM 0x08
-#define PCI_CLASS_SYSTEM_PIC 0x0800
-#define PCI_CLASS_SYSTEM_PIC_IOAPIC 0x080010
-#define PCI_CLASS_SYSTEM_PIC_IOXAPIC 0x080020
-#define PCI_CLASS_SYSTEM_DMA 0x0801
-#define PCI_CLASS_SYSTEM_TIMER 0x0802
-#define PCI_CLASS_SYSTEM_RTC 0x0803
-#define PCI_CLASS_SYSTEM_PCI_HOTPLUG 0x0804
-#define PCI_CLASS_SYSTEM_SDHCI 0x0805
-#define PCI_CLASS_SYSTEM_OTHER 0x0880
-
-#define PCI_BASE_CLASS_INPUT 0x09
-#define PCI_CLASS_INPUT_KEYBOARD 0x0900
-#define PCI_CLASS_INPUT_PEN 0x0901
-#define PCI_CLASS_INPUT_MOUSE 0x0902
-#define PCI_CLASS_INPUT_SCANNER 0x0903
-#define PCI_CLASS_INPUT_GAMEPORT 0x0904
-#define PCI_CLASS_INPUT_OTHER 0x0980
-
-#define PCI_BASE_CLASS_DOCKING 0x0a
-#define PCI_CLASS_DOCKING_GENERIC 0x0a00
-#define PCI_CLASS_DOCKING_OTHER 0x0a80
-
-#define PCI_BASE_CLASS_PROCESSOR 0x0b
-#define PCI_CLASS_PROCESSOR_386 0x0b00
-#define PCI_CLASS_PROCESSOR_486 0x0b01
-#define PCI_CLASS_PROCESSOR_PENTIUM 0x0b02
-#define PCI_CLASS_PROCESSOR_ALPHA 0x0b10
-#define PCI_CLASS_PROCESSOR_POWERPC 0x0b20
-#define PCI_CLASS_PROCESSOR_MIPS 0x0b30
-#define PCI_CLASS_PROCESSOR_CO 0x0b40
-
-#define PCI_BASE_CLASS_SERIAL 0x0c
-#define PCI_CLASS_SERIAL_FIREWIRE 0x0c00
-#define PCI_CLASS_SERIAL_FIREWIRE_OHCI 0x0c0010
-#define PCI_CLASS_SERIAL_ACCESS 0x0c01
-#define PCI_CLASS_SERIAL_SSA 0x0c02
-#define PCI_CLASS_SERIAL_USB 0x0c03
-#define PCI_CLASS_SERIAL_USB_UHCI 0x0c0300
-#define PCI_CLASS_SERIAL_USB_OHCI 0x0c0310
-#define PCI_CLASS_SERIAL_USB_EHCI 0x0c0320
-#define PCI_CLASS_SERIAL_USB_XHCI 0x0c0330
-#define PCI_CLASS_SERIAL_FIBER 0x0c04
-#define PCI_CLASS_SERIAL_SMBUS 0x0c05
-
-#define PCI_BASE_CLASS_WIRELESS 0x0d
-#define PCI_CLASS_WIRELESS_RF_CONTROLLER 0x0d10
-#define PCI_CLASS_WIRELESS_WHCI 0x0d1010
-
-#define PCI_BASE_CLASS_INTELLIGENT 0x0e
-#define PCI_CLASS_INTELLIGENT_I2O 0x0e00
-
-#define PCI_BASE_CLASS_SATELLITE 0x0f
-#define PCI_CLASS_SATELLITE_TV 0x0f00
-#define PCI_CLASS_SATELLITE_AUDIO 0x0f01
-#define PCI_CLASS_SATELLITE_VOICE 0x0f03
-#define PCI_CLASS_SATELLITE_DATA 0x0f04
-
-#define PCI_BASE_CLASS_CRYPT 0x10
-#define PCI_CLASS_CRYPT_NETWORK 0x1000
-#define PCI_CLASS_CRYPT_ENTERTAINMENT 0x1001
-#define PCI_CLASS_CRYPT_OTHER 0x1080
-
-#define PCI_BASE_CLASS_SIGNAL_PROCESSING 0x11
-#define PCI_CLASS_SP_DPIO 0x1100
-#define PCI_CLASS_SP_OTHER 0x1180
-
-#define PCI_CLASS_OTHERS 0xff
+#include <uapi/linux/pci_ids.h>
/* Vendors and devices. Sort key: vendor first, device next. */
diff --git a/include/uapi/linux/pci_ids.h b/include/uapi/linux/pci_ids.h
new file mode 100644
index 0000000..45d3f64
--- /dev/null
+++ b/include/uapi/linux/pci_ids.h
@@ -0,0 +1,137 @@
+/*
+ * PCI Class IDs.
+ *
+ * Please keep sorted.
+ */
+#ifndef _UAPI_LINUX_PCI_IDS_H
+#define _UAPI_LINUX_PCI_IDS_H
+
+/* Device classes and subclasses */
+
+#define PCI_CLASS_NOT_DEFINED 0x0000
+#define PCI_CLASS_NOT_DEFINED_VGA 0x0001
+
+#define PCI_BASE_CLASS_STORAGE 0x01
+#define PCI_CLASS_STORAGE_SCSI 0x0100
+#define PCI_CLASS_STORAGE_IDE 0x0101
+#define PCI_CLASS_STORAGE_FLOPPY 0x0102
+#define PCI_CLASS_STORAGE_IPI 0x0103
+#define PCI_CLASS_STORAGE_RAID 0x0104
+#define PCI_CLASS_STORAGE_SATA 0x0106
+#define PCI_CLASS_STORAGE_SATA_AHCI 0x010601
+#define PCI_CLASS_STORAGE_SAS 0x0107
+#define PCI_CLASS_STORAGE_OTHER 0x0180
+
+#define PCI_BASE_CLASS_NETWORK 0x02
+#define PCI_CLASS_NETWORK_ETHERNET 0x0200
+#define PCI_CLASS_NETWORK_TOKEN_RING 0x0201
+#define PCI_CLASS_NETWORK_FDDI 0x0202
+#define PCI_CLASS_NETWORK_ATM 0x0203
+#define PCI_CLASS_NETWORK_OTHER 0x0280
+
+#define PCI_BASE_CLASS_DISPLAY 0x03
+#define PCI_CLASS_DISPLAY_VGA 0x0300
+#define PCI_CLASS_DISPLAY_XGA 0x0301
+#define PCI_CLASS_DISPLAY_3D 0x0302
+#define PCI_CLASS_DISPLAY_OTHER 0x0380
+
+#define PCI_BASE_CLASS_MULTIMEDIA 0x04
+#define PCI_CLASS_MULTIMEDIA_VIDEO 0x0400
+#define PCI_CLASS_MULTIMEDIA_AUDIO 0x0401
+#define PCI_CLASS_MULTIMEDIA_PHONE 0x0402
+#define PCI_CLASS_MULTIMEDIA_OTHER 0x0480
+
+#define PCI_BASE_CLASS_MEMORY 0x05
+#define PCI_CLASS_MEMORY_RAM 0x0500
+#define PCI_CLASS_MEMORY_FLASH 0x0501
+#define PCI_CLASS_MEMORY_OTHER 0x0580
+
+#define PCI_BASE_CLASS_BRIDGE 0x06
+#define PCI_CLASS_BRIDGE_HOST 0x0600
+#define PCI_CLASS_BRIDGE_ISA 0x0601
+#define PCI_CLASS_BRIDGE_EISA 0x0602
+#define PCI_CLASS_BRIDGE_MC 0x0603
+#define PCI_CLASS_BRIDGE_PCI 0x0604
+#define PCI_CLASS_BRIDGE_PCMCIA 0x0605
+#define PCI_CLASS_BRIDGE_NUBUS 0x0606
+#define PCI_CLASS_BRIDGE_CARDBUS 0x0607
+#define PCI_CLASS_BRIDGE_RACEWAY 0x0608
+#define PCI_CLASS_BRIDGE_OTHER 0x0680
+
+#define PCI_BASE_CLASS_COMMUNICATION 0x07
+#define PCI_CLASS_COMMUNICATION_SERIAL 0x0700
+#define PCI_CLASS_COMMUNICATION_PARALLEL 0x0701
+#define PCI_CLASS_COMMUNICATION_MULTISERIAL 0x0702
+#define PCI_CLASS_COMMUNICATION_MODEM 0x0703
+#define PCI_CLASS_COMMUNICATION_OTHER 0x0780
+
+#define PCI_BASE_CLASS_SYSTEM 0x08
+#define PCI_CLASS_SYSTEM_PIC 0x0800
+#define PCI_CLASS_SYSTEM_PIC_IOAPIC 0x080010
+#define PCI_CLASS_SYSTEM_PIC_IOXAPIC 0x080020
+#define PCI_CLASS_SYSTEM_DMA 0x0801
+#define PCI_CLASS_SYSTEM_TIMER 0x0802
+#define PCI_CLASS_SYSTEM_RTC 0x0803
+#define PCI_CLASS_SYSTEM_PCI_HOTPLUG 0x0804
+#define PCI_CLASS_SYSTEM_SDHCI 0x0805
+#define PCI_CLASS_SYSTEM_OTHER 0x0880
+
+#define PCI_BASE_CLASS_INPUT 0x09
+#define PCI_CLASS_INPUT_KEYBOARD 0x0900
+#define PCI_CLASS_INPUT_PEN 0x0901
+#define PCI_CLASS_INPUT_MOUSE 0x0902
+#define PCI_CLASS_INPUT_SCANNER 0x0903
+#define PCI_CLASS_INPUT_GAMEPORT 0x0904
+#define PCI_CLASS_INPUT_OTHER 0x0980
+
+#define PCI_BASE_CLASS_DOCKING 0x0a
+#define PCI_CLASS_DOCKING_GENERIC 0x0a00
+#define PCI_CLASS_DOCKING_OTHER 0x0a80
+
+#define PCI_BASE_CLASS_PROCESSOR 0x0b
+#define PCI_CLASS_PROCESSOR_386 0x0b00
+#define PCI_CLASS_PROCESSOR_486 0x0b01
+#define PCI_CLASS_PROCESSOR_PENTIUM 0x0b02
+#define PCI_CLASS_PROCESSOR_ALPHA 0x0b10
+#define PCI_CLASS_PROCESSOR_POWERPC 0x0b20
+#define PCI_CLASS_PROCESSOR_MIPS 0x0b30
+#define PCI_CLASS_PROCESSOR_CO 0x0b40
+
+#define PCI_BASE_CLASS_SERIAL 0x0c
+#define PCI_CLASS_SERIAL_FIREWIRE 0x0c00
+#define PCI_CLASS_SERIAL_FIREWIRE_OHCI 0x0c0010
+#define PCI_CLASS_SERIAL_ACCESS 0x0c01
+#define PCI_CLASS_SERIAL_SSA 0x0c02
+#define PCI_CLASS_SERIAL_USB 0x0c03
+#define PCI_CLASS_SERIAL_USB_UHCI 0x0c0300
+#define PCI_CLASS_SERIAL_USB_OHCI 0x0c0310
+#define PCI_CLASS_SERIAL_USB_EHCI 0x0c0320
+#define PCI_CLASS_SERIAL_USB_XHCI 0x0c0330
+#define PCI_CLASS_SERIAL_FIBER 0x0c04
+#define PCI_CLASS_SERIAL_SMBUS 0x0c05
+
+#define PCI_BASE_CLASS_WIRELESS 0x0d
+#define PCI_CLASS_WIRELESS_RF_CONTROLLER 0x0d10
+#define PCI_CLASS_WIRELESS_WHCI 0x0d1010
+
+#define PCI_BASE_CLASS_INTELLIGENT 0x0e
+#define PCI_CLASS_INTELLIGENT_I2O 0x0e00
+
+#define PCI_BASE_CLASS_SATELLITE 0x0f
+#define PCI_CLASS_SATELLITE_TV 0x0f00
+#define PCI_CLASS_SATELLITE_AUDIO 0x0f01
+#define PCI_CLASS_SATELLITE_VOICE 0x0f03
+#define PCI_CLASS_SATELLITE_DATA 0x0f04
+
+#define PCI_BASE_CLASS_CRYPT 0x10
+#define PCI_CLASS_CRYPT_NETWORK 0x1000
+#define PCI_CLASS_CRYPT_ENTERTAINMENT 0x1001
+#define PCI_CLASS_CRYPT_OTHER 0x1080
+
+#define PCI_BASE_CLASS_SIGNAL_PROCESSING 0x11
+#define PCI_CLASS_SP_DPIO 0x1100
+#define PCI_CLASS_SP_OTHER 0x1180
+
+#define PCI_CLASS_OTHERS 0xff
+
+#endif /* _LINUX_PCI_IDS_H */
diff --git a/Documentation/PCI/pci.txt b/Documentation/PCI/pci.txt
index 9518006..53da450 100644
--- a/Documentation/PCI/pci.txt
+++ b/Documentation/PCI/pci.txt
@@ -135,7 +135,7 @@ Each entry consists of:
class Device class, subclass, and "interface" to match.
See Appendix D of the PCI Local Bus Spec or
- include/linux/pci_ids.h for a full list of classes.
+ include/uapi/linux/pci_ids.h for a full list of classes.
Most drivers do not need to specify class/class_mask
as vendor/device is normally sufficient.
diff --git a/include/uapi/linux/Kbuild b/include/uapi/linux/Kbuild
index 68ceb97..1e16ec4 100644
--- a/include/uapi/linux/Kbuild
+++ b/include/uapi/linux/Kbuild
@@ -310,6 +310,7 @@ header-y += param.h
header-y += parport.h
header-y += patchkey.h
header-y += pci.h
+header-y += pci_ids.h
header-y += pci_regs.h
header-y += perf_event.h
header-y += personality.h
--
MST
^ permalink raw reply related
* Re: [PATCH 01/86] pci: export pci_ids.h
From: Greg KH @ 2015-03-30 11:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Bjorn Helgaas,
Jonathan Corbet, David S. Miller, Hans Verkuil,
Mauro Carvalho Chehab, Alexei Starovoitov, stephen hemminger,
Masahiro Yamada, Andy Shevchenko, Andy Lutomirski,
Rasmus Villemoes, Stephane Eranian, Huang Rui, Peter Neubauer,
linux-pci-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150330130020-mutt-send-email-mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Mon, Mar 30, 2015 at 01:19:14PM +0200, Michael S. Tsirkin wrote:
> Why there much more that two packages, I can find
> at least 5 copies in the wild. Why? I think it's because it's part of
> linux ABI that doesn't have matching headers. People are asked to build
> their own, so of course they copy each other.
>
> Once linux exports these headers everyone can stop
> duplicating each other and others, and just use linux headers too.
As I'm not the PCI maintainer anymore, I'll let Bjorn decide about this.
But, your original patch series isn't ok, we don't want to export the
pci vendor/device ids to userspace, that isn't the job of the kernel to
do so. If we should be exporting the pci class and config structures or
not is Bjorn's decision.
greg k-h
^ permalink raw reply
* Re: [PATCH 01/86] pci: export pci_ids.h
From: Michael S. Tsirkin @ 2015-03-30 11:41 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA, Bjorn Helgaas,
Jonathan Corbet, David S. Miller, Hans Verkuil,
Mauro Carvalho Chehab, Alexei Starovoitov, stephen hemminger,
Masahiro Yamada, Andy Shevchenko, Andy Lutomirski,
Rasmus Villemoes, Stephane Eranian, Huang Rui, Peter Neubauer,
linux-pci-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150330113516.GA19501-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
On Mon, Mar 30, 2015 at 01:35:16PM +0200, Greg KH wrote:
> On Mon, Mar 30, 2015 at 01:19:14PM +0200, Michael S. Tsirkin wrote:
> > Why there much more that two packages, I can find
> > at least 5 copies in the wild. Why? I think it's because it's part of
> > linux ABI that doesn't have matching headers. People are asked to build
> > their own, so of course they copy each other.
> >
> > Once linux exports these headers everyone can stop
> > duplicating each other and others, and just use linux headers too.
>
> As I'm not the PCI maintainer anymore, I'll let Bjorn decide about this.
>
> But, your original patch series isn't ok, we don't want to export the
> pci vendor/device ids to userspace, that isn't the job of the kernel to
> do so. If we should be exporting the pci class and config structures or
> not is Bjorn's decision.
>
> greg k-h
I sent v2 doing exactly that.
Thanks a lot for the review.
--
MST
^ permalink raw reply
* Re: [PATCH] Add virtio gpu driver.
From: Gerd Hoffmann @ 2015-03-30 12:23 UTC (permalink / raw)
To: Daniel Vetter
Cc: virtio-dev, Michael S. Tsirkin, open list:ABI/API, Rusty Russell,
open list, open list:DRM DRIVERS, open list:VIRTIO CORE, NET...,
Dave Airlie
In-Reply-To: <20150324165057.GN1349@phenom.ffwll.local>
Hi,
> > Signed-off-by: Dave Airlie <airlied@redhat.com>
> > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
>
> Standard request from my side for new drm drivers (especially if they're
> this simple): Can you please update the drivers to latest drm internal
> interfaces, i.e. using universal planes and atomic?
Up'n'running. Incremental patch:
https://www.kraxel.org/cgit/linux/commit/?h=virtio-gpu-2d&id=b8edf4f38a1ec5a50f6ac8948521a12f862d3d5a
v2 coming, but I'll go over the other reviews first.
cheers,
Gerd
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [RFC] capabilities: Ambient capabilities
From: Christoph Lameter @ 2015-03-30 12:55 UTC (permalink / raw)
To: Andrew G. Morgan
Cc: Andy Lutomirski, Jarkko Sakkinen, Ted Ts'o, Andrew Lutomirski,
Andrew Morton, Michael Kerrisk, Mimi Zohar, Linux API,
Austin S Hemmelgarn, linux-security-module, Aaron Jones, LKML,
Serge Hallyn, Markku Savela, Kees Cook, Jonathan Corbet
In-Reply-To: <CALQRfL7AyadFcu9JqtC6FGnuuqY7zbnFAL81XnwBvn0o6c6UWQ@mail.gmail.com>
On Sat, 14 Mar 2015, Andrew G. Morgan wrote:
>
> I thought I did. Please implement a lockable secure bit and I will
Would this suffice? It puts the CAP_SETPCAP limitation back to how it
was in my earlier patch.
Subject: ambient caps: Allow disabling with SETPCAP
Do not allow setting ambient caps if CAP_SETPCAP is not set.
Signed-off-by: Christoph Lameter <cl@linux.com>
Index: linux/security/commoncap.c
===================================================================
--- linux.orig/security/commoncap.c
+++ linux/security/commoncap.c
@@ -962,6 +962,9 @@ int cap_task_prctl(int option, unsigned
if (((!cap_valid(arg3)) | arg4 | arg5))
return -EINVAL;
+ if (!ns_capable(current_user_ns(), CAP_SETPCAP))
+ return -EPERM;
+
if (arg2 == PR_CAP_AMBIENT_GET) {
return !!cap_raised(current_cred()->cap_ambient, arg3);
} else if (arg2 != PR_CAP_AMBIENT_RAISE &&
^ permalink raw reply
* Re: [PATCH] stm class: Adding master and channel parameter to ioctl function
From: Alexander Shishkin @ 2015-03-30 14:19 UTC (permalink / raw)
Cc: gregkh, linux-kernel, peter.lachner, norbert.schulz, keven.boell,
yann.fouassier, laurent.fert, linux-api, kaixu.xia, zhang.chunyan,
mathieu.poirier
In-Reply-To: <1427406371-6699-1-git-send-email-mathieu.poirier@linaro.org>
Mathieu Poirier <mathieu.poirier@linaro.org> writes:
> Hey Alex,
Hi Mathieu,
> Have a look at the following patch and see if you agree with my approach. If so
> simply add the code to a third version.
Greg's comment about the two levels of ioctls got me thinking in the
direction of adding callbacks to stm_data on case-by-case basis and then
I realized that we might actually move these private callbacks into the
stm core as well (see my followup to the coresight-stm driver patch).
So, I suggest that we work through the ioctl commands that you need for
coresight stm and try to see how they fit into the generic scheme of
things and if we still find that we need implementation-specific ioctl
commands, then we try to shape them as individual callbacks rather than
just .ioctl().
> diff --git a/include/linux/stm.h b/include/linux/stm.h
> index 976c94d8f17f..84dd83c47fe7 100644
> --- a/include/linux/stm.h
> +++ b/include/linux/stm.h
> @@ -62,8 +62,9 @@ struct stm_data {
> unsigned int);
> void (*unlink)(struct stm_data *, unsigned int,
> unsigned int);
> - long (*ioctl)(struct stm_data *, unsigned int,
> - unsigned long);
> + long (*ioctl)(struct stm_data *,
> + unsigned int, unsigned int,
> + unsigned int, unsigned long);
We might go even further and pass a struct stm_output (which also
contains number of channels) pointer here, having first moved it from
drivers/... to include/linux/stm.h, and while at it also do the same to
the other callbacks that pass master/channel around. Does this make
sense to you?
Regards,
--
Alex
^ permalink raw reply
* Re: [patch 1/2] mm, doc: cleanup and clarify munmap behavior for hugetlb memory
From: Eric B Munson @ 2015-03-30 14:23 UTC (permalink / raw)
To: Hugh Dickins
Cc: David Rientjes, Andrew Morton, Jonathan Corbet, Davide Libenzi,
Luiz Capitulino, Shuah Khan, Andrea Arcangeli, Joern Engel,
Jianguo Wu, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <alpine.LSU.2.11.1503291801400.1052-fupSdm12i1nKWymIFiNcPA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1918 bytes --]
On Sun, 29 Mar 2015, Hugh Dickins wrote:
> On Thu, 26 Mar 2015, David Rientjes wrote:
>
> > munmap(2) of hugetlb memory requires a length that is hugepage aligned,
> > otherwise it may fail. Add this to the documentation.
>
> Thanks for taking this on, David. But although munmap(2) is the one
> Davide called out, it goes beyond that, doesn't it? To mprotect and
> madvise and ...
>
> I don't want to work out the list myself: is_vm_hugetlb_page() is
> special-cased all over, and different syscalls react differently.
>
> Which is another reason why, like you, I much prefer not to interfere
> with the long established behavior: it would be very easy to introduce
> bugs and worse inconsistencies.
>
> And mprotect(2) is a good example of why we should not mess around
> with the long established API here: changing an mprotect from failing
> on a particular size to acting on a larger size is not a safe change.
>
> Eric, I apologize for bringing you in to the discussion, and then
> ignoring your input. I understand that you would like MAP_HUGETLB
> to behave more understandably. We can all agree that the existing
> behavior is unsatisfying. But it's many years too late now to
> change it around - and I suspect that a full exercise to do so would
> actually discover some good reasons why the original choices were made.
No worries, my main concern was avoiding the confusion that led me down
the rabbit hole of compaction and mlock. As long as the documentation,
man pages, and the code all agree I am satisfied. I would have
preferred to make the code match the docs, but I understand that
changing the code now introduces a risk of breaking userspace.
It is charitable of you to assume that there were good reasons for the
original decision. But as the author of the code in question, I suspect
the omission was one of my own inexperience.
Eric
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [RFC] capabilities: Ambient capabilities
From: Andy Lutomirski @ 2015-03-30 14:31 UTC (permalink / raw)
To: Christoph Lameter
Cc: Jarkko Sakkinen, Andrew Lutomirski, Ted Ts'o, Andrew Morton,
Andrew G. Morgan, Linux API, Mimi Zohar, Michael Kerrisk,
Austin S Hemmelgarn, linux-security-module, Aaron Jones,
Serge Hallyn, LKML, Markku Savela, Kees Cook, Jonathan Corbet
In-Reply-To: <alpine.DEB.2.11.1503300754070.5031-gkYfJU5Cukgdnm+yROfE0A@public.gmane.org>
On Mar 30, 2015 7:55 AM, "Christoph Lameter" <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org> wrote:
>
> On Sat, 14 Mar 2015, Andrew G. Morgan wrote:
>
> >
> > I thought I did. Please implement a lockable secure bit and I will
>
> Would this suffice? It puts the CAP_SETPCAP limitation back to how it
> was in my earlier patch.
>
I really don't like that variant. CAP_SETPCAP is dangerous and so
absurdly powerful that people really shouldn't hand it out.
I'll submit a new version this week with the securebits. Sorry for the delay.
--Andy
>
>
> Subject: ambient caps: Allow disabling with SETPCAP
>
> Do not allow setting ambient caps if CAP_SETPCAP is not set.
>
> Signed-off-by: Christoph Lameter <cl-vYTEC60ixJUAvxtiuMwx3w@public.gmane.org>
>
> Index: linux/security/commoncap.c
> ===================================================================
> --- linux.orig/security/commoncap.c
> +++ linux/security/commoncap.c
> @@ -962,6 +962,9 @@ int cap_task_prctl(int option, unsigned
> if (((!cap_valid(arg3)) | arg4 | arg5))
> return -EINVAL;
>
> + if (!ns_capable(current_user_ns(), CAP_SETPCAP))
> + return -EPERM;
> +
> if (arg2 == PR_CAP_AMBIENT_GET) {
> return !!cap_raised(current_cred()->cap_ambient, arg3);
> } else if (arg2 != PR_CAP_AMBIENT_RAISE &&
^ permalink raw reply
* Re: [PATCH] stm class: Adding master and channel parameter to ioctl function
From: Mathieu Poirier @ 2015-03-30 14:33 UTC (permalink / raw)
To: Alexander Shishkin
Cc: Greg KH, linux-kernel@vger.kernel.org, peter.lachner,
norbert.schulz, keven.boell, yann.fouassier, laurent.fert,
linux-api, Kaixu Xia, Chunyan Zhang
In-Reply-To: <878ueed0u3.fsf@ashishki-desk.ger.corp.intel.com>
On 30 March 2015 at 08:19, Alexander Shishkin
<alexander.shishkin@linux.intel.com> wrote:
> Mathieu Poirier <mathieu.poirier@linaro.org> writes:
>
>> Hey Alex,
>
> Hi Mathieu,
>
>> Have a look at the following patch and see if you agree with my approach. If so
>> simply add the code to a third version.
>
> Greg's comment about the two levels of ioctls got me thinking in the
> direction of adding callbacks to stm_data on case-by-case basis and then
> I realized that we might actually move these private callbacks into the
> stm core as well (see my followup to the coresight-stm driver patch).
>
> So, I suggest that we work through the ioctl commands that you need for
> coresight stm and try to see how they fit into the generic scheme of
> things and if we still find that we need implementation-specific ioctl
> commands, then we try to shape them as individual callbacks rather than
> just .ioctl().
Yeah, I'm pretty sure we can come up with something better here.
>
>> diff --git a/include/linux/stm.h b/include/linux/stm.h
>> index 976c94d8f17f..84dd83c47fe7 100644
>> --- a/include/linux/stm.h
>> +++ b/include/linux/stm.h
>> @@ -62,8 +62,9 @@ struct stm_data {
>> unsigned int);
>> void (*unlink)(struct stm_data *, unsigned int,
>> unsigned int);
>> - long (*ioctl)(struct stm_data *, unsigned int,
>> - unsigned long);
>> + long (*ioctl)(struct stm_data *,
>> + unsigned int, unsigned int,
>> + unsigned int, unsigned long);
>
> We might go even further and pass a struct stm_output (which also
> contains number of channels) pointer here, having first moved it from
> drivers/... to include/linux/stm.h, and while at it also do the same to
> the other callbacks that pass master/channel around. Does this make
> sense to you?
Humm... Do callbacks really need to have access to a struct output?
>From my side of the fence dealing with individual master/channels is
sufficient but other people may have different scenarios. The end
result is the same and modifying the callback signature doesn't
represent a lot of work.
>
> Regards,
> --
> Alex
^ permalink raw reply
* Re: [PATCH] Add virtio gpu driver.
From: Daniel Vetter @ 2015-03-30 14:49 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Daniel Vetter, virtio-dev, Michael S. Tsirkin, open list:ABI/API,
Rusty Russell, open list, open list:DRM DRIVERS,
open list:VIRTIO CORE, NET..., Dave Airlie
In-Reply-To: <1427718227.3372.33.camel@nilsson.home.kraxel.org>
On Mon, Mar 30, 2015 at 02:23:47PM +0200, Gerd Hoffmann wrote:
> Hi,
>
> > > Signed-off-by: Dave Airlie <airlied@redhat.com>
> > > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> >
> > Standard request from my side for new drm drivers (especially if they're
> > this simple): Can you please update the drivers to latest drm internal
> > interfaces, i.e. using universal planes and atomic?
>
> Up'n'running. Incremental patch:
>
> https://www.kraxel.org/cgit/linux/commit/?h=virtio-gpu-2d&id=b8edf4f38a1ec5a50f6ac8948521a12f862d3d5a
>
> v2 coming, but I'll go over the other reviews first.
Looking good. Wrt pageflip the current MO is to handroll it in your
driver, common approach is to use the msm async commit implementation
msm_atomic_commit. The issue is simply that right now there's still no
useable generic vblank callback support (drm_irq.c is a mess) hence why
the core helpers don't support async flips yet.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [RFC] capabilities: Ambient capabilities
From: Christoph Lameter @ 2015-03-30 15:05 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Jarkko Sakkinen, Andrew Lutomirski, Ted Ts'o, Andrew Morton,
Andrew G. Morgan, Linux API, Mimi Zohar, Michael Kerrisk,
Austin S Hemmelgarn, linux-security-module, Aaron Jones,
Serge Hallyn, LKML, Markku Savela, Kees Cook, Jonathan Corbet
In-Reply-To: <CALCETrW9ckw=7-1JSyFkenhFu2_1MvVqjA+inOmsuuWemGaU0w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Mon, 30 Mar 2015, Andy Lutomirski wrote:
> > Would this suffice? It puts the CAP_SETPCAP limitation back to how it
> > was in my earlier patch.
> I really don't like that variant. CAP_SETPCAP is dangerous and so
> absurdly powerful that people really shouldn't hand it out.
According to
man 7 capabilities
CAP_SETPCAP is required to setup securebits.
This hides the functionality behind yet another stage of security and
obscures this ability somewhat more?
^ permalink raw reply
* Re: [PATCH 5/5] kselftest: Add exit code defines
From: Darren Hart @ 2015-03-30 16:26 UTC (permalink / raw)
To: Michael Ellerman
Cc: Linux Kernel Mailing List, Shuah Khan,
linux-api-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar, Peter Zijlstra,
Thomas Gleixner, Davidlohr Bueso, KOSAKI Motohiro
In-Reply-To: <1427672690.4218.1.camel-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org>
On 3/29/15, 4:44 PM, "Michael Ellerman" <mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org> wrote:
>On Fri, 2015-03-27 at 16:09 -0700, Darren Hart wrote:
>>
>> On 3/27/15 3:59 PM, Michael Ellerman wrote:
>> > On Fri, 2015-03-27 at 15:17 -0700, Darren Hart wrote:
>> >> Define the exit codes with KSFT_PASS and similar so tests can use
>>these
>> >> directly if they choose. Also enable harnesses and other tooling to
>>use
>> >> the defines instead of hardcoding the return codes.
>> >
>> > +1
>> >
>> >> diff --git a/tools/testing/selftests/kselftest.h
>>b/tools/testing/selftests/kselftest.h
>> >> index 572c888..ef1c80d 100644
>> >> --- a/tools/testing/selftests/kselftest.h
>> >> +++ b/tools/testing/selftests/kselftest.h
>> >> @@ -13,6 +13,13 @@
>> >> #include <stdlib.h>
>> >> #include <unistd.h>
>> >>
>> >> +/* define kselftest exit codes */
>> >> +#define KSFT_PASS 0
>> >> +#define KSFT_FAIL 1
>> >> +#define KSFT_XFAIL 2
>> >> +#define KSFT_XPASS 3
>> >> +#define KSFT_SKIP 4
>> >> +
>> >> /* counters */
>> >> struct ksft_count {
>> >> unsigned int ksft_pass;
>> >> @@ -40,23 +47,23 @@ static inline void ksft_print_cnts(void)
>> >>
>> >> static inline int ksft_exit_pass(void)
>> >> {
>> >> - exit(0);
>> >> + exit(KSFT_PASS);
>> >> }
>> >
>> > Am I the only person who's bothered by the fact that these don't
>>actually
>> > return int?
>>
>> That bothered me to, but I couldn't be bothered to go read the manuals
>> apparently to come up with a compelling argument :-)
>
>Yeah, obviously the compiler accepts it, but it's still a bit weird.
>
>> I also think the ksft_exit* routines should go ahead and increment the
>> counters (at least optionally) so we don't have to call two functions.
>
>But the ksft_exit_*() routines exit, so there's no point incrementing the
>counters. Unless they *also* print the counters before exiting?
>
>To be honest I think we need to decide if the selftests are going to
>speak TAP
>or xUnit or whatever, and then switch to that. In their current form these
>helpers don't help much.
Fair point. This isn't a space I'm well versed in, but some standard means
of doing this would be welcome.
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply
* Re: [PATCH 5/5] kselftest: Add exit code defines
From: Peter Zijlstra @ 2015-03-30 16:30 UTC (permalink / raw)
To: Darren Hart
Cc: Michael Ellerman, Linux Kernel Mailing List, Shuah Khan,
linux-api-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar, Thomas Gleixner,
Davidlohr Bueso, KOSAKI Motohiro
In-Reply-To: <D13EC710.C43DF%dvhart-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
On Mon, Mar 30, 2015 at 09:26:07AM -0700, Darren Hart wrote:
> Fair point. This isn't a space I'm well versed in, but some standard means
> of doing this would be welcome.
I'm reading standard memes, time to get away from the keyboard for a
little while I suppose :-)
^ permalink raw reply
* Re: [PATCH 2/5] selftest/futex: Update Makefile to use lib.mk
From: Darren Hart @ 2015-03-30 16:31 UTC (permalink / raw)
To: Michael Ellerman
Cc: Linux Kernel Mailing List, Shuah Khan,
linux-api-u79uwXL29TY76Z2rM5mHXA, Ingo Molnar, Peter Zijlstra,
Thomas Gleixner, Davidlohr Bueso, KOSAKI Motohiro
In-Reply-To: <1427672978.4218.3.camel-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org>
On 3/29/15, 4:49 PM, "Michael Ellerman" <mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org> wrote:
>On Fri, 2015-03-27 at 15:17 -0700, Darren Hart wrote:
>> Adapt the futextest Makefiles to use lib.mk macros for RUN_TESTS and
>> EMIT_TESTS. For now, we reuse the run.sh mechanism provided by
>> futextest. This doesn't provide the standard selftests: [PASS|FAIL]
>> format, but the tests provide very similar output already.
>>
>> This results in the run_kselftest.sh script for futexes including a
>> single line: ./run.sh
>>
>> Cc: Shuah Khan <shuahkh-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org>
>> Cc: linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> Cc: Ingo Molnar <mingo-X9Un+BFzKDI@public.gmane.org>
>> Cc: Peter Zijlstra <peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
>> Cc: Thomas Gleixner <tglx-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
>> Cc: Davidlohr Bueso <dave-h16yJtLeMjHk1uMJSBkQmQ@public.gmane.org>
>> Cc: KOSAKI Motohiro <kosaki.motohiro-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
>> Signed-off-by: Darren Hart <dvhart-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> ---
>> tools/testing/selftests/futex/Makefile | 21
>>+++++++++++++++++++++
>> tools/testing/selftests/futex/functional/Makefile | 4 ++++
>> 2 files changed, 25 insertions(+)
>>
>> diff --git a/tools/testing/selftests/futex/Makefile
>>b/tools/testing/selftests/futex/Makefile
>> index 8629187..6a17529 100644
>> --- a/tools/testing/selftests/futex/Makefile
>> +++ b/tools/testing/selftests/futex/Makefile
>> @@ -1,8 +1,29 @@
>> SUBDIRS := functional
>>
>> +TEST_PROGS := run.sh
>> +
>> .PHONY: all clean
>
>lib.mk defines all & clean as PHONY for you.
OK, I can remove this (it was just part of the originals). Shall I resent
the whole series or just a follow-on patch?
>
>> all:
>> for DIR in $(SUBDIRS); do $(MAKE) -C $$DIR $@ ; done
>>
>> +include ../lib.mk
>> +
>> +override define RUN_TESTS
>> + ./run.sh
>> +endef
>
>Do you need to do this override? The standard logic should work AFAICS,
>or do
>you not want the echo logic?
The standard logic wants to run each individual test and uses the echo
logic. My tests have their own pass/fail reporting mechanism. As I run
many options on the same tests and have pretty-printing logic, I prefer to
keep that in a run script rather than in a makefile.
I don't care for the echo logic as a mechanism, I think the reporting
should be part of the test. It also doesn't scale past pass/fail - like
xpass and xfail, for example.
>
>> +override define INSTALL_RULE
>> + mkdir -p $(INSTALL_PATH)
>> + install -t $(INSTALL_PATH) $(TEST_PROGS) $(TEST_PROGS_EXTENDED)
>>$(TEST_FILES)
>> +
>> + @for SUBDIR in $(SUBDIRS); do \
>> + $(MAKE) -C $$SUBDIR INSTALL_PATH=$(INSTALL_PATH)/$$SUBDIR install; \
>> + done;
>> +endef
>> +
>> +override define EMIT_TESTS
>> + echo "./run.sh"
>> +endef
>
>Ditto.
>
>cheers
>
>
>
>
>
--
Darren Hart
Intel Open Source Technology Center
^ permalink raw reply
* Re: [PATCH v4 00/14] Add kdbus implementation
From: David Herrmann @ 2015-03-30 16:56 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Greg Kroah-Hartman, Arnd Bergmann, Eric W. Biederman,
One Thousand Gnomes, Tom Gundersen, Jiri Kosina, Linux API,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Daniel Mack,
Djalal Harouni
In-Reply-To: <CALCETrUuWhtJ9w9vyCGgaJDMGithJad4A4wf-BuxWLb_af5eDg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
Hi
On Wed, Mar 25, 2015 at 7:12 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Wed, Mar 25, 2015 at 10:29 AM, David Herrmann <dh.herrmann-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
[...]
>>> I could be wrong about the lack of use cases. If so, please enlighten me.
>>
>> We have several dbus APIs that allow clients to register as a special
>> handler/controller/etc. (eg., see systemd-logind TakeControl()). The
>> API provider checks the privileges of a client on registration and
>> then just tracks the client ID. This way, the client can be privileged
>> when asking for special access, then drop privileges and still use the
>> interface. You cannot re-connect in between, as the API provider
>> tracks your bus ID. Without message-metadata, all your (other) calls
>> on this bus would always be treated as privileged. We *really* want to
>> avoid this.
>
> Connect twice?
The remote peer might cache your connection ID to track you. You have
to use the same connection to talk to that peer, in this given
scenario. That's how D-Bus1 is used today, and we have to follow the
semantics.
> You *already* have to reconnect or connect twice because you have
> per-connection metadata. That's part of my problem with this scheme
> -- you support *both styles*, which seems like it'll give you most of
> the downsides of both without the upsides.
Not necessarily. Connection metadata describes the state at the time
you connected to the bus. If someone ask for this information, they
will get exactly that. In this model, you cannot drop privileges, if
you need to be privileged during setup.
If someone asks for per-message metadata, they better ought not ask
for per-connection creds. It's not the information they're looking
for, so it will not match the data that at the time the message was
sent.
There is no immediate need to make both match. For security decisions,
we mandate per-message creds. Per-connection creds are for
backwards-compatibility to dbus1 and for passive introspection of bus
connections.
>>
>> Another example is logging, where we want exact data at the time a
>> message is logged. Otherwise, the data is useless.
>
> Why?
>
> No, really, why is exact data at the time of logging so important? It
> sounds nice, but I really don't see it.
Example: If you don't have message-metadata, you don't know the thread
which sent a log-message. In a multi-threaded application, that's
incredibly useful information.
After all, logging is all about correct data. Logging creds that were
not effective at the time the message was sent is futile.
>>
>> kdbus is a _bus_, not a p2p channel. Thus, a peer may talk to multiple
>> destinations, and it may want to look different to each of them. DBus
>> method-calls allow 'syscall'-ish behavior when calling into other
>> processes. We *want* to be able to drop privileges after doing process
>> setup. We want further bus-calls to no longer be treated privileged.
>
> You could have an IOCTL that re-captures your connection metata.
I don't see how this makes the model easier, or more predictable. On
the contrary, per-connection metadata is no longer compatible to UDS /
dbus1, nor is a per-message metadata concept reliable.
>>
>> Furthermore, DBus was designed to allow peers to track other peers
>> (which is why it always had the NameOwnerChanged signal). This is an
>> essential feature, that simplifies access-management considerably, as
>> you can cache it together with the unique name of a peer. We only open
>> a single connection to a bus. glib, libdbus, efl, ell, qt, sd-bus, and
>> others use cached bus-connections that are shared by all code of a
>> single thread. Hence, the bus connection is kinda part of the process
>> itself, like stdin/stdout. Without message-metadata, it is impossible
>> to ever drop privileges on a bus, without losing all state.
>
> See above about an IOCTL that re-captures your connection metadata.
>
> Again, you seem to be arguing that per-connection metadata is bad, but
> you still have an implementation of per-connection metadata, so you
> still have all these problems.
I don't see why we get the problems of per-connection metadata. Just
because you _can_ use it, doesn't mean you should use it for all
imaginable use-cases. The same goes for reading information from
/proc. There are valid use-cases to do so, but also a lot of cases
where it will not provide the information you want.
> I'm actually okay with per-message metadata in principle, but I'd like
> to see evidence (with numbers, please) that a send+recv of per-message
> metadata is *not* significantly slower than a recv of already-captured
> per-connection metadata. If this is in fact the case, then maybe you
> should trash per-connection metadata instead and the legacy
> compatibility code can figure out a way to deal with it. IMO that
> would be a pretty nice outcome, since you would never have to worry
> whether your connection to the bus is inadvertantly privileged.
Per-message metadata makes SEND about 25% slower, if you transmit the
full set of all possible information. Just 3% if you only use
PIDs+UIDs. The expensive metadata is cgroup-path and exe-path.
If a service needs that information, however, and if that information
is not guaranteed to be up-to-date, the service _will_ go and look it
up in /proc or somewhere else, which is certainly a whole lot more
expensive than the code in kdbus.
In general, there seems to be a number of misconception in this thread
about what kdbus is supposed to be. We're not inventing something new
here with a clean slate, but we're moving parts of an existing
implementation that has tons of users into the kernel, in order to fix
issues that cannot be fixed otherwise in userspace (most notably, the
race gaps that exist when retrieving per-message metadata). Therefore,
we have to keep existing semantics stable, otherwise the exercise is
somewhat pointless.
Thanks
David
^ permalink raw reply
* Re: [PATCH v7 0/5] vfs: Non-blockling buffered fs read (page cache only)
From: Jeremy Allison @ 2015-03-30 17:19 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jeremy Allison, Andrew Morton, Milosz Tanski,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
linux-aio-Bw31MaZKKs3YtjvyW6yDsg, Mel Gorman, Volker Lendecke,
Tejun Heo, Jeff Moyer, Theodore Ts'o, Al Viro,
linux-api-u79uwXL29TY76Z2rM5mHXA, Michael Kerrisk,
linux-arch-u79uwXL29TY76Z2rM5mHXA, Dave Chinner
In-Reply-To: <20150330073604.GB22229-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
On Mon, Mar 30, 2015 at 12:36:04AM -0700, Christoph Hellwig wrote:
> On Fri, Mar 27, 2015 at 08:58:54AM -0700, Jeremy Allison wrote:
> > The problem with the above is that we can't tell the difference
> > between pread2() returning a short read because the pages are not
> > in cache, or because someone truncated the file. So we need some
> > way to differentiate this.
>
> Is a race vs truncate really that time critical that you can't
> wait for the thread pool to do the second read to notice it?
Probably not, as this is the fallback path anyway.
> > My preference from userspace would be for pread2() to return
> > EAGAIN if *all* the data requested is not available (where
> > 'all' can be less than the size requested if the file has
> > been truncated in the meantime).
>
> That is easily implementable, but I can see that for example web apps
> would be happy to get as much as possible. So if Samba can be ok
> with short reads and only detecting the truncated case in the slow
> path that would make life simpler. Otherwise we might indeed need two
> flags.
Simpler is better. I can live with the partial read+fallback.
Jeremy.
^ permalink raw reply
* [PATCH 1/6] virtio_balloon: transitional interface
From: Michael S. Tsirkin @ 2015-03-30 17:37 UTC (permalink / raw)
To: linux-kernel; +Cc: virtio-dev, linux-api, virtualization
In-Reply-To: <1427737006-23191-1-git-send-email-mst@redhat.com>
Virtio 1.0 doesn't include a modern balloon device.
But it's not a big change to support a transitional
balloon device: this has the advantage of supporting
existing drivers, transparently.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
include/uapi/linux/virtio_balloon.h | 6 ++++++
drivers/virtio/virtio_balloon.c | 8 ++++++--
2 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
index 4b0488f..5d83902 100644
--- a/include/uapi/linux/virtio_balloon.h
+++ b/include/uapi/linux/virtio_balloon.h
@@ -56,4 +56,10 @@ struct virtio_balloon_stat {
__u64 val;
} __attribute__((packed));
+struct virtio_balloon_stat_modern {
+ __u8 reserved[6];
+ __u16 tag;
+ __u64 val;
+};
+
#endif /* _LINUX_VIRTIO_BALLOON_H */
diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
index 6a356e3..68e937f 100644
--- a/drivers/virtio/virtio_balloon.c
+++ b/drivers/virtio/virtio_balloon.c
@@ -77,7 +77,7 @@ struct virtio_balloon {
/* Memory statistics */
int need_stats_update;
- struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
+ struct virtio_balloon_stat_modern stats[VIRTIO_BALLOON_S_NR];
/* To register callback in oom notifier call chain */
struct notifier_block nb;
@@ -269,7 +269,11 @@ static void stats_handle_request(struct virtio_balloon *vb)
vq = vb->stats_vq;
if (!virtqueue_get_buf(vq, &len))
return;
- sg_init_one(&sg, vb->stats, sizeof(vb->stats));
+ if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
+ sg_init_one(&sg, vb->stats, sizeof(vb->stats));
+ else
+ sg_init_one(&sg, &vb->stats->tag, sizeof(vb->stats) -
+ offsetof(typeof(*vb->stats, tag);
virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
virtqueue_kick(vq);
}
--
MST
^ permalink raw reply related
* Re: [PATCH 1/6] virtio_balloon: transitional interface
From: Michael S. Tsirkin @ 2015-03-30 17:55 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: virtio-dev-sDuHXQ4OtrM4h7I2RyI4rWD2FQJk+8+b,
virtualization-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
Rusty Russell, linux-api-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1427737006-23191-2-git-send-email-mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Mon, Mar 30, 2015 at 07:37:12PM +0200, Michael S. Tsirkin wrote:
> Virtio 1.0 doesn't include a modern balloon device.
> But it's not a big change to support a transitional
> balloon device: this has the advantage of supporting
> existing drivers, transparently.
>
> Signed-off-by: Michael S. Tsirkin <mst-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
Hmm, re-reading the code, this won't be enough for BE
guests. We are doing one extra byteswap there.
I'll fix up and repost.
> include/uapi/linux/virtio_balloon.h | 6 ++++++
> drivers/virtio/virtio_balloon.c | 8 ++++++--
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/include/uapi/linux/virtio_balloon.h b/include/uapi/linux/virtio_balloon.h
> index 4b0488f..5d83902 100644
> --- a/include/uapi/linux/virtio_balloon.h
> +++ b/include/uapi/linux/virtio_balloon.h
> @@ -56,4 +56,10 @@ struct virtio_balloon_stat {
> __u64 val;
> } __attribute__((packed));
>
> +struct virtio_balloon_stat_modern {
> + __u8 reserved[6];
> + __u16 tag;
> + __u64 val;
> +};
> +
> #endif /* _LINUX_VIRTIO_BALLOON_H */
> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> index 6a356e3..68e937f 100644
> --- a/drivers/virtio/virtio_balloon.c
> +++ b/drivers/virtio/virtio_balloon.c
> @@ -77,7 +77,7 @@ struct virtio_balloon {
>
> /* Memory statistics */
> int need_stats_update;
> - struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
> + struct virtio_balloon_stat_modern stats[VIRTIO_BALLOON_S_NR];
>
> /* To register callback in oom notifier call chain */
> struct notifier_block nb;
> @@ -269,7 +269,11 @@ static void stats_handle_request(struct virtio_balloon *vb)
> vq = vb->stats_vq;
> if (!virtqueue_get_buf(vq, &len))
> return;
> - sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> + if (virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
> + sg_init_one(&sg, vb->stats, sizeof(vb->stats));
> + else
> + sg_init_one(&sg, &vb->stats->tag, sizeof(vb->stats) -
> + offsetof(typeof(*vb->stats, tag);
> virtqueue_add_outbuf(vq, &sg, 1, vb, GFP_KERNEL);
> virtqueue_kick(vq);
> }
> --
> MST
>
^ permalink raw reply
* Re: [PATCH v7 0/5] vfs: Non-blockling buffered fs read (page cache only)
From: Andrew Morton @ 2015-03-30 18:54 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Milosz Tanski, LKML, linux-fsdevel@vger.kernel.org,
linux-aio@kvack.org, Mel Gorman, Volker Lendecke, Tejun Heo,
Jeff Moyer, Theodore Ts'o, Al Viro, Linux API,
Michael Kerrisk, linux-arch, Dave Chinner
In-Reply-To: <20150330074020.GC22229@infradead.org>
On Mon, 30 Mar 2015 00:40:20 -0700 Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Mar 27, 2015 at 10:04:11AM -0700, Andrew Morton wrote:
> > mm... I don't think we should be adding placeholders to the kernel API
> > to support code which hasn't been written, tested, reviewed, merged,
> > etc. It's possible none of this will ever happen and we end up with a
> > syscall nobody needs or uses. Plus it's always possible that during
> > this development we decide the pwrite2() interface needs alteration but
> > it's too late.
> >
> > What would be the downside of deferring pwrite2() until it's all
> > implemented?
>
> It _is_ implemented. I just decided to submit it separately as Miklos
> already has to deal with enough bikeshedding for his feature that I
> don't want to put the burden of dealing with the BS for the one I wrote
> on him.
afacit the only difference between this pwritev2() and the existing
pwritev() is that pwritev2() interprets pos==-1 as "current position",
which duplicates writev()?
Unless I've missed something, there's no point in merging this
pwritev2() and it would be better to separate this syscall out into a
pwritev2() patchset which can be considered and merged separately. For
the reasons described above.
^ permalink raw reply
* Re: [patch 1/2] mm, doc: cleanup and clarify munmap behavior for hugetlb memory
From: Hugh Dickins @ 2015-03-30 20:23 UTC (permalink / raw)
To: Eric B Munson
Cc: Hugh Dickins, David Rientjes, Andrew Morton, Jonathan Corbet,
Davide Libenzi, Luiz Capitulino, Shuah Khan, Andrea Arcangeli,
Joern Engel, Jianguo Wu, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-api-u79uwXL29TY76Z2rM5mHXA,
linux-doc-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20150330142336.GB17678-JqFfY2XvxFXQT0dZR+AlfA@public.gmane.org>
On Mon, 30 Mar 2015, Eric B Munson wrote:
> On Sun, 29 Mar 2015, Hugh Dickins wrote:
> >
> > Eric, I apologize for bringing you in to the discussion, and then
> > ignoring your input. I understand that you would like MAP_HUGETLB
> > to behave more understandably. We can all agree that the existing
> > behavior is unsatisfying. But it's many years too late now to
> > change it around - and I suspect that a full exercise to do so would
> > actually discover some good reasons why the original choices were made.
>
> No worries, my main concern was avoiding the confusion that led me down
> the rabbit hole of compaction and mlock. As long as the documentation,
> man pages, and the code all agree I am satisfied. I would have
> preferred to make the code match the docs, but I understand that
> changing the code now introduces a risk of breaking userspace.
>
> It is charitable of you to assume that there were good reasons for the
> original decision. But as the author of the code in question, I suspect
> the omission was one of my own inexperience.
No, you are both too modest and too arrogant :)
You were extending the existing hugetlbfs infrastructure to be
accessible through a MAP_HUGETLB interface. You therefore inherited
the defects (some probably necessary, others perhaps not) of the
original hugetlbfs implementation, which is where this disagreeable
behaviour comes from.
If you were to ask for MAP_HUGETLB to behave differently from mapping
hugetlbfs here, I would shout no. For a start, we'd have to add a
VM_HUGETLB2 flag so that each place that tests VM_HUGETLB (usually
through is_vm_hugetlb_page(vma) - sic) could decide how to behave
instead.
I for one have neither time nor inclination to write or review
any such patch.
Hugh
^ permalink raw reply
* Re: [PATCH v7 0/5] vfs: Non-blockling buffered fs read (page cache only)
From: Andrew Morton @ 2015-03-30 20:26 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jeremy Allison, Milosz Tanski, linux-kernel, linux-fsdevel,
linux-aio, Mel Gorman, Volker Lendecke, Tejun Heo, Jeff Moyer,
Theodore Ts'o, Al Viro, linux-api, Michael Kerrisk,
linux-arch, Dave Chinner
In-Reply-To: <20150330073604.GB22229@infradead.org>
On Mon, 30 Mar 2015 00:36:04 -0700 Christoph Hellwig <hch@infradead.org> wrote:
> On Fri, Mar 27, 2015 at 08:58:54AM -0700, Jeremy Allison wrote:
> > The problem with the above is that we can't tell the difference
> > between pread2() returning a short read because the pages are not
> > in cache, or because someone truncated the file. So we need some
> > way to differentiate this.
>
> Is a race vs truncate really that time critical that you can't
> wait for the thread pool to do the second read to notice it?
>
> > My preference from userspace would be for pread2() to return
> > EAGAIN if *all* the data requested is not available (where
> > 'all' can be less than the size requested if the file has
> > been truncated in the meantime).
>
> That is easily implementable, but I can see that for example web apps
> would be happy to get as much as possible. So if Samba can be ok
> with short reads and only detecting the truncated case in the slow
> path that would make life simpler. Otherwise we might indeed need two
> flags.
The problem is that many applications (including samba!) want
all-or-nothing behaviour, and preadv2() cannot provide it. By the time
preadv2() discovers a not-present page, it has already copied bulk data
out to userspace.
To fix this, preadv2() would need to take two passes across the pages,
pinning them in between and somehow blocking out truncate. That's a
big change.
With the current preadv2(), applications would have to do
nr_read = preadv2(..., offset, len, ...);
if (nr_read == len)
process data;
else
punt(offset + nr_read, len - nr_read);
and the worker thread will later have to splice together the initial
data and the later-arriving data, probably on another CPU, probably
after the initial data has gone cache-cold.
A cleaner solution is
if (fincore(fd, NULL, offset, len) == len) {
preadv(..., offset, len);
process data;
} else {
punt(offset, len);
}
This way all the data gets copied in a single hit and is cache-hot when
userspace processes it.
Comparing fincore()+pread() to preadv2():
pros:
a) fincore() may be used to provide both all-or-nothing and
part-read-ok behaviour cleanly and with optimum cache behaviour.
b) fincore() doesn't add overhead, complexity and stack depth to
core pagecache read() code. Nor does it expand VFS data structures.
c) with a non-NULL second argument, fincore provides the
mincore()-style page map.
cons:
d) fincore() is more expensive
e) fincore() will very occasionally block
Tradeoffs are involved. To decide on the best path we should examine
d). I expect that the overhead will be significant for small reads but
not significant for medium and large reads. Needs quantifying.
And I don't believe that e) will be a problem in the real world. It's
a significant increase in worst-case latency and a negligible increase
in average latency. I've asked at least three times for someone to
explain why this is unacceptable and no explanation has been provided.
--
To unsubscribe, send a message with 'unsubscribe linux-aio' in
the body to majordomo@kvack.org. For more info on Linux AIO,
see: http://www.kvack.org/aio/
Don't email: <a href=mailto:"aart@kvack.org">aart@kvack.org</a>
^ permalink raw reply
* Re: [PATCH v7 0/5] vfs: Non-blockling buffered fs read (page cache only)
From: Jeremy Allison @ 2015-03-30 20:32 UTC (permalink / raw)
To: Andrew Morton
Cc: Christoph Hellwig, Jeremy Allison, Milosz Tanski, linux-kernel,
linux-fsdevel, linux-aio, Mel Gorman, Volker Lendecke, Tejun Heo,
Jeff Moyer, Theodore Ts'o, Al Viro, linux-api,
Michael Kerrisk, linux-arch, Dave Chinner
In-Reply-To: <20150330132625.52b1250527ca3dcda79e349e@linux-foundation.org>
On Mon, Mar 30, 2015 at 01:26:25PM -0700, Andrew Morton wrote:
>
> cons:
>
> d) fincore() is more expensive
>
> e) fincore() will very occasionally block
The above is the killer for Samba. If fincore
returns true but when we schedule the pread
we block, we're hosed.
Once we block, we're done serving clients on the main
thread until this returns. That can cause unpredictable
response times which can cause client timeouts.
A fincore+pread solution that blocks is simply unsafe
to use for us. We'll have to stay with the threadpool :-(.
> And I don't believe that e) will be a problem in the real world. It's
> a significant increase in worst-case latency and a negligible increase
> in average latency. I've asked at least three times for someone to
> explain why this is unacceptable and no explanation has been provided.
See above.
^ permalink raw reply
* Re: [PATCH v7 0/5] vfs: Non-blockling buffered fs read (page cache only)
From: Andrew Morton @ 2015-03-30 20:37 UTC (permalink / raw)
To: Jeremy Allison
Cc: Christoph Hellwig, Milosz Tanski, linux-kernel, linux-fsdevel,
linux-aio, Mel Gorman, Volker Lendecke, Tejun Heo, Jeff Moyer,
Theodore Ts'o, Al Viro, linux-api, Michael Kerrisk,
linux-arch, Dave Chinner
In-Reply-To: <20150330203227.GA4987@samba2>
On Mon, 30 Mar 2015 13:32:27 -0700 Jeremy Allison <jra@samba.org> wrote:
> On Mon, Mar 30, 2015 at 01:26:25PM -0700, Andrew Morton wrote:
> >
> > cons:
> >
> > d) fincore() is more expensive
> >
> > e) fincore() will very occasionally block
>
> The above is the killer for Samba. If fincore
> returns true but when we schedule the pread
> we block, we're hosed.
>
> Once we block, we're done serving clients on the main
> thread until this returns. That can cause unpredictable
> response times which can cause client timeouts.
>
> A fincore+pread solution that blocks is simply unsafe
> to use for us. We'll have to stay with the threadpool :-(.
Finally. Thanks ;)
This implies that the samba main thread also has to avoid any memory
allocations both direct and within syscall and pagefault - those will
occasionally exhibit similar worse-case latency. Is this done now?
^ permalink raw reply
* Re: [PATCH v7 0/5] vfs: Non-blockling buffered fs read (page cache only)
From: Jeremy Allison @ 2015-03-30 20:49 UTC (permalink / raw)
To: Andrew Morton
Cc: Jeremy Allison, Christoph Hellwig, Milosz Tanski, linux-kernel,
linux-fsdevel, linux-aio, Mel Gorman, Volker Lendecke, Tejun Heo,
Jeff Moyer, Theodore Ts'o, Al Viro, linux-api,
Michael Kerrisk, linux-arch, Dave Chinner
In-Reply-To: <20150330133758.d2788f6de72f121170ff0301@linux-foundation.org>
On Mon, Mar 30, 2015 at 01:37:58PM -0700, Andrew Morton wrote:
> On Mon, 30 Mar 2015 13:32:27 -0700 Jeremy Allison <jra@samba.org> wrote:
>
> > On Mon, Mar 30, 2015 at 01:26:25PM -0700, Andrew Morton wrote:
> > >
> > > cons:
> > >
> > > d) fincore() is more expensive
> > >
> > > e) fincore() will very occasionally block
> >
> > The above is the killer for Samba. If fincore
> > returns true but when we schedule the pread
> > we block, we're hosed.
> >
> > Once we block, we're done serving clients on the main
> > thread until this returns. That can cause unpredictable
> > response times which can cause client timeouts.
> >
> > A fincore+pread solution that blocks is simply unsafe
> > to use for us. We'll have to stay with the threadpool :-(.
>
> Finally. Thanks ;)
>
> This implies that the samba main thread also has to avoid any memory
> allocations both direct and within syscall and pagefault - those will
> occasionally exhibit similar worse-case latency. Is this done now?
We don't do anything special around allocations in syscall.
For aio read we do talloc (internal memory allocator) the
return chunk before going into the pthread pread, so I
suppose this could block. Haven't seen this as a reported
problem though. I suppose you can say "well exactly the
same thing is true of fincore()" :-).
^ 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