* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-17 0:34 UTC (permalink / raw)
To: Segher Boessenkool
Cc: libc-alpha, musl, Nicholas Piggin, Florian Weimer, libc-dev,
linuxppc-dev
In-Reply-To: <20200416230235.GG26902@gate.crashing.org>
On Thu, Apr 16, 2020 at 06:02:35PM -0500, Segher Boessenkool wrote:
> On Thu, Apr 16, 2020 at 08:12:19PM +0200, Florian Weimer wrote:
> > > I think my choice would be just making the inline syscall be a single
> > > call insn to an asm source file that out-of-lines the loading of TOC
> > > pointer and call through it or branch based on hwcap so that it's not
> > > repeated all over the place.
> >
> > I don't know how problematic control flow out of an inline asm is on
> > POWER. But this is basically the -moutline-atomics approach.
>
> Control flow out of inline asm (other than with "asm goto") is not
> allowed at all, just like on any other target (and will not work in
> practice, either -- just like on any other target). But the suggestion
> was to use actual assembler code, not inline asm?
Calling it control flow out of inline asm is something of a misnomer.
The enclosing state is not discarded or altered; the asm statement
exits normally, reaching the next instruction in the enclosing
block/function as soon as the call from the asm statement returns,
with all register/clobber constraints satisfied.
Control flow out of inline asm would be more like longjmp, and it can
be valid -- for instance, you can implement coroutines this way
(assuming you switch stack correctly) or do longjmp this way (jumping
to the location saved by setjmp). But it's not what'd be happening
here.
Rich
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Segher Boessenkool @ 2020-04-16 23:02 UTC (permalink / raw)
To: Florian Weimer
Cc: Rich Felker, libc-alpha, musl, Nicholas Piggin, libc-dev,
linuxppc-dev
In-Reply-To: <87ftd3e1vg.fsf@mid.deneb.enyo.de>
On Thu, Apr 16, 2020 at 08:12:19PM +0200, Florian Weimer wrote:
> > I think my choice would be just making the inline syscall be a single
> > call insn to an asm source file that out-of-lines the loading of TOC
> > pointer and call through it or branch based on hwcap so that it's not
> > repeated all over the place.
>
> I don't know how problematic control flow out of an inline asm is on
> POWER. But this is basically the -moutline-atomics approach.
Control flow out of inline asm (other than with "asm goto") is not
allowed at all, just like on any other target (and will not work in
practice, either -- just like on any other target). But the suggestion
was to use actual assembler code, not inline asm?
Segher
^ permalink raw reply
* Re: [PATCH V2] vhost: do not enable VHOST_MENU by default
From: Michael S. Tsirkin @ 2020-04-16 22:55 UTC (permalink / raw)
To: Jason Wang
Cc: linux-s390, tsbogend, gor, kvm, linux-kernel, heiko.carstens,
linux-mips, virtualization, borntraeger, geert, netdev, paulus,
linuxppc-dev
In-Reply-To: <20200415024356.23751-1-jasowang@redhat.com>
On Wed, Apr 15, 2020 at 10:43:56AM +0800, Jason Wang wrote:
> We try to keep the defconfig untouched after decoupling CONFIG_VHOST
> out of CONFIG_VIRTUALIZATION in commit 20c384f1ea1a
> ("vhost: refine vhost and vringh kconfig") by enabling VHOST_MENU by
> default. Then the defconfigs can keep enabling CONFIG_VHOST_NET
> without the caring of CONFIG_VHOST.
>
> But this will leave a "CONFIG_VHOST_MENU=y" in all defconfigs and even
> for the ones that doesn't want vhost. So it actually shifts the
> burdens to the maintainers of all other to add "CONFIG_VHOST_MENU is
> not set". So this patch tries to enable CONFIG_VHOST explicitly in
> defconfigs that enables CONFIG_VHOST_NET and CONFIG_VHOST_VSOCK.
>
> Acked-by: Christian Borntraeger <borntraeger@de.ibm.com> (s390)
> Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Michael Ellerman <mpe@ellerman.id.au>
> Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
> Cc: Vasily Gorbik <gor@linux.ibm.com>
> Cc: Christian Borntraeger <borntraeger@de.ibm.com>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
I rebased this on top of OABI fix since that
seems more orgent to fix.
Pushed to my vhost branch pls take a look and
if possible test.
Thanks!
> ---
> Change since V1:
> - depends on EVENTFD for VHOST
> ---
> arch/mips/configs/malta_kvm_defconfig | 1 +
> arch/powerpc/configs/powernv_defconfig | 1 +
> arch/powerpc/configs/ppc64_defconfig | 1 +
> arch/powerpc/configs/pseries_defconfig | 1 +
> arch/s390/configs/debug_defconfig | 1 +
> arch/s390/configs/defconfig | 1 +
> drivers/vhost/Kconfig | 26 +++++++++-----------------
> 7 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/arch/mips/configs/malta_kvm_defconfig b/arch/mips/configs/malta_kvm_defconfig
> index 8ef612552a19..06f0c7a0ca87 100644
> --- a/arch/mips/configs/malta_kvm_defconfig
> +++ b/arch/mips/configs/malta_kvm_defconfig
> @@ -18,6 +18,7 @@ CONFIG_PCI=y
> CONFIG_VIRTUALIZATION=y
> CONFIG_KVM=m
> CONFIG_KVM_MIPS_DEBUG_COP0_COUNTERS=y
> +CONFIG_VHOST=m
> CONFIG_VHOST_NET=m
> CONFIG_MODULES=y
> CONFIG_MODULE_UNLOAD=y
> diff --git a/arch/powerpc/configs/powernv_defconfig b/arch/powerpc/configs/powernv_defconfig
> index 71749377d164..404245b4594d 100644
> --- a/arch/powerpc/configs/powernv_defconfig
> +++ b/arch/powerpc/configs/powernv_defconfig
> @@ -346,5 +346,6 @@ CONFIG_CRYPTO_DEV_VMX=y
> CONFIG_VIRTUALIZATION=y
> CONFIG_KVM_BOOK3S_64=m
> CONFIG_KVM_BOOK3S_64_HV=m
> +CONFIG_VHOST=m
> CONFIG_VHOST_NET=m
> CONFIG_PRINTK_TIME=y
> diff --git a/arch/powerpc/configs/ppc64_defconfig b/arch/powerpc/configs/ppc64_defconfig
> index 7e68cb222c7b..4599fc7be285 100644
> --- a/arch/powerpc/configs/ppc64_defconfig
> +++ b/arch/powerpc/configs/ppc64_defconfig
> @@ -61,6 +61,7 @@ CONFIG_ELECTRA_CF=y
> CONFIG_VIRTUALIZATION=y
> CONFIG_KVM_BOOK3S_64=m
> CONFIG_KVM_BOOK3S_64_HV=m
> +CONFIG_VHOST=m
> CONFIG_VHOST_NET=m
> CONFIG_OPROFILE=m
> CONFIG_KPROBES=y
> diff --git a/arch/powerpc/configs/pseries_defconfig b/arch/powerpc/configs/pseries_defconfig
> index 6b68109e248f..4cad3901b5de 100644
> --- a/arch/powerpc/configs/pseries_defconfig
> +++ b/arch/powerpc/configs/pseries_defconfig
> @@ -321,5 +321,6 @@ CONFIG_CRYPTO_DEV_VMX=y
> CONFIG_VIRTUALIZATION=y
> CONFIG_KVM_BOOK3S_64=m
> CONFIG_KVM_BOOK3S_64_HV=m
> +CONFIG_VHOST=m
> CONFIG_VHOST_NET=m
> CONFIG_PRINTK_TIME=y
> diff --git a/arch/s390/configs/debug_defconfig b/arch/s390/configs/debug_defconfig
> index 0c86ba19fa2b..6ec6e69630d1 100644
> --- a/arch/s390/configs/debug_defconfig
> +++ b/arch/s390/configs/debug_defconfig
> @@ -57,6 +57,7 @@ CONFIG_PROTECTED_VIRTUALIZATION_GUEST=y
> CONFIG_CMM=m
> CONFIG_APPLDATA_BASE=y
> CONFIG_KVM=m
> +CONFIG_VHOST=m
> CONFIG_VHOST_NET=m
> CONFIG_VHOST_VSOCK=m
> CONFIG_OPROFILE=m
> diff --git a/arch/s390/configs/defconfig b/arch/s390/configs/defconfig
> index 6b27d861a9a3..d1b3bf83d687 100644
> --- a/arch/s390/configs/defconfig
> +++ b/arch/s390/configs/defconfig
> @@ -57,6 +57,7 @@ CONFIG_PROTECTED_VIRTUALIZATION_GUEST=y
> CONFIG_CMM=m
> CONFIG_APPLDATA_BASE=y
> CONFIG_KVM=m
> +CONFIG_VHOST=m
> CONFIG_VHOST_NET=m
> CONFIG_VHOST_VSOCK=m
> CONFIG_OPROFILE=m
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index e79cbbdfea45..29f171a53d8a 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -12,23 +12,19 @@ config VHOST_RING
> This option is selected by any driver which needs to access
> the host side of a virtio ring.
>
> -config VHOST
> - tristate
> +menuconfig VHOST
> + tristate "Vhost Devices"
> + depends on EVENTFD
> select VHOST_IOTLB
> help
> - This option is selected by any driver which needs to access
> - the core of vhost.
> + Enable option to support host kernel or hardware accelerator
> + for virtio device.
>
> -menuconfig VHOST_MENU
> - bool "VHOST drivers"
> - default y
> -
> -if VHOST_MENU
> +if VHOST
>
> config VHOST_NET
> tristate "Host kernel accelerator for virtio net"
> - depends on NET && EVENTFD && (TUN || !TUN) && (TAP || !TAP)
> - select VHOST
> + depends on NET && (TUN || !TUN) && (TAP || !TAP)
> ---help---
> This kernel module can be loaded in host kernel to accelerate
> guest networking with virtio_net. Not to be confused with virtio_net
> @@ -39,8 +35,7 @@ config VHOST_NET
>
> config VHOST_SCSI
> tristate "VHOST_SCSI TCM fabric driver"
> - depends on TARGET_CORE && EVENTFD
> - select VHOST
> + depends on TARGET_CORE
> default n
> ---help---
> Say M here to enable the vhost_scsi TCM fabric module
> @@ -48,8 +43,7 @@ config VHOST_SCSI
>
> config VHOST_VSOCK
> tristate "vhost virtio-vsock driver"
> - depends on VSOCKETS && EVENTFD
> - select VHOST
> + depends on VSOCKETS
> select VIRTIO_VSOCKETS_COMMON
> default n
> ---help---
> @@ -62,8 +56,6 @@ config VHOST_VSOCK
>
> config VHOST_VDPA
> tristate "Vhost driver for vDPA-based backend"
> - depends on EVENTFD
> - select VHOST
> depends on VDPA
> help
> This kernel module can be loaded in host kernel to accelerate
> --
> 2.20.1
^ permalink raw reply
* [PATCH v3] powerpc/setup_64: Set cache-line-size based on cache-block-size
From: Chris Packham @ 2020-04-16 22:19 UTC (permalink / raw)
To: mpe, benh, christophe.leroy, tglx, paulus, cai, oss
Cc: Chris Packham, linuxppc-dev, linux-kernel
If {i,d}-cache-block-size is set and {i,d}-cache-line-size is not, use
the block-size value for both. Per the devicetree spec cache-line-size
is only needed if it differs from the block size.
Originally the code would fallback from block size to line size. An
error message was printed if both properties were missing.
Later the code was refactored to use clearer names and logic but it
inadvertently made line size a required property. This caused the
default values to be used and in turn leads to Power9 systems using the
wrong size.
Fixes: bd067f83b084 ("powerpc/64: Fix naming of cache block vs. cache line")
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
It looks as though the bsizep = lsizep is not required per the spec but it's
probably safer to retain it.
Changes in v3:
- Rebase against 5.7.0-rc1
- Add Fixes tag
- Add more information to commit message
Changes in v2:
- Scott pointed out that u-boot should be filling in the cache properties
(which it does). But it does not specify a cache-line-size because it
provides a cache-block-size and the spec says you don't have to if they are
the same. So the error is in the parsing not in the devicetree itself.
arch/powerpc/kernel/setup_64.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/arch/powerpc/kernel/setup_64.c b/arch/powerpc/kernel/setup_64.c
index 438a9befce41..8105010b0e76 100644
--- a/arch/powerpc/kernel/setup_64.c
+++ b/arch/powerpc/kernel/setup_64.c
@@ -534,6 +534,8 @@ static bool __init parse_cache_info(struct device_node *np,
lsizep = of_get_property(np, propnames[3], NULL);
if (bsizep == NULL)
bsizep = lsizep;
+ if (lsizep == NULL)
+ lsizep = bsizep;
if (lsizep != NULL)
lsize = be32_to_cpu(*lsizep);
if (bsizep != NULL)
--
2.25.1
^ permalink raw reply related
* Re: [Bug 206203] kmemleak reports various leaks in drivers/of/unittest.c
From: Frank Rowand @ 2020-04-16 21:52 UTC (permalink / raw)
To: Michael Ellerman, Rob Herring; +Cc: devicetree, linuxppc-dev
In-Reply-To: <a717b335-de6c-46eb-1daa-ae5054664e93@gmail.com>
On 4/15/20 10:27 PM, Frank Rowand wrote:
> On 4/8/20 10:22 AM, Frank Rowand wrote:
>> Hi Michael,
>>
>> On 4/7/20 10:13 PM, Michael Ellerman wrote:
>>> bugzilla-daemon@bugzilla.kernel.org writes:
>>>> https://bugzilla.kernel.org/show_bug.cgi?id=206203
>>>>
>>>> Erhard F. (erhard_f@mailbox.org) changed:
>>>>
>>>> What |Removed |Added
>>>> ----------------------------------------------------------------------------
>>>> Attachment #286801|0 |1
>>>> is obsolete| |
>>>>
>>>> --- Comment #10 from Erhard F. (erhard_f@mailbox.org) ---
>>>> Created attachment 288189
>>>> --> https://bugzilla.kernel.org/attachment.cgi?id=288189&action=edit
>>>> kmemleak output (kernel 5.6.2, Talos II)
>>>
>>> These are all in or triggered by the of unittest code AFAICS.
>>> Content of the log reproduced below.
>>>
>>> Frank/Rob, are these memory leaks expected?
>>
>> Thanks for the report. I'll look at each one.
>
> Only one of the leaks was expected. I have patches to fix the
> unexpected leaks and to remove the expected leak so that the
> kmemleak report of it will not have to be checked again.
>
> I expect to send the patch series tomorrow (Thursday).
The patches for the memory leaks that I saw on an ARM board are at:
https://lore.kernel.org/r/1587073370-25963-1-git-send-email-frowand.list@gmail.com
-Frank
< snip >
^ permalink raw reply
* [PATCH] PCI: Use of_node_name_eq for node name comparisons
From: Rob Herring @ 2020-04-16 21:51 UTC (permalink / raw)
To: Bjorn Helgaas; +Cc: linux-pci, Paul Mackerras, linuxppc-dev
Convert string compares of DT node names to use of_node_name_eq helper
instead. This removes direct access to the node name pointer.
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-pci@vger.kernel.org
Signed-off-by: Rob Herring <robh@kernel.org>
---
drivers/pci/hotplug/rpaphp_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/hotplug/rpaphp_core.c b/drivers/pci/hotplug/rpaphp_core.c
index 6504869efabc..9887c9de08c3 100644
--- a/drivers/pci/hotplug/rpaphp_core.c
+++ b/drivers/pci/hotplug/rpaphp_core.c
@@ -435,7 +435,7 @@ static int rpaphp_drc_add_slot(struct device_node *dn)
*/
int rpaphp_add_slot(struct device_node *dn)
{
- if (!dn->name || strcmp(dn->name, "pci"))
+ if (!of_node_name_eq(dn, "pci"))
return 0;
if (of_find_property(dn, "ibm,drc-info", NULL))
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v4,4/4] drivers: uio: new driver for fsl_85xx_cache_sram
From: Rob Herring @ 2020-04-16 21:35 UTC (permalink / raw)
To: Scott Wood; +Cc: gregkh, linux-kernel, kernel, Wang Wenhu, linuxppc-dev
In-Reply-To: <16f8fa2d26d88f22ed05e9870709c2fd5c3960cf.camel@buserror.net>
On Thu, Apr 16, 2020 at 02:59:36PM -0500, Scott Wood wrote:
> On Thu, 2020-04-16 at 08:35 -0700, Wang Wenhu wrote:
> > +#define UIO_INFO_VER "devicetree,pseudo"
>
> What does this mean? Changing a number into a non-obvious string (Why
> "pseudo"? Why does the UIO user care that the config came from the device
> tree?) just to avoid setting off Greg's version number autoresponse isn't
> really helping anything.
>
> > +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
> > + { .compatible = "uio,mpc85xx-cache-sram", },
Form is <vendor>,<device> and "uio" is not a vendor (and never will be).
> > + {},
> > +};
> > +
> > +static struct platform_driver uio_fsl_85xx_cache_sram = {
> > + .probe = uio_fsl_85xx_cache_sram_probe,
> > + .remove = uio_fsl_85xx_cache_sram_remove,
> > + .driver = {
> > + .name = DRIVER_NAME,
> > + .owner = THIS_MODULE,
> > + .of_match_table = uio_mpc85xx_l2ctlr_of_match,
> > + },
> > +};
>
> Greg's comment notwithstanding, I really don't think this belongs in the
> device tree (and if I do get overruled on that point, it at least needs a
> binding document). Let me try to come up with a patch for dynamic allocation.
Agreed. "UIO" bindings have long been rejected.
Rob
^ permalink raw reply
* Re: [PATCH v2] powerpc/setup_64: Set cache-line-size based on cache-block-size
From: Chris Packham @ 2020-04-16 21:28 UTC (permalink / raw)
To: christophe.leroy@c-s.fr, paulus@samba.org, mpe@ellerman.id.au,
benh@kernel.crashing.org, oss@buserror.net, tglx@linutronix.de
Cc: Hamish Martin, linuxppc-dev@lists.ozlabs.org,
linux-kernel@vger.kernel.org
In-Reply-To: <87tv1jirlj.fsf@mpe.ellerman.id.au>
On Thu, 2020-04-16 at 21:43 +1000, Michael Ellerman wrote:
> Chris Packham <Chris.Packham@alliedtelesis.co.nz> writes:
> > Hi All,
> >
> > On Wed, 2020-03-25 at 16:18 +1300, Chris Packham wrote:
> > > If {i,d}-cache-block-size is set and {i,d}-cache-line-size is
> > > not,
> > > use
> > > the block-size value for both. Per the devicetree spec cache-
> > > line-
> > > size
> > > is only needed if it differs from the block size.
> > >
> > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> > > ---
> > > It looks as though the bsizep = lsizep is not required per the
> > > spec
> > > but it's
> > > probably safer to retain it.
> > >
> > > Changes in v2:
> > > - Scott pointed out that u-boot should be filling in the cache
> > > properties
> > > (which it does). But it does not specify a cache-line-size
> > > because
> > > it
> > > provides a cache-block-size and the spec says you don't have to
> > > if
> > > they are
> > > the same. So the error is in the parsing not in the devicetree
> > > itself.
> > >
> >
> > Ping? This thread went kind of quiet.
>
> I replied in the other thread:
>
>
> https://lore.kernel.org/linuxppc-dev/87369xx99u.fsf@mpe.ellerman.id.au/
>
> But then the merge window happened which is a busy time.
>
Yeah I figured that was the case.
> What I'd really like is a v3 that incorporates the info I wrote in
> the
> other thread and a Fixes tag.
>
> If you feel like doing that, that would be great. Otherwise I'll do
> it
> tomorrow.
I'll rebase against Linus's tree and have a go a adding some more words
to the commit message.
>
> cheers
^ permalink raw reply
* Re: [PATCH] PCI/DPC: Allow Non-ACPI Native ports to use DPC
From: Derrick, Jonathan @ 2020-04-16 20:50 UTC (permalink / raw)
To: sathyanarayanan.kuppuswamy@linux.intel.com, helgaas@kernel.org
Cc: bhelgaas@google.com, fred@fredlawl.com, sbobroff@linux.ibm.com,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
kbusch@kernel.org, oohall@gmail.com, olof@lixom.net,
linuxppc-dev@lists.ozlabs.org
In-Reply-To: <4c2bf639-5510-fb17-2f02-73e7016d8f70@linux.intel.com>
On Thu, 2020-04-16 at 13:32 -0700, Kuppuswamy, Sathyanarayanan wrote:
> Hi,
>
> On 4/16/20 12:59 PM, Jon Derrick wrote:
> > Some platforms have a mix of ports whose capabilities can be negotiated
> > by _OSC, and some ports which are not described by ACPI and instead
> > managed by Native drivers. The existing Firmware-First HEST model can
> > incorrectly tag these Native, Non-ACPI ports as Firmware-First capable
> > ports by advertising the HEST Global flag and specifying the type and
> > class (aer_hest_parse).
> >
> > This ultimately can lead to bad situations if the BIOS or port firmware
> > leaves DPC preconfigured and the Linux DPC driver is unable to bind to
> > the port to handle DPC events.
> >
> > This patch adds the check for Native DPC in the port's host bridge in
> > order to allow DPC services to bind to the port.
> >
> > Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> > ---
> > drivers/pci/pcie/dpc.c | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> > index 7621704..a1e355d 100644
> > --- a/drivers/pci/pcie/dpc.c
> > +++ b/drivers/pci/pcie/dpc.c
> > @@ -281,10 +281,12 @@ static int dpc_probe(struct pcie_device *dev)
> > {
> > struct pci_dev *pdev = dev->port;
> > struct device *device = &dev->device;
> > + struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
> > int status;
> > u16 ctl, cap;
> >
> > - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
> > + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native &&
> For other PCIe services, this check is added in
> get_port_device_capability().
> Why not add it there for DPC as well ?
Sure. Looking at this, it seems like it needs some more de-tangling to
fit into my model.
> > + !host->native_dpc)
> > return -ENOTSUPP;
> >
> > status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
> >
^ permalink raw reply
* Re: [PATCH 11/29] mm: only allow page table mappings for built-in zsmalloc
From: Minchan Kim @ 2020-04-16 20:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-hyperv, sergey.senozhatsky.work, David Airlie, dri-devel,
linux-mm, K. Y. Srinivasan, Sumit Semwal, linux-arch, linux-s390,
Wei Liu, Stephen Hemminger, x86, Peter Zijlstra, Laura Abbott,
Nitin Gupta, Daniel Vetter, Haiyang Zhang, linaro-mm-sig, bpf,
linux-arm-kernel, Robin Murphy, linux-kernel, iommu, Sakari Ailus,
Andrew Morton, linuxppc-dev
In-Reply-To: <20200414131348.444715-12-hch@lst.de>
On Tue, Apr 14, 2020 at 03:13:30PM +0200, Christoph Hellwig wrote:
> This allows to unexport map_vm_area and unmap_kernel_range, which are
> rather deep internal and should not be available to modules, as they for
> example allow fine grained control of mapping permissions, and also
> allow splitting the setup of a vmalloc area and the actual mapping and
> thus expose vmalloc internals.
>
> zsmalloc is typically built-in and continues to work (just like the
> percpu-vm code using a similar patter), while modular zsmalloc also
> continues to work, but must use copies.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Minchan Kim <minchan@kernel.org>
Thanks!
^ permalink raw reply
* Re: [PATCH 10/28] mm: only allow page table mappings for built-in zsmalloc
From: Minchan Kim @ 2020-04-16 20:37 UTC (permalink / raw)
To: Christoph Hellwig
Cc: linux-hyperv, David Airlie, dri-devel, linux-mm, K. Y. Srinivasan,
Sumit Semwal, linux-arch, linux-s390, Wei Liu, Stephen Hemminger,
x86, Peter Zijlstra, Laura Abbott, Nitin Gupta, Daniel Vetter,
Haiyang Zhang, linaro-mm-sig, bpf, linux-arm-kernel, Robin Murphy,
linux-kernel, Sergey Senozhatsky, iommu, Sakari Ailus,
Andrew Morton, linuxppc-dev
In-Reply-To: <20200411072052.GA31242@lst.de>
Hi Christoph,
Sorry for the late.
On Sat, Apr 11, 2020 at 09:20:52AM +0200, Christoph Hellwig wrote:
> Hi Minchan,
>
> On Fri, Apr 10, 2020 at 04:11:36PM -0700, Minchan Kim wrote:
> > It doesn't mean we couldn't use zsmalloc as module any longer. It means
> > we couldn't use zsmalloc as module with pgtable mapping whcih was little
> > bit faster on microbenchmark in some architecutre(However, I usually temped
> > to remove it since it had several problems). However, we could still use
> > zsmalloc as module as copy way instead of pgtable mapping. Thus, if someone
> > really want to rollback the feature, they should provide reasonable reason
> > why it doesn't work for them. "A little fast" wouldn't be enough to exports
> > deep internal to the module.
>
> do you have any data how much faster it is on arm (and does that include
> arm64 as well)? Besides the exports which were my prime concern,
https://github.com/sjenning/zsmapbench
I need to recall the memory. IIRC, it was almost 30% faster at that time
in ARM so was not trivial at that time. However, it was story from
several years ago.
> zsmalloc with pgtable mappings also is the only user of map_kernel_range
> outside of vmalloc.c, if it really is another code base for tiny
> improvements we could mark map_kernel_range or in fact remove it entirely
> and open code it in the remaining callers.
I alsh have temped to remove it. Let me have time to revist it in this
chance.
Thanks.
^ permalink raw reply
* Re: [PATCH] PCI/DPC: Allow Non-ACPI Native ports to use DPC
From: Kuppuswamy, Sathyanarayanan @ 2020-04-16 20:32 UTC (permalink / raw)
To: Jon Derrick, Bjorn Helgaas
Cc: Frederick Lawler, Sam Bobroff, linux-pci, linux-kernel,
Keith Busch, Olof Johansson, Oliver O'Halloran, Bjorn Helgaas,
linuxppc-dev
In-Reply-To: <1587067157-2291-1-git-send-email-jonathan.derrick@intel.com>
Hi,
On 4/16/20 12:59 PM, Jon Derrick wrote:
> Some platforms have a mix of ports whose capabilities can be negotiated
> by _OSC, and some ports which are not described by ACPI and instead
> managed by Native drivers. The existing Firmware-First HEST model can
> incorrectly tag these Native, Non-ACPI ports as Firmware-First capable
> ports by advertising the HEST Global flag and specifying the type and
> class (aer_hest_parse).
>
> This ultimately can lead to bad situations if the BIOS or port firmware
> leaves DPC preconfigured and the Linux DPC driver is unable to bind to
> the port to handle DPC events.
>
> This patch adds the check for Native DPC in the port's host bridge in
> order to allow DPC services to bind to the port.
>
> Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
> ---
> drivers/pci/pcie/dpc.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
> index 7621704..a1e355d 100644
> --- a/drivers/pci/pcie/dpc.c
> +++ b/drivers/pci/pcie/dpc.c
> @@ -281,10 +281,12 @@ static int dpc_probe(struct pcie_device *dev)
> {
> struct pci_dev *pdev = dev->port;
> struct device *device = &dev->device;
> + struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
> int status;
> u16 ctl, cap;
>
> - if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
> + if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native &&
For other PCIe services, this check is added in
get_port_device_capability().
Why not add it there for DPC as well ?
> + !host->native_dpc)
> return -ENOTSUPP;
>
> status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
>
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Florian Weimer @ 2020-04-16 20:18 UTC (permalink / raw)
To: Nicholas Piggin via Libc-alpha
Cc: libc-dev, Rich Felker, linuxppc-dev, Nicholas Piggin, musl
In-Reply-To: <1587004907.ioxh0bxsln.astroid@bobo.none>
* Nicholas Piggin via Libc-alpha:
> We may or may not be getting a new ABI that will use instructions not
> supported by old processors.
>
> https://sourceware.org/legacy-ml/binutils/2019-05/msg00331.html
>
> Current ABI continues to work of course and be the default for some
> time, but building for new one would give some opportunity to drop
> such support for old procs, at least for glibc.
If I recall correctly, during last year's GNU Tools Cauldron, I think
it was pretty clear that this was only to be used for intra-DSO ABIs,
not cross-DSO optimization. Relocatable object files have an ABI,
too, of course, so that's why there's a ABI documentation needed.
For cross-DSO optimization, the link editor would look at the DSO
being linked in, check if it uses the -mfuture ABI, and apply some
shortcuts. But at that point, if the DSO is swapped back to a version
built without -mfuture, it no longer works with those newly linked
binaries against the -mfuture version. Such a thing is a clear ABI
bump, and based what I remember from Cauldron, that is not the plan
here.
(I don't have any insider knowledge—I just don't want people to read
this think: gosh, yet another POWER ABI bump. But the PCREL stuff
*is* exciting!)
^ permalink raw reply
* [PATCH] PCI/DPC: Allow Non-ACPI Native ports to use DPC
From: Jon Derrick @ 2020-04-16 19:59 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: Kuppuswamy Sathyanarayanan, Frederick Lawler, Sam Bobroff,
linux-pci, linux-kernel, Keith Busch, Olof Johansson,
Oliver O'Halloran, Bjorn Helgaas, linuxppc-dev, Jon Derrick
Some platforms have a mix of ports whose capabilities can be negotiated
by _OSC, and some ports which are not described by ACPI and instead
managed by Native drivers. The existing Firmware-First HEST model can
incorrectly tag these Native, Non-ACPI ports as Firmware-First capable
ports by advertising the HEST Global flag and specifying the type and
class (aer_hest_parse).
This ultimately can lead to bad situations if the BIOS or port firmware
leaves DPC preconfigured and the Linux DPC driver is unable to bind to
the port to handle DPC events.
This patch adds the check for Native DPC in the port's host bridge in
order to allow DPC services to bind to the port.
Signed-off-by: Jon Derrick <jonathan.derrick@intel.com>
---
drivers/pci/pcie/dpc.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/pcie/dpc.c b/drivers/pci/pcie/dpc.c
index 7621704..a1e355d 100644
--- a/drivers/pci/pcie/dpc.c
+++ b/drivers/pci/pcie/dpc.c
@@ -281,10 +281,12 @@ static int dpc_probe(struct pcie_device *dev)
{
struct pci_dev *pdev = dev->port;
struct device *device = &dev->device;
+ struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
int status;
u16 ctl, cap;
- if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native)
+ if (pcie_aer_get_firmware_first(pdev) && !pcie_ports_dpc_native &&
+ !host->native_dpc)
return -ENOTSUPP;
status = devm_request_threaded_irq(device, dev->irq, dpc_irq,
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH v4,4/4] drivers: uio: new driver for fsl_85xx_cache_sram
From: Scott Wood @ 2020-04-16 19:59 UTC (permalink / raw)
To: Wang Wenhu, gregkh, linux-kernel, christophe.leroy, linuxppc-dev; +Cc: kernel
In-Reply-To: <20200416153537.23736-5-wenhu.wang@vivo.com>
On Thu, 2020-04-16 at 08:35 -0700, Wang Wenhu wrote:
> +#define UIO_INFO_VER "devicetree,pseudo"
What does this mean? Changing a number into a non-obvious string (Why
"pseudo"? Why does the UIO user care that the config came from the device
tree?) just to avoid setting off Greg's version number autoresponse isn't
really helping anything.
> +static const struct of_device_id uio_mpc85xx_l2ctlr_of_match[] = {
> + { .compatible = "uio,mpc85xx-cache-sram", },
> + {},
> +};
> +
> +static struct platform_driver uio_fsl_85xx_cache_sram = {
> + .probe = uio_fsl_85xx_cache_sram_probe,
> + .remove = uio_fsl_85xx_cache_sram_remove,
> + .driver = {
> + .name = DRIVER_NAME,
> + .owner = THIS_MODULE,
> + .of_match_table = uio_mpc85xx_l2ctlr_of_match,
> + },
> +};
Greg's comment notwithstanding, I really don't think this belongs in the
device tree (and if I do get overruled on that point, it at least needs a
binding document). Let me try to come up with a patch for dynamic allocation.
-Scott
^ permalink raw reply
* Re: [PATCH v2,5/5] drivers: uio: new driver for fsl_85xx_cache_sram
From: Scott Wood @ 2020-04-16 19:40 UTC (permalink / raw)
To: Greg KH; +Cc: linux-kernel, kernel, Wang Wenhu, linuxppc-dev
In-Reply-To: <20200416063002.GA299193@kroah.com>
On Thu, 2020-04-16 at 08:30 +0200, Greg KH wrote:
> On Wed, Apr 15, 2020 at 02:26:55PM -0500, Scott Wood wrote:
> > Instead, have module parameters that take the sizes and alignments you'd
> > like
> > to allocate and expose to userspace. Better still would be some sort of
> > dynamic allocation (e.g. open a fd, ioctl to set the requested
> > size/alignment,
> > if it succeeds you can mmap it, and when the fd is closed the region is
> > freed).
>
> No module parameters please, this is not the 1990's.
>
> Use device tree, that is what it is there for.
Since when is the device tree for indicating desired allocations? This is not
hardware description.
If module parameters are unacceptable, then I'd suggest dynamic allocation as
described above.
-Scott
^ permalink raw reply
* Re: [PATCH v2,5/5] drivers: uio: new driver for fsl_85xx_cache_sram
From: Scott Wood @ 2020-04-16 19:36 UTC (permalink / raw)
To: Greg KH; +Cc: linux-kernel, kernel, Wang Wenhu, linuxppc-dev
In-Reply-To: <20200416063034.GB299193@kroah.com>
On Thu, 2020-04-16 at 08:30 +0200, Greg KH wrote:
> On Wed, Apr 15, 2020 at 02:27:51PM -0500, Scott Wood wrote:
> > > > + dev_err(&pdev->dev, "error no valid uio-map
> > > > configured\n");
> > > > + ret = -EINVAL;
> > > > + goto err_info_free_internel;
> > > > + }
> > > > +
> > > > + info->version = "0.1.0";
> > >
> > > Could you define some DRIVER_VERSION in the top of the file next to
> > > DRIVER_NAME instead of hard coding in the middle on a function ?
> >
> > That's what v1 had, and Greg KH said to remove it. I'm guessing that he
> > thought it was the common-but-pointless practice of having the driver
> > print a
> > version number that never gets updated, rather than something the UIO API
> > (unfortunately, compared to a feature query interface) expects. That
> > said,
> > I'm not sure what the value is of making it a macro since it should only
> > be
> > used once, that use is self documenting, it isn't tunable, etc. Though if
> > this isn't a macro, UIO_NAME also shouldn't be (and if it is made a macro
> > again, it should be UIO_VERSION, not DRIVER_VERSION).
> >
> > Does this really need a three-part version scheme? What's wrong with a
> > version of "1", to be changed to "2" in the hopefully-unlikely event that
> > the
> > userspace API changes? Assuming UIO is used for this at all, which
> > doesn't
> > seem like a great fit to me.
>
> No driver version numbers at all please, they do not make any sense when
> the driver is included in the kernel tree.
Again, reporting a version string is part of the UIO API. It might not be a
good API, but if it's left as NULL the registration will fail.
-Scott
^ permalink raw reply
* Re: [PATCH v2] powerpc/uaccess: Implement unsafe_put_user() using 'asm goto'
From: Segher Boessenkool @ 2020-04-16 19:01 UTC (permalink / raw)
To: Christophe Leroy; +Cc: linux-kernel, npiggin, Paul Mackerras, linuxppc-dev
In-Reply-To: <f797e5ac-a29a-0eb0-89c8-ff0a9f537ccf@c-s.fr>
On Thu, Apr 16, 2020 at 02:41:56PM +0200, Christophe Leroy wrote:
> Le 16/04/2020 à 00:37, Segher Boessenkool a écrit :
> >>+ __put_user_nocheck_goto((__typeof__(*(ptr)))(x), (ptr),
> >>sizeof(*(ptr)), label)
> >
> >This line gets too long, can you break it up somehow?
>
> This line has 86 chars.
(And your mail program has wrapped it ;-) )
> powerpc arch tolerates lines with up to 90 chars, see
> arch/powerpc/tools/checkpatch.sh
I *tolerate* it as well, sure, but long lines are bad for readability.
Like, I noticed it because it wrapped :-)
That "90" thing is just dumb, we should get rid of it. Sometimes you
can have long lines, if that is better than the alternatives. There
does not need to be a ridiculous "rule" that is unhappy *both* ways!
(This is true for many things in checkpatch, btw... Rules of thumb,
not rules).
Segher
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Adhemerval Zanella @ 2020-04-16 18:52 UTC (permalink / raw)
To: Rich Felker; +Cc: musl, libc-alpha, linuxppc-dev, Nicholas Piggin, libc-dev
In-Reply-To: <20200416183151.GA11469@brightrain.aerifal.cx>
On 16/04/2020 15:31, Rich Felker wrote:
> On Thu, Apr 16, 2020 at 03:18:42PM -0300, Adhemerval Zanella wrote:
>>
>>
>> On 16/04/2020 14:59, Rich Felker wrote:
>>> On Thu, Apr 16, 2020 at 02:50:18PM -0300, Adhemerval Zanella wrote:
>>>>
>>>>
>>>> On 16/04/2020 12:37, Rich Felker wrote:
>>>>> On Thu, Apr 16, 2020 at 11:16:04AM -0300, Adhemerval Zanella wrote:
>>>>>>> My preference would be that it work just like the i386 AT_SYSINFO
>>>>>>> where you just replace "int $128" with "call *%%gs:16" and the kernel
>>>>>>> provides a stub in the vdso that performs either scv or the old
>>>>>>> mechanism with the same calling convention. Then if the kernel doesn't
>>>>>>> provide it (because the kernel is too old) libc would have to provide
>>>>>>> its own stub that uses the legacy method and matches the calling
>>>>>>> convention of the one the kernel is expected to provide.
>>>>>>
>>>>>> What about pthread cancellation and the requirement of checking the
>>>>>> cancellable syscall anchors in asynchronous cancellation? My plan is
>>>>>> still to use musl strategy on glibc (BZ#12683) and for i686 it
>>>>>> requires to always use old int$128 for program that uses cancellation
>>>>>> (static case) or just threads (dynamic mode, which should be more
>>>>>> common on glibc).
>>>>>>
>>>>>> Using the i686 strategy of a vDSO bridge symbol would require to always
>>>>>> fallback to 'sc' to still use the same cancellation strategy (and
>>>>>> thus defeating this optimization in such cases).
>>>>>
>>>>> Yes, I assumed it would be the same, ignoring the new syscall
>>>>> mechanism for cancellable syscalls. While there are some exceptions,
>>>>> cancellable syscalls are generally not hot paths but things that are
>>>>> expected to block and to have significant amounts of work to do in
>>>>> kernelspace, so saving a few tens of cycles is rather pointless.
>>>>>
>>>>> It's possible to do a branch/multiple versions of the syscall asm for
>>>>> cancellation but would require extending the cancellation handler to
>>>>> support checking against multiple independent address ranges or using
>>>>> some alternate markup of them.
>>>>
>>>> The main issue is at least for glibc dynamic linking is way more common
>>>> than static linking and once the program become multithread the fallback
>>>> will be always used.
>>>
>>> I'm not relying on static linking optimizing out the cancellable
>>> version. I'm talking about how cancellable syscalls are pretty much
>>> all "heavy" operations to begin with where a few tens of cycles are in
>>> the realm of "measurement noise" relative to the dominating time
>>> costs.
>>
>> Yes I am aware, but at same time I am not sure how it plays on real world.
>> For instance, some workloads might issue kernel query syscalls, such as
>> recv, where buffer copying might not be dominant factor. So I see that if
>> the idea is optimizing syscall mechanism, we should try to leverage it
>> as whole in libc.
>
> Have you timed a minimal recv? I'm not assuming buffer copying is the
> dominant factor. I'm assuming the overhead of all the kernel layers
> involved is dominant.
Not really, but reading the advantages of using 'scv' over 'sc' also does
not outline the real expect gain. Taking in consideration this should
be a micro-optimization (focused on entry syscall patch), I think we should
use where it possible.
>
>>>> And besides the cancellation performance issue, a new bridge vDSO mechanism
>>>> will still require to setup some extra bridge for the case of the older
>>>> kernel. In the scheme you suggested:
>>>>
>>>> __asm__("indirect call" ... with common clobbers);
>>>>
>>>> The indirect call will be either the vDSO bridge or an libc provided that
>>>> fallback to 'sc' for !PPC_FEATURE2_SCV. I am not this is really a gain
>>>> against:
>>>>
>>>> if (hwcap & PPC_FEATURE2_SCV) {
>>>> __asm__(... with some clobbers);
>>>> } else {
>>>> __asm__(... with different clobbers);
>>>> }
>>>
>>> If the indirect call can be made roughly as efficiently as the sc
>>> sequence now (which already have some cost due to handling the nasty
>>> error return convention, making the indirect call likely just as small
>>> or smaller), it's O(1) additional code size (and thus icache usage)
>>> rather than O(n) where n is number of syscall points.
>>>
>>> Of course it would work just as well (for avoiding O(n) growth) to
>>> have a direct call to out-of-line branch like you suggested.
>>
>> Yes, but does it really matter to optimize this specific usage case
>> for size? glibc, for instance, tries to leverage the syscall mechanism
>> by adding some complex pre-processor asm directives. It optimizes
>> the syscall code size in most cases. For instance, kill in static case
>> generates on x86_64:
>>
>> 0000000000000000 <__kill>:
>> 0: b8 3e 00 00 00 mov $0x3e,%eax
>> 5: 0f 05 syscall
>> 7: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
>> d: 0f 83 00 00 00 00 jae 13 <__kill+0x13>
>> 13: c3 retq
>>
>> While on musl:
>>
>> 0000000000000000 <kill>:
>> 0: 48 83 ec 08 sub $0x8,%rsp
>> 4: 48 63 ff movslq %edi,%rdi
>> 7: 48 63 f6 movslq %esi,%rsi
>> a: b8 3e 00 00 00 mov $0x3e,%eax
>> f: 0f 05 syscall
>> 11: 48 89 c7 mov %rax,%rdi
>> 14: e8 00 00 00 00 callq 19 <kill+0x19>
>> 19: 5a pop %rdx
>> 1a: c3 retq
>
> Wow that's some extraordinarily bad codegen going on by gcc... The
> sign-extension is semantically needed and I don't see a good way
> around it (glibc's asm is kinda a hack taking advantage of kernel not
> looking at high bits, I think), but the gratuitous stack adjustment
> and refusal to generate a tail call isn't. I'll see if we can track
> down what's going on and get it fixed.
Wrt glibc, it is most likely and it has bitten us on x32 port recently
(where some types were being passed correctly). In any case, my long
term plan to also get rid of this nasty assembly pre-processor on
syscall passing.
>
>> But I hardly think it pays off the required code complexity. Some
>> for providing a O(1) bridge: this will require additional complexity
>> to write it and setup correctly.
>
> In some sense I agree, but inline instructions are a lot more
> expensive on ppc (being 32-bit each), and it might take out-of-lining
> anyway to get rid of stack frame setups if that ends up being a
> problem.
Indeed, I didn't started to prototype what would be required to make
this change on glibc. Maybe an out-of-line helper might make sense.
>
>>>> Specially if 'hwcap & PPC_FEATURE2_SCV' could be optimized with a
>>>> TCB member (as we do on glibc) and if we could make the asm clever
>>>> enough to not require different clobbers (although not sure if
>>>> it would be possible).
>>>
>>> The easy way not to require different clobbers is just using the union
>>> of the clobbers, no? Does the proposed new method clobber any
>>> call-saved registers that would make it painful (requiring new call
>>> frames to save them in)?
>>
>> As far I can tell, it should be ok.
>
> Note that because lr is clobbered we need at least once normally
> call-clobbered register that's not syscall clobbered to save lr in.
> Otherwise stack frame setup is required to spill it. (And I'm not even
> sure if gcc does things right to avoid it by using a register -- we
> should check that I guess...)
If I recall correctly Florian has found some issue in lr clobbering.
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-16 18:44 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: musl, libc-alpha, linuxppc-dev, Nicholas Piggin, libc-dev
In-Reply-To: <20200416183151.GA11469@brightrain.aerifal.cx>
On Thu, Apr 16, 2020 at 02:31:51PM -0400, Rich Felker wrote:
> > While on musl:
> >
> > 0000000000000000 <kill>:
> > 0: 48 83 ec 08 sub $0x8,%rsp
> > 4: 48 63 ff movslq %edi,%rdi
> > 7: 48 63 f6 movslq %esi,%rsi
> > a: b8 3e 00 00 00 mov $0x3e,%eax
> > f: 0f 05 syscall
> > 11: 48 89 c7 mov %rax,%rdi
> > 14: e8 00 00 00 00 callq 19 <kill+0x19>
> > 19: 5a pop %rdx
> > 1a: c3 retq
>
> Wow that's some extraordinarily bad codegen going on by gcc... The
> sign-extension is semantically needed and I don't see a good way
> around it (glibc's asm is kinda a hack taking advantage of kernel not
> looking at high bits, I think), but the gratuitous stack adjustment
> and refusal to generate a tail call isn't. I'll see if we can track
> down what's going on and get it fixed.
It seems to be https://gcc.gnu.org/bugzilla/show_bug.cgi?id=14441
which I've updated with a comment about the above.
Rich
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-16 18:31 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: musl, libc-alpha, linuxppc-dev, Nicholas Piggin, libc-dev
In-Reply-To: <4f824a37-e660-8912-25aa-fde88d4b79f3@linaro.org>
On Thu, Apr 16, 2020 at 03:18:42PM -0300, Adhemerval Zanella wrote:
>
>
> On 16/04/2020 14:59, Rich Felker wrote:
> > On Thu, Apr 16, 2020 at 02:50:18PM -0300, Adhemerval Zanella wrote:
> >>
> >>
> >> On 16/04/2020 12:37, Rich Felker wrote:
> >>> On Thu, Apr 16, 2020 at 11:16:04AM -0300, Adhemerval Zanella wrote:
> >>>>> My preference would be that it work just like the i386 AT_SYSINFO
> >>>>> where you just replace "int $128" with "call *%%gs:16" and the kernel
> >>>>> provides a stub in the vdso that performs either scv or the old
> >>>>> mechanism with the same calling convention. Then if the kernel doesn't
> >>>>> provide it (because the kernel is too old) libc would have to provide
> >>>>> its own stub that uses the legacy method and matches the calling
> >>>>> convention of the one the kernel is expected to provide.
> >>>>
> >>>> What about pthread cancellation and the requirement of checking the
> >>>> cancellable syscall anchors in asynchronous cancellation? My plan is
> >>>> still to use musl strategy on glibc (BZ#12683) and for i686 it
> >>>> requires to always use old int$128 for program that uses cancellation
> >>>> (static case) or just threads (dynamic mode, which should be more
> >>>> common on glibc).
> >>>>
> >>>> Using the i686 strategy of a vDSO bridge symbol would require to always
> >>>> fallback to 'sc' to still use the same cancellation strategy (and
> >>>> thus defeating this optimization in such cases).
> >>>
> >>> Yes, I assumed it would be the same, ignoring the new syscall
> >>> mechanism for cancellable syscalls. While there are some exceptions,
> >>> cancellable syscalls are generally not hot paths but things that are
> >>> expected to block and to have significant amounts of work to do in
> >>> kernelspace, so saving a few tens of cycles is rather pointless.
> >>>
> >>> It's possible to do a branch/multiple versions of the syscall asm for
> >>> cancellation but would require extending the cancellation handler to
> >>> support checking against multiple independent address ranges or using
> >>> some alternate markup of them.
> >>
> >> The main issue is at least for glibc dynamic linking is way more common
> >> than static linking and once the program become multithread the fallback
> >> will be always used.
> >
> > I'm not relying on static linking optimizing out the cancellable
> > version. I'm talking about how cancellable syscalls are pretty much
> > all "heavy" operations to begin with where a few tens of cycles are in
> > the realm of "measurement noise" relative to the dominating time
> > costs.
>
> Yes I am aware, but at same time I am not sure how it plays on real world.
> For instance, some workloads might issue kernel query syscalls, such as
> recv, where buffer copying might not be dominant factor. So I see that if
> the idea is optimizing syscall mechanism, we should try to leverage it
> as whole in libc.
Have you timed a minimal recv? I'm not assuming buffer copying is the
dominant factor. I'm assuming the overhead of all the kernel layers
involved is dominant.
> >> And besides the cancellation performance issue, a new bridge vDSO mechanism
> >> will still require to setup some extra bridge for the case of the older
> >> kernel. In the scheme you suggested:
> >>
> >> __asm__("indirect call" ... with common clobbers);
> >>
> >> The indirect call will be either the vDSO bridge or an libc provided that
> >> fallback to 'sc' for !PPC_FEATURE2_SCV. I am not this is really a gain
> >> against:
> >>
> >> if (hwcap & PPC_FEATURE2_SCV) {
> >> __asm__(... with some clobbers);
> >> } else {
> >> __asm__(... with different clobbers);
> >> }
> >
> > If the indirect call can be made roughly as efficiently as the sc
> > sequence now (which already have some cost due to handling the nasty
> > error return convention, making the indirect call likely just as small
> > or smaller), it's O(1) additional code size (and thus icache usage)
> > rather than O(n) where n is number of syscall points.
> >
> > Of course it would work just as well (for avoiding O(n) growth) to
> > have a direct call to out-of-line branch like you suggested.
>
> Yes, but does it really matter to optimize this specific usage case
> for size? glibc, for instance, tries to leverage the syscall mechanism
> by adding some complex pre-processor asm directives. It optimizes
> the syscall code size in most cases. For instance, kill in static case
> generates on x86_64:
>
> 0000000000000000 <__kill>:
> 0: b8 3e 00 00 00 mov $0x3e,%eax
> 5: 0f 05 syscall
> 7: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
> d: 0f 83 00 00 00 00 jae 13 <__kill+0x13>
> 13: c3 retq
>
> While on musl:
>
> 0000000000000000 <kill>:
> 0: 48 83 ec 08 sub $0x8,%rsp
> 4: 48 63 ff movslq %edi,%rdi
> 7: 48 63 f6 movslq %esi,%rsi
> a: b8 3e 00 00 00 mov $0x3e,%eax
> f: 0f 05 syscall
> 11: 48 89 c7 mov %rax,%rdi
> 14: e8 00 00 00 00 callq 19 <kill+0x19>
> 19: 5a pop %rdx
> 1a: c3 retq
Wow that's some extraordinarily bad codegen going on by gcc... The
sign-extension is semantically needed and I don't see a good way
around it (glibc's asm is kinda a hack taking advantage of kernel not
looking at high bits, I think), but the gratuitous stack adjustment
and refusal to generate a tail call isn't. I'll see if we can track
down what's going on and get it fixed.
> But I hardly think it pays off the required code complexity. Some
> for providing a O(1) bridge: this will require additional complexity
> to write it and setup correctly.
In some sense I agree, but inline instructions are a lot more
expensive on ppc (being 32-bit each), and it might take out-of-lining
anyway to get rid of stack frame setups if that ends up being a
problem.
> >> Specially if 'hwcap & PPC_FEATURE2_SCV' could be optimized with a
> >> TCB member (as we do on glibc) and if we could make the asm clever
> >> enough to not require different clobbers (although not sure if
> >> it would be possible).
> >
> > The easy way not to require different clobbers is just using the union
> > of the clobbers, no? Does the proposed new method clobber any
> > call-saved registers that would make it painful (requiring new call
> > frames to save them in)?
>
> As far I can tell, it should be ok.
Note that because lr is clobbered we need at least once normally
call-clobbered register that's not syscall clobbered to save lr in.
Otherwise stack frame setup is required to spill it. (And I'm not even
sure if gcc does things right to avoid it by using a register -- we
should check that I guess...)
Rich
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Adhemerval Zanella @ 2020-04-16 18:18 UTC (permalink / raw)
To: Rich Felker; +Cc: musl, libc-alpha, linuxppc-dev, Nicholas Piggin, libc-dev
In-Reply-To: <20200416175932.GZ11469@brightrain.aerifal.cx>
On 16/04/2020 14:59, Rich Felker wrote:
> On Thu, Apr 16, 2020 at 02:50:18PM -0300, Adhemerval Zanella wrote:
>>
>>
>> On 16/04/2020 12:37, Rich Felker wrote:
>>> On Thu, Apr 16, 2020 at 11:16:04AM -0300, Adhemerval Zanella wrote:
>>>>> My preference would be that it work just like the i386 AT_SYSINFO
>>>>> where you just replace "int $128" with "call *%%gs:16" and the kernel
>>>>> provides a stub in the vdso that performs either scv or the old
>>>>> mechanism with the same calling convention. Then if the kernel doesn't
>>>>> provide it (because the kernel is too old) libc would have to provide
>>>>> its own stub that uses the legacy method and matches the calling
>>>>> convention of the one the kernel is expected to provide.
>>>>
>>>> What about pthread cancellation and the requirement of checking the
>>>> cancellable syscall anchors in asynchronous cancellation? My plan is
>>>> still to use musl strategy on glibc (BZ#12683) and for i686 it
>>>> requires to always use old int$128 for program that uses cancellation
>>>> (static case) or just threads (dynamic mode, which should be more
>>>> common on glibc).
>>>>
>>>> Using the i686 strategy of a vDSO bridge symbol would require to always
>>>> fallback to 'sc' to still use the same cancellation strategy (and
>>>> thus defeating this optimization in such cases).
>>>
>>> Yes, I assumed it would be the same, ignoring the new syscall
>>> mechanism for cancellable syscalls. While there are some exceptions,
>>> cancellable syscalls are generally not hot paths but things that are
>>> expected to block and to have significant amounts of work to do in
>>> kernelspace, so saving a few tens of cycles is rather pointless.
>>>
>>> It's possible to do a branch/multiple versions of the syscall asm for
>>> cancellation but would require extending the cancellation handler to
>>> support checking against multiple independent address ranges or using
>>> some alternate markup of them.
>>
>> The main issue is at least for glibc dynamic linking is way more common
>> than static linking and once the program become multithread the fallback
>> will be always used.
>
> I'm not relying on static linking optimizing out the cancellable
> version. I'm talking about how cancellable syscalls are pretty much
> all "heavy" operations to begin with where a few tens of cycles are in
> the realm of "measurement noise" relative to the dominating time
> costs.
Yes I am aware, but at same time I am not sure how it plays on real world.
For instance, some workloads might issue kernel query syscalls, such as
recv, where buffer copying might not be dominant factor. So I see that if
the idea is optimizing syscall mechanism, we should try to leverage it
as whole in libc.
>
>> And besides the cancellation performance issue, a new bridge vDSO mechanism
>> will still require to setup some extra bridge for the case of the older
>> kernel. In the scheme you suggested:
>>
>> __asm__("indirect call" ... with common clobbers);
>>
>> The indirect call will be either the vDSO bridge or an libc provided that
>> fallback to 'sc' for !PPC_FEATURE2_SCV. I am not this is really a gain
>> against:
>>
>> if (hwcap & PPC_FEATURE2_SCV) {
>> __asm__(... with some clobbers);
>> } else {
>> __asm__(... with different clobbers);
>> }
>
> If the indirect call can be made roughly as efficiently as the sc
> sequence now (which already have some cost due to handling the nasty
> error return convention, making the indirect call likely just as small
> or smaller), it's O(1) additional code size (and thus icache usage)
> rather than O(n) where n is number of syscall points.
>
> Of course it would work just as well (for avoiding O(n) growth) to
> have a direct call to out-of-line branch like you suggested.
Yes, but does it really matter to optimize this specific usage case
for size? glibc, for instance, tries to leverage the syscall mechanism
by adding some complex pre-processor asm directives. It optimizes
the syscall code size in most cases. For instance, kill in static case
generates on x86_64:
0000000000000000 <__kill>:
0: b8 3e 00 00 00 mov $0x3e,%eax
5: 0f 05 syscall
7: 48 3d 01 f0 ff ff cmp $0xfffffffffffff001,%rax
d: 0f 83 00 00 00 00 jae 13 <__kill+0x13>
13: c3 retq
While on musl:
0000000000000000 <kill>:
0: 48 83 ec 08 sub $0x8,%rsp
4: 48 63 ff movslq %edi,%rdi
7: 48 63 f6 movslq %esi,%rsi
a: b8 3e 00 00 00 mov $0x3e,%eax
f: 0f 05 syscall
11: 48 89 c7 mov %rax,%rdi
14: e8 00 00 00 00 callq 19 <kill+0x19>
19: 5a pop %rdx
1a: c3 retq
But I hardly think it pays off the required code complexity. Some
for providing a O(1) bridge: this will require additional complexity
to write it and setup correctly.
>
>> Specially if 'hwcap & PPC_FEATURE2_SCV' could be optimized with a
>> TCB member (as we do on glibc) and if we could make the asm clever
>> enough to not require different clobbers (although not sure if
>> it would be possible).
>
> The easy way not to require different clobbers is just using the union
> of the clobbers, no? Does the proposed new method clobber any
> call-saved registers that would make it painful (requiring new call
> frames to save them in)?
As far I can tell, it should be ok.
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Florian Weimer @ 2020-04-16 18:12 UTC (permalink / raw)
To: Rich Felker; +Cc: musl, libc-alpha, linuxppc-dev, Nicholas Piggin, libc-dev
In-Reply-To: <20200416165257.GY11469@brightrain.aerifal.cx>
* Rich Felker:
> On Thu, Apr 16, 2020 at 06:42:32PM +0200, Florian Weimer wrote:
>> * Rich Felker:
>>
>> > On Thu, Apr 16, 2020 at 06:48:44AM +0200, Florian Weimer wrote:
>> >> * Rich Felker:
>> >>
>> >> > My preference would be that it work just like the i386 AT_SYSINFO
>> >> > where you just replace "int $128" with "call *%%gs:16" and the kernel
>> >> > provides a stub in the vdso that performs either scv or the old
>> >> > mechanism with the same calling convention.
>> >>
>> >> The i386 mechanism has received some criticism because it provides an
>> >> effective means to redirect execution flow to anyone who can write to
>> >> the TCB. I am not sure if it makes sense to copy it.
>> >
>> > Indeed that's a good point. Do you have ideas for making it equally
>> > efficient without use of a function pointer in the TCB?
>>
>> We could add a shared non-writable mapping at a 64K offset from the
>> thread pointer and store the function pointer or the code there. Then
>> it would be safe.
>>
>> However, since this is apparently tied to POWER9 and we already have a
>> POWER9 multilib, and assuming that we are going to backport the kernel
>> change, I would tweak the selection criterion for that multilib to
>> include the new HWCAP2 flag. If a user runs this glibc on a kernel
>> which does not have support, they will get set baseline (POWER8)
>> multilib, which still works. This way, outside the dynamic loader, no
>> run-time dispatch is needed at all. I guess this is not at all the
>> answer you were looking for. 8-)
>
> How does this work with -static? :-)
-static is not supported. 8-) (If you use the unsupported static
libraries, you get POWER8 code.)
(Just to be clear, in case someone doesn't get the joke: This is about
a potential approach for a heavily constrained, vertically integrated
environment. It does not reflect general glibc recommendations.)
>> If a single binary is needed, I would perhaps follow what Arm did for
>> -moutline-atomics: lay out the code so that its easy to execute for
>> the non-POWER9 case, assuming that POWER9 machines will be better at
>> predicting things than their predecessors.
>>
>> Or you could also put the function pointer into a RELRO segment. Then
>> there's overlap with the __libc_single_threaded discussion, where
>> people objected to this kind of optimization (although I did not
>> propose to change the TCB ABI, that would be required for
>> __libc_single_threaded because it's an external interface).
>
> Of course you can use a normal global, but now every call point needs
> to setup a TOC pointer (= two entry points and more icache lines for
> otherwise trivial functions).
>
> I think my choice would be just making the inline syscall be a single
> call insn to an asm source file that out-of-lines the loading of TOC
> pointer and call through it or branch based on hwcap so that it's not
> repeated all over the place.
I don't know how problematic control flow out of an inline asm is on
POWER. But this is basically the -moutline-atomics approach.
> Alternatively, it would perhaps work to just put hwcap in the TCB and
> branch on it rather than making an indirect call to a function pointer
> in the TCB, so that the worst you could do by clobbering it is execute
> the wrong syscall insn and thereby get SIGILL.
The HWCAP is already in the TCB. I expect this is what generic glibc
builds are going to use (perhaps with a bit of tweaking favorable to
POWER8 implementations, but we'll see).
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-16 17:59 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: musl, libc-alpha, linuxppc-dev, Nicholas Piggin, libc-dev
In-Reply-To: <4b2a7a56-dd2b-1863-50e5-2f4cdbeef47c@linaro.org>
On Thu, Apr 16, 2020 at 02:50:18PM -0300, Adhemerval Zanella wrote:
>
>
> On 16/04/2020 12:37, Rich Felker wrote:
> > On Thu, Apr 16, 2020 at 11:16:04AM -0300, Adhemerval Zanella wrote:
> >>> My preference would be that it work just like the i386 AT_SYSINFO
> >>> where you just replace "int $128" with "call *%%gs:16" and the kernel
> >>> provides a stub in the vdso that performs either scv or the old
> >>> mechanism with the same calling convention. Then if the kernel doesn't
> >>> provide it (because the kernel is too old) libc would have to provide
> >>> its own stub that uses the legacy method and matches the calling
> >>> convention of the one the kernel is expected to provide.
> >>
> >> What about pthread cancellation and the requirement of checking the
> >> cancellable syscall anchors in asynchronous cancellation? My plan is
> >> still to use musl strategy on glibc (BZ#12683) and for i686 it
> >> requires to always use old int$128 for program that uses cancellation
> >> (static case) or just threads (dynamic mode, which should be more
> >> common on glibc).
> >>
> >> Using the i686 strategy of a vDSO bridge symbol would require to always
> >> fallback to 'sc' to still use the same cancellation strategy (and
> >> thus defeating this optimization in such cases).
> >
> > Yes, I assumed it would be the same, ignoring the new syscall
> > mechanism for cancellable syscalls. While there are some exceptions,
> > cancellable syscalls are generally not hot paths but things that are
> > expected to block and to have significant amounts of work to do in
> > kernelspace, so saving a few tens of cycles is rather pointless.
> >
> > It's possible to do a branch/multiple versions of the syscall asm for
> > cancellation but would require extending the cancellation handler to
> > support checking against multiple independent address ranges or using
> > some alternate markup of them.
>
> The main issue is at least for glibc dynamic linking is way more common
> than static linking and once the program become multithread the fallback
> will be always used.
I'm not relying on static linking optimizing out the cancellable
version. I'm talking about how cancellable syscalls are pretty much
all "heavy" operations to begin with where a few tens of cycles are in
the realm of "measurement noise" relative to the dominating time
costs.
> And besides the cancellation performance issue, a new bridge vDSO mechanism
> will still require to setup some extra bridge for the case of the older
> kernel. In the scheme you suggested:
>
> __asm__("indirect call" ... with common clobbers);
>
> The indirect call will be either the vDSO bridge or an libc provided that
> fallback to 'sc' for !PPC_FEATURE2_SCV. I am not this is really a gain
> against:
>
> if (hwcap & PPC_FEATURE2_SCV) {
> __asm__(... with some clobbers);
> } else {
> __asm__(... with different clobbers);
> }
If the indirect call can be made roughly as efficiently as the sc
sequence now (which already have some cost due to handling the nasty
error return convention, making the indirect call likely just as small
or smaller), it's O(1) additional code size (and thus icache usage)
rather than O(n) where n is number of syscall points.
Of course it would work just as well (for avoiding O(n) growth) to
have a direct call to out-of-line branch like you suggested.
> Specially if 'hwcap & PPC_FEATURE2_SCV' could be optimized with a
> TCB member (as we do on glibc) and if we could make the asm clever
> enough to not require different clobbers (although not sure if
> it would be possible).
The easy way not to require different clobbers is just using the union
of the clobbers, no? Does the proposed new method clobber any
call-saved registers that would make it painful (requiring new call
frames to save them in)?
Rich
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Adhemerval Zanella @ 2020-04-16 17:50 UTC (permalink / raw)
To: Rich Felker; +Cc: musl, libc-alpha, linuxppc-dev, Nicholas Piggin, libc-dev
In-Reply-To: <20200416153756.GU11469@brightrain.aerifal.cx>
On 16/04/2020 12:37, Rich Felker wrote:
> On Thu, Apr 16, 2020 at 11:16:04AM -0300, Adhemerval Zanella wrote:
>>> My preference would be that it work just like the i386 AT_SYSINFO
>>> where you just replace "int $128" with "call *%%gs:16" and the kernel
>>> provides a stub in the vdso that performs either scv or the old
>>> mechanism with the same calling convention. Then if the kernel doesn't
>>> provide it (because the kernel is too old) libc would have to provide
>>> its own stub that uses the legacy method and matches the calling
>>> convention of the one the kernel is expected to provide.
>>
>> What about pthread cancellation and the requirement of checking the
>> cancellable syscall anchors in asynchronous cancellation? My plan is
>> still to use musl strategy on glibc (BZ#12683) and for i686 it
>> requires to always use old int$128 for program that uses cancellation
>> (static case) or just threads (dynamic mode, which should be more
>> common on glibc).
>>
>> Using the i686 strategy of a vDSO bridge symbol would require to always
>> fallback to 'sc' to still use the same cancellation strategy (and
>> thus defeating this optimization in such cases).
>
> Yes, I assumed it would be the same, ignoring the new syscall
> mechanism for cancellable syscalls. While there are some exceptions,
> cancellable syscalls are generally not hot paths but things that are
> expected to block and to have significant amounts of work to do in
> kernelspace, so saving a few tens of cycles is rather pointless.
>
> It's possible to do a branch/multiple versions of the syscall asm for
> cancellation but would require extending the cancellation handler to
> support checking against multiple independent address ranges or using
> some alternate markup of them.
The main issue is at least for glibc dynamic linking is way more common
than static linking and once the program become multithread the fallback
will be always used.
And besides the cancellation performance issue, a new bridge vDSO mechanism
will still require to setup some extra bridge for the case of the older
kernel. In the scheme you suggested:
__asm__("indirect call" ... with common clobbers);
The indirect call will be either the vDSO bridge or an libc provided that
fallback to 'sc' for !PPC_FEATURE2_SCV. I am not this is really a gain
against:
if (hwcap & PPC_FEATURE2_SCV) {
__asm__(... with some clobbers);
} else {
__asm__(... with different clobbers);
}
Specially if 'hwcap & PPC_FEATURE2_SCV' could be optimized with a
TCB member (as we do on glibc) and if we could make the asm clever
enough to not require different clobbers (although not sure if
it would be possible).
^ 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