* Re: crash in cpuidle_enter_state with 5.7-rc1
From: Michael Ellerman @ 2020-04-21 12:21 UTC (permalink / raw)
To: Michal Suchánek; +Cc: linuxppc-dev
In-Reply-To: <20200420194132.GM25468@kitsune.suse.cz>
Michal Suchánek <msuchanek@suse.de> writes:
> On Mon, Apr 20, 2020 at 08:50:30AM +0200, Michal Suchánek wrote:
>> On Mon, Apr 20, 2020 at 04:15:39PM +1000, Michael Ellerman wrote:
>> > Michal Suchánek <msuchanek@suse.de> writes:
> ...
>> >
>> >
>> > And I've just hit it with your config on a machine here, but the crash
>> > is different:
>> That does not look like it.
>> You don't have this part in the stack trace:
>> > [ 1.234899] [c000000007597420] [0000000000000000] 0x0
>> > [ 1.234908] [c000000007597720] [0000000000000a6d] 0xa6d
>> > [ 1.234919] [c000000007597a20] [0000000000000000] 0x0
>> > [ 1.234931] [c000000007597d20] [0000000000000004] 0x4
>> which is somewhat random but at least on such line is always present in
>> the traces I get. Also I always get crash in cpuidle_enter_state
> ..
>> > I'm going to guess it's STRICT_KERNEL_RWX that's at fault.
>> I can try without that as well.
>
> Can't reproduce without STRICT_KERNEL_RWX either.
I've reproduced something similar all the way back to v5.5, though it
seems harder to hit - sometimes 5 boots will succeed before one fails.
Are you testing on top of PowerVM or KVM?
cheers
^ permalink raw reply
* Re: crash in cpuidle_enter_state with 5.7-rc1
From: Michal Suchánek @ 2020-04-21 12:37 UTC (permalink / raw)
To: Michael Ellerman; +Cc: linuxppc-dev
In-Reply-To: <87sggxghb3.fsf@mpe.ellerman.id.au>
On Tue, Apr 21, 2020 at 10:21:52PM +1000, Michael Ellerman wrote:
> Michal Suchánek <msuchanek@suse.de> writes:
> > On Mon, Apr 20, 2020 at 08:50:30AM +0200, Michal Suchánek wrote:
> >> On Mon, Apr 20, 2020 at 04:15:39PM +1000, Michael Ellerman wrote:
> >> > Michal Suchánek <msuchanek@suse.de> writes:
> > ...
> >> >
> >> >
> >> > And I've just hit it with your config on a machine here, but the crash
> >> > is different:
> >> That does not look like it.
> >> You don't have this part in the stack trace:
> >> > [ 1.234899] [c000000007597420] [0000000000000000] 0x0
> >> > [ 1.234908] [c000000007597720] [0000000000000a6d] 0xa6d
> >> > [ 1.234919] [c000000007597a20] [0000000000000000] 0x0
> >> > [ 1.234931] [c000000007597d20] [0000000000000004] 0x4
> >> which is somewhat random but at least on such line is always present in
> >> the traces I get. Also I always get crash in cpuidle_enter_state
> > ..
> >> > I'm going to guess it's STRICT_KERNEL_RWX that's at fault.
> >> I can try without that as well.
> >
> > Can't reproduce without STRICT_KERNEL_RWX either.
>
> I've reproduced something similar all the way back to v5.5, though it
> seems harder to hit - sometimes 5 boots will succeed before one fails.
I only tried 3 times because I do not have automation in place to
capture these early crashes. I suppose I could tell the kernel to not
reboot on panic and try rebooting several times.
>
> Are you testing on top of PowerVM or KVM?
PowerVM.
Thanks
Michal
^ permalink raw reply
* Re: [PATCH 1/2] powerpc: Add base support for ISA v3.1
From: Michael Ellerman @ 2020-04-21 12:39 UTC (permalink / raw)
To: Segher Boessenkool, Alistair Popple; +Cc: mikey, linuxppc-dev, npiggin
In-Reply-To: <20200403153208.GI26902@gate.crashing.org>
Segher Boessenkool <segher@kernel.crashing.org> writes:
> Hi!
>
> On Fri, Apr 03, 2020 at 03:10:54PM +1100, Alistair Popple wrote:
>> +#define PCR_ARCH_300 0x10 /* Architecture 3.00 */
>
> It's called 3.0, not 3.00?
It should actually be 3.0B shouldn't it?
cheers
^ permalink raw reply
* Re: [PATCH 1/3] kexec: Prevent removal of memory in use by a loaded kexec image
From: David Hildenbrand @ 2020-04-21 13:29 UTC (permalink / raw)
To: Baoquan He, Andrew Morton
Cc: piliu, Anshuman Khandual, Catalin Marinas, Bhupesh Sharma,
linuxppc-dev, kexec, Russell King - ARM Linux admin, linux-mm,
James Morse, Eric W. Biederman, Will Deacon, linux-arm-kernel
In-Reply-To: <2525cc9c-3566-6275-105b-7f4af8f980bc@redhat.com>
>> ACPI SRAT is embeded into efi, need read out the rsdp pointer. If we don't
>> pass the efi, it won't get the SRAT table correctly, if I remember
>> correctly. Yeah, I remeber kvm guest can get memory hotplugged with
>> ACPI only, this won't happen on bare metal though. Need check carefully.
>> I have been using kvm guest with uefi firmwire recently.
>
> Yeah, I can imagine that bare metal is different. kvm only uses ACPI.
>
> I'm also asking because of virtio-mem. Memory added via virtio-mem is
> not part of any efi tables or whatsoever. So I assume the kexec kernel
> will not detect it automatically (good!), instead load the virtio-mem
> driver and let it add memory back to the system.
>
> I should probably play with kexec and virtio-mem once I have some spare
> cycles ... to find out what's broken and needs to be addressed :)
FWIW, I just gave virtio-mem and kexec/kdump a try.
a) kdump seems to work. Memory added by virtio-mem is getting dumped.
The kexec kernel only uses memory in the crash region. The virtio-mem
driver properly bails out due to is_kdump_kernel().
b) "kexec -s -l" seems to work fine. For now, the kernel does not seem
to get placed on virtio-mem memory (pure luck due to the left-to-right
search). Memory added by virtio-mem is not getting added to the e820
map. Once the virtio-mem driver comes back up in the kexec kernel, the
right memory is readded.
c) "kexec -c -l" does not work properly. All memory added by virtio-mem
is added to the e820 map, which is wrong. Memory that should not be
touched will be touched by the kexec kernel. I assume kexec-tools just
goes ahead and adds anything it can find in /proc/iomem (or
/sys/firmware/memmap/) to the e820 map of the new kernel.
Due to c), I assume all hotplugged memory (e.g., ACPI DIMMs) is
similarly added to the e820 map and, therefore, won't be able to be
onlined MOVABLE easily.
At least for virtio-mem, I would either have to
a) Not support "kexec -c -l". A viable option if we would be planning on
not supporting it either way in the long term. I could block this
in-kernel somehow eventually.
b) Teach kexec-tools to leave virtio-mem added memory alone. E.g., by
indicating it in /proc/iomem in a special way ("System RAM
(hotplugged)"/"System RAM (virtio-mem)").
Baoquan, any opinion on that?
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [PATCH v5 0/5] Track and expose idle PURR and SPURR ticks
From: Nathan Lynch @ 2020-04-21 13:30 UTC (permalink / raw)
To: Gautham R. Shenoy
Cc: Gautham R. Shenoy, Tyrel Datwyler, linux-kernel, Kamalesh Babulal,
Naveen N. Rao, Vaidyanathan Srinivasan, linuxppc-dev
In-Reply-To: <1586249263-14048-1-git-send-email-ego@linux.vnet.ibm.com>
"Gautham R. Shenoy" <ego@linux.vnet.ibm.com> writes:
> This is the fifth version of the patches to track and expose idle PURR
> and SPURR ticks. These patches are required by tools such as lparstat
> to compute system utilization for capacity planning purposes.
>
> The previous versions can be found here:
> v4: https://lkml.org/lkml/2020/3/27/323
> v3: https://lkml.org/lkml/2020/3/11/331
> v2: https://lkml.org/lkml/2020/2/21/21
> v1: https://lore.kernel.org/patchwork/cover/1159341/
>
> They changes from v4 are:
>
> - As suggested by Naveen, moved the functions read_this_idle_purr()
> and read_this_idle_spurr() from Patch 2 and Patch 3 respectively
> to Patch 4 where it is invoked.
>
> - Dropped Patch 6 which cached the values of purr, spurr,
> idle_purr, idle_spurr in order to minimize the number of IPIs
> sent.
>
> - Updated the dates for the idle_purr, idle_spurr in the
> Documentation Patch 5.
LGTM
Acked-by: Nathan Lynch <nathanl@linux.ibm.com>
Thanks.
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Book3S HV: read ibm,secure-memory nodes
From: Michael Ellerman @ 2020-04-21 13:34 UTC (permalink / raw)
To: Laurent Dufour, kvm-ppc, linuxppc-dev
Cc: Alexey Kardashevskiy, paulus, linux-kernel
In-Reply-To: <20200416162715.45846-1-ldufour@linux.ibm.com>
Hi Laurent,
Laurent Dufour <ldufour@linux.ibm.com> writes:
> The newly introduced ibm,secure-memory nodes supersede the
> ibm,uv-firmware's property secure-memory-ranges.
Is either documented in a device tree binding document anywhere?
cheers
> Firmware will no more expose the secure-memory-ranges property so first
> read the new one and if not found rollback to the older one.
>
> Signed-off-by: Laurent Dufour <ldufour@linux.ibm.com>
> ---
> arch/powerpc/kvm/book3s_hv_uvmem.c | 14 ++++++++++++++
> 1 file changed, 14 insertions(+)
>
> diff --git a/arch/powerpc/kvm/book3s_hv_uvmem.c b/arch/powerpc/kvm/book3s_hv_uvmem.c
> index 53b88cae3e73..ad950f8996e0 100644
> --- a/arch/powerpc/kvm/book3s_hv_uvmem.c
> +++ b/arch/powerpc/kvm/book3s_hv_uvmem.c
> @@ -735,6 +735,20 @@ static u64 kvmppc_get_secmem_size(void)
> const __be32 *prop;
> u64 size = 0;
>
> + /*
> + * First try the new ibm,secure-memory nodes which supersede the
> + * secure-memory-ranges property.
> + * If we found somes, no need to read the deprecated one.
> + */
> + for_each_compatible_node(np, NULL, "ibm,secure-memory") {
> + prop = of_get_property(np, "reg", &len);
> + if (!prop)
> + continue;
> + size += of_read_number(prop + 2, 2);
> + }
> + if (size)
> + return size;
> +
> np = of_find_compatible_node(NULL, NULL, "ibm,uv-firmware");
> if (!np)
> goto out;
> --
> 2.26.1
^ permalink raw reply
* Re: [PATCH] KVM: PPC: Book3S HV: read ibm,secure-memory nodes
From: Oliver O'Halloran @ 2020-04-21 13:43 UTC (permalink / raw)
To: Michael Ellerman
Cc: Alexey Kardashevskiy, Linux Kernel Mailing List, kvm-ppc,
Paul Mackerras, Laurent Dufour, linuxppc-dev
In-Reply-To: <87k129gdx8.fsf@mpe.ellerman.id.au>
On Tue, Apr 21, 2020 at 11:37 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
>
> Hi Laurent,
>
> Laurent Dufour <ldufour@linux.ibm.com> writes:
> > The newly introduced ibm,secure-memory nodes supersede the
> > ibm,uv-firmware's property secure-memory-ranges.
>
> Is either documented in a device tree binding document anywhere?
>
> cheers
>
> > Firmware will no more expose the secure-memory-ranges property so first
> > read the new one and if not found rollback to the older one.
There's some in Ryan's UV support series for skiboot:
https://patchwork.ozlabs.org/project/skiboot/patch/20200227204023.22125-2-grimm@linux.ibm.com/
...which is also marked RFC. Cool.
Oliver
^ permalink raw reply
* [PATCH v2 0/7] libfs: group and simplify linux fs code
From: Emanuele Giuseppe Esposito @ 2020-04-21 13:51 UTC (permalink / raw)
To: linux-fsdevel
Cc: Rafael J. Wysocki, David Airlie, dri-devel, Christoph Hellwig,
Andrew Donnellan, Emanuele Giuseppe Esposito, linux-scsi,
James Morris, Serge E. Hallyn, Daniel Vetter, Arnd Bergmann,
James E.J. Bottomley, Maarten Lankhorst, Maxime Ripard,
Manoj N. Kumar, Alexander Viro, Matthew R. Ochs, Uma Krishnan,
John Johansen, Martin K. Petersen, Greg Kroah-Hartman,
linux-kernel, linux-security-module, Thomas Zimmermann,
Frederic Barrat, Paolo Bonzini, linuxppc-dev, Joel Becker
libfs.c has many functions that are useful to implement dentry and inode
operations, but not many at the filesystem level. As a result, code to
create files and inodes has a lot of duplication, to the point that
tracefs has copied several hundred lines from debugfs.
The main two libfs.c functions for filesystems are simple_pin_fs and
simple_release_fs, which hide a somewhat complicated locking sequence
that is needed to serialize vfs_kern_mount and mntget. In this series,
my aim is to add functions that create dentries and inodes of various
kinds (either anonymous inodes, or directory/file/symlink). These
functions take the code that was duplicated across debugfs and tracefs
and move it to libfs.c.
In order to limit the number of arguments to the new functions, the
series first creates a data type that is passed to both
simple_pin_fs/simple_release_fs and the new creation functions. The new
struct, introduced in patch 2, simply groups the "mount" and "count"
arguments to simple_pin_fs and simple_release_fs.
Patches 1-4 are preparations to introduce the new simple_fs struct and
new functions that are useful in the remainder of the series. Patch 5
introduces the dentry and inode creation functions. Patch 6-7 can then
adopt them in debugfs and tracefs.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
v1->v2: rename simple_new_inode in new_inode_current_time,
more detailed explanations, put all common code in fs/libfs.c
Emanuele Giuseppe Esposito (7):
apparmor: just use vfs_kern_mount to make .null
libfs: wrap simple_pin_fs/simple_release_fs arguments in a struct
libfs: introduce new_inode_current_time
libfs: add alloc_anon_inode wrapper
libfs: add file creation functions
debugfs: switch to simplefs inode creation API
tracefs: switch to simplefs inode creation API
drivers/gpu/drm/drm_drv.c | 11 +-
drivers/misc/cxl/api.c | 13 +-
drivers/scsi/cxlflash/ocxl_hw.c | 14 +-
fs/binfmt_misc.c | 9 +-
fs/configfs/mount.c | 10 +-
fs/debugfs/inode.c | 158 +++--------------
fs/libfs.c | 299 ++++++++++++++++++++++++++++++--
fs/tracefs/inode.c | 96 ++--------
include/linux/fs.h | 31 +++-
security/apparmor/apparmorfs.c | 38 ++--
security/inode.c | 11 +-
11 files changed, 399 insertions(+), 291 deletions(-)
--
2.25.2
^ permalink raw reply
* [PATCH v2 1/7] apparmor: just use vfs_kern_mount to make .null
From: Emanuele Giuseppe Esposito @ 2020-04-21 13:51 UTC (permalink / raw)
To: linux-fsdevel
Cc: Rafael J. Wysocki, David Airlie, dri-devel, Christoph Hellwig,
Andrew Donnellan, Emanuele Giuseppe Esposito, linux-scsi,
James Morris, Serge E. Hallyn, Daniel Vetter, Arnd Bergmann,
James E.J. Bottomley, Maarten Lankhorst, Maxime Ripard,
Manoj N. Kumar, Alexander Viro, Matthew R. Ochs, Uma Krishnan,
John Johansen, Martin K. Petersen, Greg Kroah-Hartman,
linux-kernel, linux-security-module, Thomas Zimmermann,
Frederic Barrat, Paolo Bonzini, linuxppc-dev, Joel Becker
In-Reply-To: <20200421135119.30007-1-eesposit@redhat.com>
aa_mk_null_file is using simple_pin_fs/simple_release_fs with local
variables as arguments, for what would amount to a simple
vfs_kern_mount/mntput pair if everything was inlined. Just use
the normal filesystem API since the reference counting is not needed
here (it is a local variable and always 0 on entry and on exit).
There is no functional change intended.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
security/apparmor/apparmorfs.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 280741fc0f5f..36f848734902 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -2525,14 +2525,15 @@ struct path aa_null;
static int aa_mk_null_file(struct dentry *parent)
{
- struct vfsmount *mount = NULL;
+ struct file_system_type *type = parent->d_sb->s_type;
+ struct vfsmount *mount;
struct dentry *dentry;
struct inode *inode;
- int count = 0;
- int error = simple_pin_fs(parent->d_sb->s_type, &mount, &count);
+ int error;
- if (error)
- return error;
+ mount = vfs_kern_mount(type, SB_KERNMOUNT, type->name, NULL);
+ if (IS_ERR(mount))
+ return PTR_ERR(mount);
inode_lock(d_inode(parent));
dentry = lookup_one_len(NULL_FILE_NAME, parent, strlen(NULL_FILE_NAME));
@@ -2561,7 +2562,7 @@ static int aa_mk_null_file(struct dentry *parent)
dput(dentry);
out:
inode_unlock(d_inode(parent));
- simple_release_fs(&mount, &count);
+ mntput(mount);
return error;
}
--
2.25.2
^ permalink raw reply related
* [PATCH v2 2/7] libfs: wrap simple_pin_fs/simple_release_fs arguments in a struct
From: Emanuele Giuseppe Esposito @ 2020-04-21 13:51 UTC (permalink / raw)
To: linux-fsdevel
Cc: Rafael J. Wysocki, David Airlie, dri-devel, Christoph Hellwig,
Andrew Donnellan, Emanuele Giuseppe Esposito, linux-scsi,
James Morris, Serge E. Hallyn, Daniel Vetter, Arnd Bergmann,
James E.J. Bottomley, Maarten Lankhorst, Maxime Ripard,
Manoj N. Kumar, Alexander Viro, Matthew R. Ochs, Uma Krishnan,
John Johansen, Martin K. Petersen, Greg Kroah-Hartman,
linux-kernel, linux-security-module, Thomas Zimmermann,
Frederic Barrat, Paolo Bonzini, linuxppc-dev, Joel Becker
In-Reply-To: <20200421135119.30007-1-eesposit@redhat.com>
Simplify passing the count and mount to simple_pin_fs and
simple_release_fs by wrapping them in the simple_fs struct,
in preparation for adding more high level operations to
fs/libfs.c
There is no functional change intended.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
drivers/gpu/drm/drm_drv.c | 11 ++++----
drivers/misc/cxl/api.c | 13 +++++-----
drivers/scsi/cxlflash/ocxl_hw.c | 14 +++++-----
fs/binfmt_misc.c | 9 +++----
fs/configfs/mount.c | 10 +++-----
fs/debugfs/inode.c | 22 ++++++++--------
fs/libfs.c | 45 +++++++++++++++++++++++++--------
fs/tracefs/inode.c | 18 ++++++-------
include/linux/fs.h | 10 ++++++--
security/apparmor/apparmorfs.c | 25 +++++++++---------
security/inode.c | 11 ++++----
11 files changed, 103 insertions(+), 85 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 7b1a628d1f6e..e29424d64874 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -514,8 +514,7 @@ EXPORT_SYMBOL(drm_dev_unplug);
* iput(), but this way you'd end up with a new vfsmount for each inode.
*/
-static int drm_fs_cnt;
-static struct vfsmount *drm_fs_mnt;
+static struct simple_fs drm_fs;
static int drm_fs_init_fs_context(struct fs_context *fc)
{
@@ -534,15 +533,15 @@ static struct inode *drm_fs_inode_new(void)
struct inode *inode;
int r;
- r = simple_pin_fs(&drm_fs_type, &drm_fs_mnt, &drm_fs_cnt);
+ r = simple_pin_fs(&drm_fs, &drm_fs_type);
if (r < 0) {
DRM_ERROR("Cannot mount pseudo fs: %d\n", r);
return ERR_PTR(r);
}
- inode = alloc_anon_inode(drm_fs_mnt->mnt_sb);
+ inode = alloc_anon_inode(drm_fs.mount->mnt_sb);
if (IS_ERR(inode))
- simple_release_fs(&drm_fs_mnt, &drm_fs_cnt);
+ simple_release_fs(&drm_fs);
return inode;
}
@@ -551,7 +550,7 @@ static void drm_fs_inode_free(struct inode *inode)
{
if (inode) {
iput(inode);
- simple_release_fs(&drm_fs_mnt, &drm_fs_cnt);
+ simple_release_fs(&drm_fs);
}
}
diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
index b493de962153..67e4808bce49 100644
--- a/drivers/misc/cxl/api.c
+++ b/drivers/misc/cxl/api.c
@@ -31,8 +31,7 @@
#define CXL_PSEUDO_FS_MAGIC 0x1697697f
-static int cxl_fs_cnt;
-static struct vfsmount *cxl_vfs_mount;
+static struct simple_fs cxl_fs;
static int cxl_fs_init_fs_context(struct fs_context *fc)
{
@@ -50,7 +49,7 @@ static struct file_system_type cxl_fs_type = {
void cxl_release_mapping(struct cxl_context *ctx)
{
if (ctx->kernelapi && ctx->mapping)
- simple_release_fs(&cxl_vfs_mount, &cxl_fs_cnt);
+ simple_release_fs(&cxl_fs);
}
static struct file *cxl_getfile(const char *name,
@@ -66,20 +65,20 @@ static struct file *cxl_getfile(const char *name,
if (fops->owner && !try_module_get(fops->owner))
return ERR_PTR(-ENOENT);
- rc = simple_pin_fs(&cxl_fs_type, &cxl_vfs_mount, &cxl_fs_cnt);
+ rc = simple_pin_fs(&cxl_fs, &cxl_fs_type);
if (rc < 0) {
pr_err("Cannot mount cxl pseudo filesystem: %d\n", rc);
file = ERR_PTR(rc);
goto err_module;
}
- inode = alloc_anon_inode(cxl_vfs_mount->mnt_sb);
+ inode = alloc_anon_inode(cxl_fs.mount->mnt_sb);
if (IS_ERR(inode)) {
file = ERR_CAST(inode);
goto err_fs;
}
- file = alloc_file_pseudo(inode, cxl_vfs_mount, name,
+ file = alloc_file_pseudo(inode, cxl_fs.mount, name,
flags & (O_ACCMODE | O_NONBLOCK), fops);
if (IS_ERR(file))
goto err_inode;
@@ -91,7 +90,7 @@ static struct file *cxl_getfile(const char *name,
err_inode:
iput(inode);
err_fs:
- simple_release_fs(&cxl_vfs_mount, &cxl_fs_cnt);
+ simple_release_fs(&cxl_fs);
err_module:
module_put(fops->owner);
return file;
diff --git a/drivers/scsi/cxlflash/ocxl_hw.c b/drivers/scsi/cxlflash/ocxl_hw.c
index 7018cd802569..7fa98dd4fa28 100644
--- a/drivers/scsi/cxlflash/ocxl_hw.c
+++ b/drivers/scsi/cxlflash/ocxl_hw.c
@@ -29,8 +29,7 @@
#define OCXLFLASH_FS_MAGIC 0x1697698f
-static int ocxlflash_fs_cnt;
-static struct vfsmount *ocxlflash_vfs_mount;
+static struct simple_fs ocxlflash_fs;
static int ocxlflash_fs_init_fs_context(struct fs_context *fc)
{
@@ -51,7 +50,7 @@ static struct file_system_type ocxlflash_fs_type = {
static void ocxlflash_release_mapping(struct ocxlflash_context *ctx)
{
if (ctx->mapping)
- simple_release_fs(&ocxlflash_vfs_mount, &ocxlflash_fs_cnt);
+ simple_release_fs(&ocxlflash_fs);
ctx->mapping = NULL;
}
@@ -79,15 +78,14 @@ static struct file *ocxlflash_getfile(struct device *dev, const char *name,
goto err1;
}
- rc = simple_pin_fs(&ocxlflash_fs_type, &ocxlflash_vfs_mount,
- &ocxlflash_fs_cnt);
+ rc = simple_pin_fs(&ocxlflash_fs, &ocxlflash_fs_type);
if (unlikely(rc < 0)) {
dev_err(dev, "%s: Cannot mount ocxlflash pseudofs rc=%d\n",
__func__, rc);
goto err2;
}
- inode = alloc_anon_inode(ocxlflash_vfs_mount->mnt_sb);
+ inode = alloc_anon_inode(ocxlflash_fs.mount->mnt_sb);
if (IS_ERR(inode)) {
rc = PTR_ERR(inode);
dev_err(dev, "%s: alloc_anon_inode failed rc=%d\n",
@@ -95,7 +93,7 @@ static struct file *ocxlflash_getfile(struct device *dev, const char *name,
goto err3;
}
- file = alloc_file_pseudo(inode, ocxlflash_vfs_mount, name,
+ file = alloc_file_pseudo(inode, ocxlflash_fs.mount, name,
flags & (O_ACCMODE | O_NONBLOCK), fops);
if (IS_ERR(file)) {
rc = PTR_ERR(file);
@@ -110,7 +108,7 @@ static struct file *ocxlflash_getfile(struct device *dev, const char *name,
err4:
iput(inode);
err3:
- simple_release_fs(&ocxlflash_vfs_mount, &ocxlflash_fs_cnt);
+ simple_release_fs(&ocxlflash_fs);
err2:
module_put(fops->owner);
err1:
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index cdb45829354d..3cff446f222b 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -64,8 +64,7 @@ typedef struct {
static DEFINE_RWLOCK(entries_lock);
static struct file_system_type bm_fs_type;
-static struct vfsmount *bm_mnt;
-static int entry_count;
+static struct simple_fs bm_fs;
/*
* Max length of the register string. Determined by:
@@ -623,7 +622,7 @@ static void kill_node(Node *e)
drop_nlink(d_inode(dentry));
d_drop(dentry);
dput(dentry);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&bm_fs);
}
/* /<entry> */
@@ -718,7 +717,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (!inode)
goto out2;
- err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
+ err = simple_pin_fs(&bm_fs, &bm_fs_type);
if (err) {
iput(inode);
inode = NULL;
@@ -732,7 +731,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
if (IS_ERR(f)) {
err = PTR_ERR(f);
pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
- simple_release_fs(&bm_mnt, &entry_count);
+ simple_release_fs(&bm_fs);
iput(inode);
inode = NULL;
goto out2;
diff --git a/fs/configfs/mount.c b/fs/configfs/mount.c
index 0c6e8cf61953..9fb2791e5eed 100644
--- a/fs/configfs/mount.c
+++ b/fs/configfs/mount.c
@@ -24,9 +24,8 @@
/* Random magic number */
#define CONFIGFS_MAGIC 0x62656570
-static struct vfsmount *configfs_mount = NULL;
+static struct simple_fs configfs_fs;
struct kmem_cache *configfs_dir_cachep;
-static int configfs_mnt_count = 0;
static void configfs_free_inode(struct inode *inode)
@@ -123,14 +122,13 @@ MODULE_ALIAS_FS("configfs");
struct dentry *configfs_pin_fs(void)
{
- int err = simple_pin_fs(&configfs_fs_type, &configfs_mount,
- &configfs_mnt_count);
- return err ? ERR_PTR(err) : configfs_mount->mnt_root;
+ int err = simple_pin_fs(&configfs_fs, &configfs_fs_type);
+ return err ? ERR_PTR(err) : configfs_fs.mount->mnt_root;
}
void configfs_release_fs(void)
{
- simple_release_fs(&configfs_mount, &configfs_mnt_count);
+ simple_release_fs(&configfs_fs);
}
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index b7f2e971ecbc..5dbb74a23e7c 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -32,8 +32,7 @@
#define DEBUGFS_DEFAULT_MODE 0700
-static struct vfsmount *debugfs_mount;
-static int debugfs_mount_count;
+static struct simple_fs debugfs;
static bool debugfs_registered;
/*
@@ -297,7 +296,7 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
return NULL;
if (!parent)
- parent = debugfs_mount->mnt_root;
+ parent = debugfs.mount->mnt_root;
dentry = lookup_positive_unlocked(name, parent, strlen(name));
if (IS_ERR(dentry))
@@ -316,8 +315,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
if (IS_ERR(parent))
return parent;
- error = simple_pin_fs(&debug_fs_type, &debugfs_mount,
- &debugfs_mount_count);
+ error = simple_pin_fs(&debugfs, &debug_fs_type);
if (error) {
pr_err("Unable to pin filesystem for file '%s'\n", name);
return ERR_PTR(error);
@@ -329,7 +327,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
* have around.
*/
if (!parent)
- parent = debugfs_mount->mnt_root;
+ parent = debugfs.mount->mnt_root;
inode_lock(d_inode(parent));
if (unlikely(IS_DEADDIR(d_inode(parent))))
@@ -349,7 +347,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
if (IS_ERR(dentry)) {
inode_unlock(d_inode(parent));
- simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+ simple_release_fs(&debugfs);
}
return dentry;
@@ -359,7 +357,7 @@ static struct dentry *failed_creating(struct dentry *dentry)
{
inode_unlock(d_inode(dentry->d_parent));
dput(dentry);
- simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+ simple_release_fs(&debugfs);
return ERR_PTR(-ENOMEM);
}
@@ -676,9 +674,9 @@ static void __debugfs_file_removed(struct dentry *dentry)
static void remove_one(struct dentry *victim)
{
- if (d_is_reg(victim))
+ if (d_is_reg(victim))
__debugfs_file_removed(victim);
- simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+ simple_release_fs(&debugfs);
}
/**
@@ -699,9 +697,9 @@ void debugfs_remove(struct dentry *dentry)
if (IS_ERR_OR_NULL(dentry))
return;
- simple_pin_fs(&debug_fs_type, &debugfs_mount, &debugfs_mount_count);
+ simple_pin_fs(&debugfs, &debug_fs_type);
simple_recursive_removal(dentry, remove_one);
- simple_release_fs(&debugfs_mount, &debugfs_mount_count);
+ simple_release_fs(&debugfs);
}
EXPORT_SYMBOL_GPL(debugfs_remove);
diff --git a/fs/libfs.c b/fs/libfs.c
index 3759fbacf522..54e07ae986ca 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -665,39 +665,64 @@ EXPORT_SYMBOL(simple_fill_super);
static DEFINE_SPINLOCK(pin_fs_lock);
-int simple_pin_fs(struct file_system_type *type, struct vfsmount **mount, int *count)
+/**
+ * simple_pin_fs - generic function to pin (mount if needed,
+ * otherwise add a reference to the mount) a filesystem
+ * @fs: a pointer to a the simple_fs struct containing a struct vfs_mount
+ * pointer (that can be NULL) and a counter.
+ * @type: a pointer to the file system type used by vfs_kern_mount.
+ *
+ * This function sets fs->mount if NULL, by calling vfs_kern_mount
+ * on @type.
+ * It also takes care of incrementing the reference counter.
+ *
+ * This function will return 0 in case of success, and PTR_ERR(-ERROR)
+ * if vfs_kern_mount fails.
+ **/
+int simple_pin_fs(struct simple_fs *fs, struct file_system_type *type)
{
struct vfsmount *mnt = NULL;
spin_lock(&pin_fs_lock);
- if (unlikely(!*mount)) {
+ if (unlikely(!fs->mount)) {
spin_unlock(&pin_fs_lock);
mnt = vfs_kern_mount(type, SB_KERNMOUNT, type->name, NULL);
if (IS_ERR(mnt))
return PTR_ERR(mnt);
spin_lock(&pin_fs_lock);
- if (!*mount)
- *mount = mnt;
+ if (!fs->mount)
+ fs->mount = mnt;
}
- mntget(*mount);
- ++*count;
+ mntget(fs->mount);
+ ++fs->count;
spin_unlock(&pin_fs_lock);
mntput(mnt);
return 0;
}
EXPORT_SYMBOL(simple_pin_fs);
-void simple_release_fs(struct vfsmount **mount, int *count)
+/**
+ * simple_release_fs - decrements the reference counter and unmounts the
+ * file system.
+ * @fs: a pointer to a struct simple_fs containing the reference counter
+ * and vfs_mount pointer
+ *
+ * This function decrements the refcount of the given file system and
+ * if 0 sets the mount pointer to NULL.
+ **/
+void simple_release_fs(struct simple_fs *fs)
{
struct vfsmount *mnt;
spin_lock(&pin_fs_lock);
- mnt = *mount;
- if (!--*count)
- *mount = NULL;
+ mnt = fs->mount;
+ if (!--fs->count)
+ fs->mount = NULL;
spin_unlock(&pin_fs_lock);
mntput(mnt);
}
EXPORT_SYMBOL(simple_release_fs);
+
+
/**
* simple_read_from_buffer - copy data from the buffer to user space
* @to: the user space buffer to read to
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 0ee8c6dfb036..370eb38ff1ad 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -24,8 +24,7 @@
#define TRACEFS_DEFAULT_MODE 0700
-static struct vfsmount *tracefs_mount;
-static int tracefs_mount_count;
+static struct simple_fs tracefs;
static bool tracefs_registered;
static ssize_t default_read_file(struct file *file, char __user *buf,
@@ -316,8 +315,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
pr_debug("tracefs: creating file '%s'\n",name);
- error = simple_pin_fs(&trace_fs_type, &tracefs_mount,
- &tracefs_mount_count);
+ error = simple_pin_fs(&tracefs, &trace_fs_type);
if (error)
return ERR_PTR(error);
@@ -327,7 +325,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
* have around.
*/
if (!parent)
- parent = tracefs_mount->mnt_root;
+ parent = tracefs.mount->mnt_root;
inode_lock(parent->d_inode);
if (unlikely(IS_DEADDIR(parent->d_inode)))
@@ -341,7 +339,7 @@ static struct dentry *start_creating(const char *name, struct dentry *parent)
if (IS_ERR(dentry)) {
inode_unlock(parent->d_inode);
- simple_release_fs(&tracefs_mount, &tracefs_mount_count);
+ simple_release_fs(&tracefs);
}
return dentry;
@@ -351,7 +349,7 @@ static struct dentry *failed_creating(struct dentry *dentry)
{
inode_unlock(dentry->d_parent->d_inode);
dput(dentry);
- simple_release_fs(&tracefs_mount, &tracefs_mount_count);
+ simple_release_fs(&tracefs);
return NULL;
}
@@ -504,7 +502,7 @@ __init struct dentry *tracefs_create_instance_dir(const char *name,
static void remove_one(struct dentry *victim)
{
- simple_release_fs(&tracefs_mount, &tracefs_mount_count);
+ simple_release_fs(&tracefs);
}
/**
@@ -520,9 +518,9 @@ void tracefs_remove(struct dentry *dentry)
if (IS_ERR_OR_NULL(dentry))
return;
- simple_pin_fs(&trace_fs_type, &tracefs_mount, &tracefs_mount_count);
+ simple_pin_fs(&tracefs, &trace_fs_type);
simple_recursive_removal(dentry, remove_one);
- simple_release_fs(&tracefs_mount, &tracefs_mount_count);
+ simple_release_fs(&tracefs);
}
/**
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 4f6f59b4f22a..a3691c132b3a 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3363,8 +3363,14 @@ struct tree_descr { const char *name; const struct file_operations *ops; int mod
struct dentry *d_alloc_name(struct dentry *, const char *);
extern int simple_fill_super(struct super_block *, unsigned long,
const struct tree_descr *);
-extern int simple_pin_fs(struct file_system_type *, struct vfsmount **mount, int *count);
-extern void simple_release_fs(struct vfsmount **mount, int *count);
+
+struct simple_fs {
+ struct vfsmount *mount;
+ int count;
+};
+
+extern int simple_pin_fs(struct simple_fs *, struct file_system_type *);
+extern void simple_release_fs(struct simple_fs *);
extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
loff_t *ppos, const void *from, size_t available);
diff --git a/security/apparmor/apparmorfs.c b/security/apparmor/apparmorfs.c
index 36f848734902..00f0158fb1e1 100644
--- a/security/apparmor/apparmorfs.c
+++ b/security/apparmor/apparmorfs.c
@@ -140,8 +140,7 @@ static int mangle_name(const char *name, char *target)
*/
#define AAFS_NAME "apparmorfs"
-static struct vfsmount *aafs_mnt;
-static int aafs_count;
+static struct simple_fs aafs;
static int aafs_show_path(struct seq_file *seq, struct dentry *dentry)
@@ -273,7 +272,7 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
if (!(mode & S_IFMT))
mode = (mode & S_IALLUGO) | S_IFREG;
- error = simple_pin_fs(&aafs_ops, &aafs_mnt, &aafs_count);
+ error = simple_pin_fs(&aafs, &aafs_ops);
if (error)
return ERR_PTR(error);
@@ -303,7 +302,7 @@ static struct dentry *aafs_create(const char *name, umode_t mode,
fail_lock:
inode_unlock(dir);
- simple_release_fs(&aafs_mnt, &aafs_count);
+ simple_release_fs(&aafs);
return ERR_PTR(error);
}
@@ -395,7 +394,7 @@ static void aafs_remove(struct dentry *dentry)
dput(dentry);
}
inode_unlock(dir);
- simple_release_fs(&aafs_mnt, &aafs_count);
+ simple_release_fs(&aafs);
}
@@ -1824,7 +1823,7 @@ static int ns_mkdir_op(struct inode *dir, struct dentry *dentry, umode_t mode)
* for pin_fs
*/
inode_unlock(dir);
- error = simple_pin_fs(&aafs_ops, &aafs_mnt, &aafs_count);
+ error = simple_pin_fs(&aafs, &aafs_ops);
mutex_lock_nested(&parent->lock, parent->level);
inode_lock_nested(dir, I_MUTEX_PARENT);
if (error)
@@ -1845,7 +1844,7 @@ static int ns_mkdir_op(struct inode *dir, struct dentry *dentry, umode_t mode)
aa_put_ns(ns); /* list ref remains */
out_pin:
if (error)
- simple_release_fs(&aafs_mnt, &aafs_count);
+ simple_release_fs(&aafs);
out:
mutex_unlock(&parent->lock);
aa_put_ns(parent);
@@ -2580,7 +2579,7 @@ static const char *policy_get_link(struct dentry *dentry,
return ERR_PTR(-ECHILD);
ns = aa_get_current_ns();
- path.mnt = mntget(aafs_mnt);
+ path.mnt = mntget(aafs.mount);
path.dentry = dget(ns_dir(ns));
error = nd_jump_link(&path);
aa_put_ns(ns);
@@ -2631,10 +2630,10 @@ static int __init aa_create_aafs(void)
}
/* setup apparmorfs used to virtualize policy/ */
- aafs_mnt = kern_mount(&aafs_ops);
- if (IS_ERR(aafs_mnt))
+ aafs.mount = kern_mount(&aafs_ops);
+ if (IS_ERR(aafs.mount))
panic("can't set apparmorfs up\n");
- aafs_mnt->mnt_sb->s_flags &= ~SB_NOUSER;
+ aafs.mount->mnt_sb->s_flags &= ~SB_NOUSER;
/* Populate fs tree. */
error = entry_create_dir(&aa_sfs_entry, NULL);
@@ -2667,8 +2666,8 @@ static int __init aa_create_aafs(void)
/* policy tree referenced by magic policy symlink */
mutex_lock_nested(&root_ns->lock, root_ns->level);
- error = __aafs_ns_mkdir(root_ns, aafs_mnt->mnt_root, ".policy",
- aafs_mnt->mnt_root);
+ error = __aafs_ns_mkdir(root_ns, aafs.mount->mnt_root, ".policy",
+ aafs.mount->mnt_root);
mutex_unlock(&root_ns->lock);
if (error)
goto error;
diff --git a/security/inode.c b/security/inode.c
index 6c326939750d..8a1bee35470a 100644
--- a/security/inode.c
+++ b/security/inode.c
@@ -22,8 +22,7 @@
#include <linux/lsm_hooks.h>
#include <linux/magic.h>
-static struct vfsmount *mount;
-static int mount_count;
+static struct simple_fs securityfs;
static void securityfs_free_inode(struct inode *inode)
{
@@ -118,12 +117,12 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
pr_debug("securityfs: creating file '%s'\n",name);
- error = simple_pin_fs(&fs_type, &mount, &mount_count);
+ error = simple_pin_fs(&securityfs, &fs_type);
if (error)
return ERR_PTR(error);
if (!parent)
- parent = mount->mnt_root;
+ parent = securityfs.mount->mnt_root;
dir = d_inode(parent);
@@ -168,7 +167,7 @@ static struct dentry *securityfs_create_dentry(const char *name, umode_t mode,
dentry = ERR_PTR(error);
out:
inode_unlock(dir);
- simple_release_fs(&mount, &mount_count);
+ simple_release_fs(&securityfs);
return dentry;
}
@@ -309,7 +308,7 @@ void securityfs_remove(struct dentry *dentry)
dput(dentry);
}
inode_unlock(dir);
- simple_release_fs(&mount, &mount_count);
+ simple_release_fs(&securityfs);
}
EXPORT_SYMBOL_GPL(securityfs_remove);
--
2.25.2
^ permalink raw reply related
* Re: [PATCH v2] sched/core: fix illegal RCU from offline CPUs
From: Peter Zijlstra @ 2020-04-21 13:56 UTC (permalink / raw)
To: Qian Cai
Cc: juri.lelli, James.Bottomley@hansenpartnership.com,
vincent.guittot, linux-parisc, paulmck, deller, Nicholas Piggin,
linux-kernel, Steven Rostedt, bsegall, linux-mm, Ingo Molnar,
mgorman, tglx, linuxppc-dev, dietmar.eggemann
In-Reply-To: <BBA124FA-7924-4782-AC9D-7B1B98BE817F@lca.pw>
On Fri, Apr 17, 2020 at 09:26:56AM -0400, Qian Cai wrote:
> > Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
>
> Peter, can you take a look at this patch when you have a chance?
Sorry, -ETOOMUCHEMAIL, got it now, thanks!
^ permalink raw reply
* Re: [PATCH 1/3] kexec: Prevent removal of memory in use by a loaded kexec image
From: David Hildenbrand @ 2020-04-21 13:57 UTC (permalink / raw)
To: Baoquan He, Andrew Morton
Cc: piliu, Anshuman Khandual, Catalin Marinas, Bhupesh Sharma,
linuxppc-dev, kexec, Russell King - ARM Linux admin, linux-mm,
James Morse, Eric W. Biederman, Will Deacon, linux-arm-kernel
In-Reply-To: <9a4eb1d7-33bf-8707-9c0c-1ca657c3e502@redhat.com>
On 21.04.20 15:29, David Hildenbrand wrote:
>>> ACPI SRAT is embeded into efi, need read out the rsdp pointer. If we don't
>>> pass the efi, it won't get the SRAT table correctly, if I remember
>>> correctly. Yeah, I remeber kvm guest can get memory hotplugged with
>>> ACPI only, this won't happen on bare metal though. Need check carefully.
>>> I have been using kvm guest with uefi firmwire recently.
>>
>> Yeah, I can imagine that bare metal is different. kvm only uses ACPI.
>>
>> I'm also asking because of virtio-mem. Memory added via virtio-mem is
>> not part of any efi tables or whatsoever. So I assume the kexec kernel
>> will not detect it automatically (good!), instead load the virtio-mem
>> driver and let it add memory back to the system.
>>
>> I should probably play with kexec and virtio-mem once I have some spare
>> cycles ... to find out what's broken and needs to be addressed :)
>
> FWIW, I just gave virtio-mem and kexec/kdump a try.
>
> a) kdump seems to work. Memory added by virtio-mem is getting dumped.
> The kexec kernel only uses memory in the crash region. The virtio-mem
> driver properly bails out due to is_kdump_kernel().
>
> b) "kexec -s -l" seems to work fine. For now, the kernel does not seem
> to get placed on virtio-mem memory (pure luck due to the left-to-right
> search). Memory added by virtio-mem is not getting added to the e820
> map. Once the virtio-mem driver comes back up in the kexec kernel, the
> right memory is readded.
>
> c) "kexec -c -l" does not work properly. All memory added by virtio-mem
> is added to the e820 map, which is wrong. Memory that should not be
> touched will be touched by the kexec kernel. I assume kexec-tools just
> goes ahead and adds anything it can find in /proc/iomem (or
> /sys/firmware/memmap/) to the e820 map of the new kernel.
>
> Due to c), I assume all hotplugged memory (e.g., ACPI DIMMs) is
> similarly added to the e820 map and, therefore, won't be able to be
> onlined MOVABLE easily.
>
>
> At least for virtio-mem, I would either have to
> a) Not support "kexec -c -l". A viable option if we would be planning on
> not supporting it either way in the long term. I could block this
> in-kernel somehow eventually.
>
> b) Teach kexec-tools to leave virtio-mem added memory alone. E.g., by
> indicating it in /proc/iomem in a special way ("System RAM
> (hotplugged)"/"System RAM (virtio-mem)").
I just realized, that *not* creating /sys/firmware/memmap/ entries for
virtio-mem memory seems to be the right thing to do.
--
Thanks,
David / dhildenb
^ permalink raw reply
* [PATCH v2 3/7] libfs: introduce new_inode_current_time
From: Emanuele Giuseppe Esposito @ 2020-04-21 13:57 UTC (permalink / raw)
To: linux-fsdevel
Cc: Rafael J. Wysocki, David Airlie, dri-devel, Christoph Hellwig,
Andrew Donnellan, Emanuele Giuseppe Esposito, linux-scsi,
James Morris, Serge E. Hallyn, Daniel Vetter, Arnd Bergmann,
James E.J. Bottomley, Maarten Lankhorst, Maxime Ripard,
Manoj N. Kumar, Alexander Viro, Matthew R. Ochs, Uma Krishnan,
John Johansen, Martin K. Petersen, Greg Kroah-Hartman,
linux-kernel, linux-security-module, Thomas Zimmermann,
Frederic Barrat, Paolo Bonzini, linuxppc-dev, Joel Becker
In-Reply-To: <20200421135119.30007-1-eesposit@redhat.com>
It is a common special case for new_inode to initialize the
time to the current time and the inode to get_next_ino().
Introduce a core function that does it.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
fs/libfs.c | 20 ++++++++++++++++++++
include/linux/fs.h | 1 +
2 files changed, 21 insertions(+)
diff --git a/fs/libfs.c b/fs/libfs.c
index 54e07ae986ca..3fa0cd27ab06 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -594,6 +594,26 @@ int simple_write_end(struct file *file, struct address_space *mapping,
}
EXPORT_SYMBOL(simple_write_end);
+/**
+ * new_inode_current_time - create new inode by initializing the
+ * time to the current time and the inode to get_next_ino()
+ * @sb: pointer to super block of the file system
+ *
+ * Returns an inode pointer on success, NULL on failure.
+ */
+struct inode *new_inode_current_time(struct super_block *sb)
+{
+ struct inode *inode = new_inode(sb);
+
+ if (inode) {
+ inode->i_ino = get_next_ino();
+ inode->i_atime = inode->i_mtime =
+ inode->i_ctime = current_time(inode);
+ }
+ return inode;
+}
+EXPORT_SYMBOL(new_inode_current_time);
+
/*
* the inodes created here are not hashed. If you use iunique to generate
* unique inode values later for this filesystem, then you must take care
diff --git a/include/linux/fs.h b/include/linux/fs.h
index a3691c132b3a..de2577df30ae 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3088,6 +3088,7 @@ extern void clear_inode(struct inode *);
extern void __destroy_inode(struct inode *);
extern struct inode *new_inode_pseudo(struct super_block *sb);
extern struct inode *new_inode(struct super_block *sb);
+extern struct inode *new_inode_current_time(struct super_block *sb);
extern void free_inode_nonrcu(struct inode *inode);
extern int should_remove_suid(struct dentry *);
extern int file_remove_privs(struct file *);
--
2.25.2
^ permalink raw reply related
* [PATCH v2 4/7] libfs: add alloc_anon_inode wrapper
From: Emanuele Giuseppe Esposito @ 2020-04-21 13:57 UTC (permalink / raw)
To: linux-fsdevel
Cc: Rafael J. Wysocki, David Airlie, dri-devel, Christoph Hellwig,
Andrew Donnellan, Emanuele Giuseppe Esposito, linux-scsi,
James Morris, Serge E. Hallyn, Daniel Vetter, Arnd Bergmann,
James E.J. Bottomley, Maarten Lankhorst, Maxime Ripard,
Manoj N. Kumar, Alexander Viro, Matthew R. Ochs, Uma Krishnan,
John Johansen, Martin K. Petersen, Greg Kroah-Hartman,
linux-kernel, linux-security-module, Thomas Zimmermann,
Frederic Barrat, Paolo Bonzini, linuxppc-dev, Joel Becker
In-Reply-To: <20200421135119.30007-1-eesposit@redhat.com>
libfs.c has many functions that are useful to implement dentry and inode
operations, but not many at the filesystem level. Start adding file
creation wrappers, the simplest returns an anonymous inode.
There is no functional change intended.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
drivers/gpu/drm/drm_drv.c | 2 +-
drivers/misc/cxl/api.c | 2 +-
drivers/scsi/cxlflash/ocxl_hw.c | 2 +-
fs/libfs.c | 10 +++++++++-
include/linux/fs.h | 2 ++
5 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index e29424d64874..1854f760ad39 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -539,7 +539,7 @@ static struct inode *drm_fs_inode_new(void)
return ERR_PTR(r);
}
- inode = alloc_anon_inode(drm_fs.mount->mnt_sb);
+ inode = simple_alloc_anon_inode(&drm_fs);
if (IS_ERR(inode))
simple_release_fs(&drm_fs);
diff --git a/drivers/misc/cxl/api.c b/drivers/misc/cxl/api.c
index 67e4808bce49..57672abb6223 100644
--- a/drivers/misc/cxl/api.c
+++ b/drivers/misc/cxl/api.c
@@ -72,7 +72,7 @@ static struct file *cxl_getfile(const char *name,
goto err_module;
}
- inode = alloc_anon_inode(cxl_fs.mount->mnt_sb);
+ inode = simple_alloc_anon_inode(&cxl_fs);
if (IS_ERR(inode)) {
file = ERR_CAST(inode);
goto err_fs;
diff --git a/drivers/scsi/cxlflash/ocxl_hw.c b/drivers/scsi/cxlflash/ocxl_hw.c
index 7fa98dd4fa28..0e9f2ae7eebf 100644
--- a/drivers/scsi/cxlflash/ocxl_hw.c
+++ b/drivers/scsi/cxlflash/ocxl_hw.c
@@ -85,7 +85,7 @@ static struct file *ocxlflash_getfile(struct device *dev, const char *name,
goto err2;
}
- inode = alloc_anon_inode(ocxlflash_fs.mount->mnt_sb);
+ inode = simple_alloc_anon_inode(&ocxlflash_fs);
if (IS_ERR(inode)) {
rc = PTR_ERR(inode);
dev_err(dev, "%s: alloc_anon_inode failed rc=%d\n",
diff --git a/fs/libfs.c b/fs/libfs.c
index 3fa0cd27ab06..5c76e4c648dc 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -741,7 +741,15 @@ void simple_release_fs(struct simple_fs *fs)
}
EXPORT_SYMBOL(simple_release_fs);
-
+/**
+ * simple_alloc_anon_inode - wrapper for alloc_anon_inode
+ * @fs: a pointer to a struct simple_fs containing a valid vfs_mount pointer
+ **/
+struct inode *simple_alloc_anon_inode(struct simple_fs *fs)
+{
+ return alloc_anon_inode(fs->mount->mnt_sb);
+}
+EXPORT_SYMBOL(simple_alloc_anon_inode);
/**
* simple_read_from_buffer - copy data from the buffer to user space
diff --git a/include/linux/fs.h b/include/linux/fs.h
index de2577df30ae..5e93de72118b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3373,6 +3373,8 @@ struct simple_fs {
extern int simple_pin_fs(struct simple_fs *, struct file_system_type *);
extern void simple_release_fs(struct simple_fs *);
+extern struct inode *simple_alloc_anon_inode(struct simple_fs *fs);
+
extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
loff_t *ppos, const void *from, size_t available);
extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
--
2.25.2
^ permalink raw reply related
* [PATCH v2 5/7] libfs: add file creation functions
From: Emanuele Giuseppe Esposito @ 2020-04-21 13:57 UTC (permalink / raw)
To: linux-fsdevel
Cc: Rafael J. Wysocki, David Airlie, dri-devel, Christoph Hellwig,
Andrew Donnellan, Emanuele Giuseppe Esposito, linux-scsi,
James Morris, Serge E. Hallyn, Daniel Vetter, Arnd Bergmann,
James E.J. Bottomley, Maarten Lankhorst, Maxime Ripard,
Manoj N. Kumar, Alexander Viro, Matthew R. Ochs, Uma Krishnan,
John Johansen, Martin K. Petersen, Greg Kroah-Hartman,
linux-kernel, linux-security-module, Thomas Zimmermann,
Frederic Barrat, Paolo Bonzini, linuxppc-dev, Joel Becker
In-Reply-To: <20200421135119.30007-1-eesposit@redhat.com>
A bunch of code is duplicated between debugfs and tracefs, unify it to the
libfs library.
The code is very similar, except that dentry and inode creation are unified
into a single function (unlike start_creating in debugfs and tracefs, which
only takes care of dentries). This adds an output parameter to the
creation functions, but pushes all error recovery into fs/libfs.c.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
fs/libfs.c | 226 +++++++++++++++++++++++++++++++++++++++++++++
include/linux/fs.h | 18 ++++
2 files changed, 244 insertions(+)
diff --git a/fs/libfs.c b/fs/libfs.c
index 5c76e4c648dc..90b0c221d9a2 100644
--- a/fs/libfs.c
+++ b/fs/libfs.c
@@ -751,6 +751,232 @@ struct inode *simple_alloc_anon_inode(struct simple_fs *fs)
}
EXPORT_SYMBOL(simple_alloc_anon_inode);
+static struct dentry *failed_creating(struct simple_fs *fs, struct dentry *dentry)
+{
+ inode_unlock(d_inode(dentry->d_parent));
+ dput(dentry);
+ simple_release_fs(fs);
+ return ERR_PTR(-ENOMEM);
+}
+
+/**
+ * simplefs_create_dentry - creates a new dentry and inode
+ * @fs: a pointer to a struct simple_fs containing the reference counter
+ * and vfs_mount pointer
+ * @type: the fs type
+ * @name: dentry name
+ * @parent: parent dentry. If this parameter is NULL,
+ * then the dentry will be created in the root of the
+ * filesystem.
+ * @inode: pointer that will contain a newly created inode
+ *
+ * This function returns a new dentry, or NULL on error. On success, a
+ * new inode is created and stored into @inode. Also note that the inode
+ * for the parent directory is locked by simplefs_create_dentry(),
+ * and will be unlocked by simple_finish_dentry().
+ **/
+struct dentry *simplefs_create_dentry(struct simple_fs *fs, struct file_system_type *type,
+ const char *name, struct dentry *parent,
+ struct inode **inode)
+{
+ struct dentry *dentry;
+ int error;
+
+ pr_debug("creating file '%s'\n", name);
+
+ if (IS_ERR(parent))
+ return parent;
+
+ error = simple_pin_fs(fs, type);
+ if (error) {
+ pr_err("Unable to pin filesystem for file '%s'\n", name);
+ return ERR_PTR(error);
+ }
+
+ /* If the parent is not specified, we create it in the root.
+ * We need the root dentry to do this, which is in the super
+ * block. A pointer to that is in the struct vfsmount that we
+ * have around.
+ */
+ if (!parent)
+ parent = fs->mount->mnt_root;
+
+ inode_lock(d_inode(parent));
+ dentry = lookup_one_len(name, parent, strlen(name));
+ if (!IS_ERR(dentry) && d_really_is_positive(dentry)) {
+ if (d_is_dir(dentry))
+ pr_err("Directory '%s' with parent '%s' already present!\n",
+ name, parent->d_name.name);
+ else
+ pr_err("File '%s' in directory '%s' already present!\n",
+ name, parent->d_name.name);
+ dput(dentry);
+ dentry = ERR_PTR(-EEXIST);
+ }
+
+ if (IS_ERR(dentry)) {
+ inode_unlock(d_inode(parent));
+ simple_release_fs(fs);
+ }
+
+
+ if (IS_ERR(dentry))
+ return dentry;
+
+ *inode = new_inode_current_time(fs->mount->mnt_sb);
+ if (unlikely(!(*inode))) {
+ pr_err("out of free inodes, can not create file '%s'\n",
+ name);
+ return failed_creating(fs, dentry);
+ }
+
+ return dentry;
+}
+EXPORT_SYMBOL(simplefs_create_dentry);
+
+/**
+ * simplefs_create_file - creates a new file dentry and inode
+ * @fs: a pointer to a struct simple_fs containing the reference counter
+ * and vfs_mount pointer
+ * @type: the fs type
+ * @name: file name
+ * @mode: file mode
+ * @parent: parent dentry. If this parameter is NULL,
+ * then the file will be created in the root of the
+ * filesystem.
+ * @data: what will the file contain
+ * @inode: pointer that will contain a newly created inode
+ *
+ * This function returns a new dentry, or NULL on error. On success, a
+ * new inode is created and stored into @inode. Also note that the inode
+ * for the parent directory is locked by simplefs_create_dentry(),
+ * and will be unlocked by simple_finish_dentry().
+ **/
+struct dentry *simplefs_create_file(struct simple_fs *fs, struct file_system_type *type,
+ const char *name, umode_t mode,
+ struct dentry *parent, void *data,
+ struct inode **inode)
+{
+ struct dentry *dentry;
+
+ WARN_ON((mode & S_IFMT) && !S_ISREG(mode));
+ mode |= S_IFREG;
+
+ dentry = simplefs_create_dentry(fs, type, name, parent, inode);
+
+ if (IS_ERR(dentry))
+ return dentry;
+
+ (*inode)->i_mode = mode;
+ (*inode)->i_private = data;
+
+ return dentry;
+}
+EXPORT_SYMBOL(simplefs_create_file);
+
+
+/**
+ * simplefs_finish_dentry- complete creation of a new dentry
+ * @dentry: the dentry being created
+ * @inode: the inode associated to the dentry
+ *
+ * This function completes the creation of a dentry.
+ * This includes associating @inode with the dentry, ensuring the link
+ * counts are consistent and informing fsnotify.
+ **/
+struct dentry *simplefs_finish_dentry(struct dentry *dentry, struct inode *inode)
+{
+ d_instantiate(dentry, inode);
+ if (S_ISDIR(inode->i_mode)) {
+ inc_nlink(d_inode(dentry->d_parent));
+ fsnotify_mkdir(d_inode(dentry->d_parent), dentry);
+ } else {
+ fsnotify_create(d_inode(dentry->d_parent), dentry);
+ }
+ inode_unlock(d_inode(dentry->d_parent));
+ return dentry;
+}
+EXPORT_SYMBOL(simplefs_finish_dentry);
+
+/**
+ * simplefs_create_dir - creates a new directory dentry and inode
+ * @fs: a pointer to a struct simple_fs containing the reference counter
+ * and vfs_mount pointer
+ * @type: the fs type
+ * @name: dir name
+ * @mode: dir mode
+ * @parent: parent dentry. If this parameter is NULL,
+ * then the directory will be created in the root of the
+ * filesystem.
+ * @inode: pointer that will contain a newly created inode
+ *
+ * This function returns a new dentry, or NULL on error. On success, a
+ * new inode is created and stored into @inode. Also note that the inode
+ * for the parent directory is locked by simplefs_create_dentry(),
+ * and will be unlocked by simple_finish_dentry().
+ **/
+struct dentry *simplefs_create_dir(struct simple_fs *fs, struct file_system_type *type,
+ const char *name, umode_t mode, struct dentry *parent,
+ struct inode **inode)
+{
+ struct dentry *dentry;
+
+ WARN_ON((mode & S_IFMT) && !S_ISDIR(mode));
+ mode |= S_IFDIR;
+
+ dentry = simplefs_create_dentry(fs, type, name, parent, inode);
+ if (IS_ERR(dentry))
+ return dentry;
+
+ (*inode)->i_mode = mode;
+ (*inode)->i_op = &simple_dir_inode_operations;
+ (*inode)->i_fop = &simple_dir_operations;
+
+ /* directory inodes start off with i_nlink == 2 (for "." entry) */
+ inc_nlink(*inode);
+ return dentry;
+}
+EXPORT_SYMBOL(simplefs_create_dir);
+
+/**
+ * simplefs_create_symlink - creates a new symlink dentry and inode
+ * @fs: a pointer to a struct simple_fs containing the reference counter
+ * and vfs_mount pointer
+ * @type: the fs type
+ * @name: symlink name
+ * @parent: parent dentry. If this parameter is NULL,
+ * then the symbolic link will be created in the root of the
+ * filesystem.
+ * @inode: pointer that will contain a newly created inode
+ *
+ * This function returns a new dentry, or NULL on error. On success, a
+ * new inode is created and stored into @inode. Also note that the inode
+ * for the parent directory is locked by simplefs_create_dentry(),
+ * and will be unlocked by simple_finish_dentry().
+ **/
+struct dentry *simplefs_create_symlink(struct simple_fs *fs, struct file_system_type *type,
+ const char *name, struct dentry *parent,
+ const char *target, struct inode **inode)
+{
+ struct dentry *dentry;
+ char *link = kstrdup(target, GFP_KERNEL);
+
+ if (!link)
+ return ERR_PTR(-ENOMEM);
+
+ dentry = simplefs_create_dentry(fs, type, name, parent, inode);
+ if (IS_ERR(dentry)) {
+ kfree_link(link);
+ return dentry;
+ }
+
+ (*inode)->i_mode = S_IFLNK | S_IRWXUGO;
+ (*inode)->i_link = link;
+ (*inode)->i_op = &simple_symlink_inode_operations;
+ return dentry;
+}
+EXPORT_SYMBOL(simplefs_create_symlink);
+
/**
* simple_read_from_buffer - copy data from the buffer to user space
* @to: the user space buffer to read to
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 5e93de72118b..0569540fbe61 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -3375,6 +3375,24 @@ extern void simple_release_fs(struct simple_fs *);
extern struct inode *simple_alloc_anon_inode(struct simple_fs *fs);
+extern struct dentry *simplefs_create_dentry(struct simple_fs *fs,
+ struct file_system_type *type,
+ const char *name, struct dentry *parent,
+ struct inode **inode);
+struct dentry *simplefs_finish_dentry(struct dentry *dentry, struct inode *inode);
+
+extern struct dentry *simplefs_create_file(struct simple_fs *fs,
+ struct file_system_type *type,
+ const char *name, umode_t mode,
+ struct dentry *parent, void *data,
+ struct inode **inode);
+extern struct dentry *simplefs_create_dir(struct simple_fs *fs, struct file_system_type *type,
+ const char *name, umode_t mode, struct dentry *parent,
+ struct inode **inode);
+extern struct dentry *simplefs_create_symlink(struct simple_fs *fs, struct file_system_type *type,
+ const char *name, struct dentry *parent,
+ const char *target, struct inode **inode);
+
extern ssize_t simple_read_from_buffer(void __user *to, size_t count,
loff_t *ppos, const void *from, size_t available);
extern ssize_t simple_write_to_buffer(void *to, size_t available, loff_t *ppos,
--
2.25.2
^ permalink raw reply related
* [PATCH v2 6/7] debugfs: switch to simplefs inode creation API
From: Emanuele Giuseppe Esposito @ 2020-04-21 13:57 UTC (permalink / raw)
To: linux-fsdevel
Cc: Rafael J. Wysocki, David Airlie, dri-devel, Christoph Hellwig,
Andrew Donnellan, Emanuele Giuseppe Esposito, linux-scsi,
James Morris, Serge E. Hallyn, Daniel Vetter, Arnd Bergmann,
James E.J. Bottomley, Maarten Lankhorst, Maxime Ripard,
Manoj N. Kumar, Alexander Viro, Matthew R. Ochs, Uma Krishnan,
John Johansen, Martin K. Petersen, Greg Kroah-Hartman,
linux-kernel, linux-security-module, Thomas Zimmermann,
Frederic Barrat, Paolo Bonzini, linuxppc-dev, Joel Becker
In-Reply-To: <20200421135119.30007-1-eesposit@redhat.com>
The only difference, compared to the pre-existing code, is that symlink
creation now triggers fsnotify_create. This was a bug in the debugfs
code, since for example vfs_symlink does call fsnotify_create.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
fs/debugfs/inode.c | 144 +++++----------------------------------------
1 file changed, 15 insertions(+), 129 deletions(-)
diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c
index 5dbb74a23e7c..ccbeea9e5f6c 100644
--- a/fs/debugfs/inode.c
+++ b/fs/debugfs/inode.c
@@ -305,68 +305,6 @@ struct dentry *debugfs_lookup(const char *name, struct dentry *parent)
}
EXPORT_SYMBOL_GPL(debugfs_lookup);
-static struct dentry *start_creating(const char *name, struct dentry *parent)
-{
- struct dentry *dentry;
- int error;
-
- pr_debug("creating file '%s'\n", name);
-
- if (IS_ERR(parent))
- return parent;
-
- error = simple_pin_fs(&debugfs, &debug_fs_type);
- if (error) {
- pr_err("Unable to pin filesystem for file '%s'\n", name);
- return ERR_PTR(error);
- }
-
- /* If the parent is not specified, we create it in the root.
- * We need the root dentry to do this, which is in the super
- * block. A pointer to that is in the struct vfsmount that we
- * have around.
- */
- if (!parent)
- parent = debugfs.mount->mnt_root;
-
- inode_lock(d_inode(parent));
- if (unlikely(IS_DEADDIR(d_inode(parent))))
- dentry = ERR_PTR(-ENOENT);
- else
- dentry = lookup_one_len(name, parent, strlen(name));
- if (!IS_ERR(dentry) && d_really_is_positive(dentry)) {
- if (d_is_dir(dentry))
- pr_err("Directory '%s' with parent '%s' already present!\n",
- name, parent->d_name.name);
- else
- pr_err("File '%s' in directory '%s' already present!\n",
- name, parent->d_name.name);
- dput(dentry);
- dentry = ERR_PTR(-EEXIST);
- }
-
- if (IS_ERR(dentry)) {
- inode_unlock(d_inode(parent));
- simple_release_fs(&debugfs);
- }
-
- return dentry;
-}
-
-static struct dentry *failed_creating(struct dentry *dentry)
-{
- inode_unlock(d_inode(dentry->d_parent));
- dput(dentry);
- simple_release_fs(&debugfs);
- return ERR_PTR(-ENOMEM);
-}
-
-static struct dentry *end_creating(struct dentry *dentry)
-{
- inode_unlock(d_inode(dentry->d_parent));
- return dentry;
-}
-
static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
struct dentry *parent, void *data,
const struct file_operations *proxy_fops,
@@ -375,32 +313,17 @@ static struct dentry *__debugfs_create_file(const char *name, umode_t mode,
struct dentry *dentry;
struct inode *inode;
- if (!(mode & S_IFMT))
- mode |= S_IFREG;
- BUG_ON(!S_ISREG(mode));
- dentry = start_creating(name, parent);
-
+ dentry = simplefs_create_file(&debugfs, &debug_fs_type,
+ name, mode, parent, data, &inode);
if (IS_ERR(dentry))
return dentry;
- inode = debugfs_get_inode(dentry->d_sb);
- if (unlikely(!inode)) {
- pr_err("out of free dentries, can not create file '%s'\n",
- name);
- return failed_creating(dentry);
- }
-
- inode->i_mode = mode;
- inode->i_private = data;
-
inode->i_op = &debugfs_file_inode_operations;
inode->i_fop = proxy_fops;
dentry->d_fsdata = (void *)((unsigned long)real_fops |
DEBUGFS_FSDATA_IS_REAL_FOPS_BIT);
- d_instantiate(dentry, inode);
- fsnotify_create(d_inode(dentry->d_parent), dentry);
- return end_creating(dentry);
+ return simplefs_finish_dentry(dentry, inode);
}
/**
@@ -533,29 +456,16 @@ EXPORT_SYMBOL_GPL(debugfs_create_file_size);
*/
struct dentry *debugfs_create_dir(const char *name, struct dentry *parent)
{
- struct dentry *dentry = start_creating(name, parent);
+ struct dentry *dentry;
struct inode *inode;
+ dentry = simplefs_create_dir(&debugfs, &debug_fs_type,
+ name, 0755, parent, &inode);
if (IS_ERR(dentry))
return dentry;
- inode = debugfs_get_inode(dentry->d_sb);
- if (unlikely(!inode)) {
- pr_err("out of free dentries, can not create directory '%s'\n",
- name);
- return failed_creating(dentry);
- }
-
- inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
inode->i_op = &debugfs_dir_inode_operations;
- inode->i_fop = &simple_dir_operations;
-
- /* directory inodes start off with i_nlink == 2 (for "." entry) */
- inc_nlink(inode);
- d_instantiate(dentry, inode);
- inc_nlink(d_inode(dentry->d_parent));
- fsnotify_mkdir(d_inode(dentry->d_parent), dentry);
- return end_creating(dentry);
+ return simplefs_finish_dentry(dentry, inode);
}
EXPORT_SYMBOL_GPL(debugfs_create_dir);
@@ -575,29 +485,19 @@ struct dentry *debugfs_create_automount(const char *name,
debugfs_automount_t f,
void *data)
{
- struct dentry *dentry = start_creating(name, parent);
+ struct dentry *dentry;
struct inode *inode;
+ dentry = simplefs_create_dentry(&debugfs, &debug_fs_type, name, parent,
+ &inode);
if (IS_ERR(dentry))
return dentry;
- inode = debugfs_get_inode(dentry->d_sb);
- if (unlikely(!inode)) {
- pr_err("out of free dentries, can not create automount '%s'\n",
- name);
- return failed_creating(dentry);
- }
-
make_empty_dir_inode(inode);
inode->i_flags |= S_AUTOMOUNT;
inode->i_private = data;
dentry->d_fsdata = (void *)f;
- /* directory inodes start off with i_nlink == 2 (for "." entry) */
- inc_nlink(inode);
- d_instantiate(dentry, inode);
- inc_nlink(d_inode(dentry->d_parent));
- fsnotify_mkdir(d_inode(dentry->d_parent), dentry);
- return end_creating(dentry);
+ return simplefs_finish_dentry(dentry, inode);
}
EXPORT_SYMBOL(debugfs_create_automount);
@@ -629,28 +529,14 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent,
{
struct dentry *dentry;
struct inode *inode;
- char *link = kstrdup(target, GFP_KERNEL);
- if (!link)
- return ERR_PTR(-ENOMEM);
- dentry = start_creating(name, parent);
- if (IS_ERR(dentry)) {
- kfree(link);
+ dentry = simplefs_create_symlink(&debugfs, &debug_fs_type,
+ name, parent, target, &inode);
+ if (IS_ERR(dentry))
return dentry;
- }
- inode = debugfs_get_inode(dentry->d_sb);
- if (unlikely(!inode)) {
- pr_err("out of free dentries, can not create symlink '%s'\n",
- name);
- kfree(link);
- return failed_creating(dentry);
- }
- inode->i_mode = S_IFLNK | S_IRWXUGO;
inode->i_op = &debugfs_symlink_inode_operations;
- inode->i_link = link;
- d_instantiate(dentry, inode);
- return end_creating(dentry);
+ return simplefs_finish_dentry(dentry, inode);
}
EXPORT_SYMBOL_GPL(debugfs_create_symlink);
--
2.25.2
^ permalink raw reply related
* [PATCH v2 7/7] tracefs: switch to simplefs inode creation API
From: Emanuele Giuseppe Esposito @ 2020-04-21 13:57 UTC (permalink / raw)
To: linux-fsdevel
Cc: Rafael J. Wysocki, David Airlie, dri-devel, Christoph Hellwig,
Andrew Donnellan, Emanuele Giuseppe Esposito, linux-scsi,
James Morris, Serge E. Hallyn, Daniel Vetter, Arnd Bergmann,
James E.J. Bottomley, Maarten Lankhorst, Maxime Ripard,
Manoj N. Kumar, Alexander Viro, Matthew R. Ochs, Uma Krishnan,
John Johansen, Martin K. Petersen, Greg Kroah-Hartman,
linux-kernel, linux-security-module, Thomas Zimmermann,
Frederic Barrat, Paolo Bonzini, linuxppc-dev, Joel Becker
In-Reply-To: <20200421135119.30007-1-eesposit@redhat.com>
There is no semantic change intended; the code in the libfs.c
functions in fact was derived from debugfs and tracefs code.
Signed-off-by: Emanuele Giuseppe Esposito <eesposit@redhat.com>
---
fs/tracefs/inode.c | 86 ++++------------------------------------------
1 file changed, 7 insertions(+), 79 deletions(-)
diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c
index 370eb38ff1ad..bceaa4f45da2 100644
--- a/fs/tracefs/inode.c
+++ b/fs/tracefs/inode.c
@@ -308,57 +308,6 @@ static struct file_system_type trace_fs_type = {
};
MODULE_ALIAS_FS("tracefs");
-static struct dentry *start_creating(const char *name, struct dentry *parent)
-{
- struct dentry *dentry;
- int error;
-
- pr_debug("tracefs: creating file '%s'\n",name);
-
- error = simple_pin_fs(&tracefs, &trace_fs_type);
- if (error)
- return ERR_PTR(error);
-
- /* If the parent is not specified, we create it in the root.
- * We need the root dentry to do this, which is in the super
- * block. A pointer to that is in the struct vfsmount that we
- * have around.
- */
- if (!parent)
- parent = tracefs.mount->mnt_root;
-
- inode_lock(parent->d_inode);
- if (unlikely(IS_DEADDIR(parent->d_inode)))
- dentry = ERR_PTR(-ENOENT);
- else
- dentry = lookup_one_len(name, parent, strlen(name));
- if (!IS_ERR(dentry) && dentry->d_inode) {
- dput(dentry);
- dentry = ERR_PTR(-EEXIST);
- }
-
- if (IS_ERR(dentry)) {
- inode_unlock(parent->d_inode);
- simple_release_fs(&tracefs);
- }
-
- return dentry;
-}
-
-static struct dentry *failed_creating(struct dentry *dentry)
-{
- inode_unlock(dentry->d_parent->d_inode);
- dput(dentry);
- simple_release_fs(&tracefs);
- return NULL;
-}
-
-static struct dentry *end_creating(struct dentry *dentry)
-{
- inode_unlock(dentry->d_parent->d_inode);
- return dentry;
-}
-
/**
* tracefs_create_file - create a file in the tracefs filesystem
* @name: a pointer to a string containing the name of the file to create.
@@ -395,49 +344,28 @@ struct dentry *tracefs_create_file(const char *name, umode_t mode,
if (security_locked_down(LOCKDOWN_TRACEFS))
return NULL;
- if (!(mode & S_IFMT))
- mode |= S_IFREG;
- BUG_ON(!S_ISREG(mode));
- dentry = start_creating(name, parent);
-
+ dentry = simplefs_create_file(&tracefs, &trace_fs_type,
+ name, mode, parent, data, &inode);
if (IS_ERR(dentry))
return NULL;
- inode = tracefs_get_inode(dentry->d_sb);
- if (unlikely(!inode))
- return failed_creating(dentry);
-
- inode->i_mode = mode;
inode->i_fop = fops ? fops : &tracefs_file_operations;
- inode->i_private = data;
- d_instantiate(dentry, inode);
- fsnotify_create(dentry->d_parent->d_inode, dentry);
- return end_creating(dentry);
+ return simplefs_finish_dentry(dentry, inode);
}
static struct dentry *__create_dir(const char *name, struct dentry *parent,
const struct inode_operations *ops)
{
- struct dentry *dentry = start_creating(name, parent);
+ struct dentry *dentry;
struct inode *inode;
+ dentry = simplefs_create_dir(&tracefs, &trace_fs_type,
+ name, 0755, parent, &inode);
if (IS_ERR(dentry))
return NULL;
- inode = tracefs_get_inode(dentry->d_sb);
- if (unlikely(!inode))
- return failed_creating(dentry);
-
- inode->i_mode = S_IFDIR | S_IRWXU | S_IRUGO | S_IXUGO;
inode->i_op = ops;
- inode->i_fop = &simple_dir_operations;
-
- /* directory inodes start off with i_nlink == 2 (for "." entry) */
- inc_nlink(inode);
- d_instantiate(dentry, inode);
- inc_nlink(dentry->d_parent->d_inode);
- fsnotify_mkdir(dentry->d_parent->d_inode, dentry);
- return end_creating(dentry);
+ return simplefs_finish_dentry(dentry, inode);
}
/**
--
2.25.2
^ permalink raw reply related
* Re: [PATCH v3 0/4] Clean up hugetlb boot command line processing
From: Gerald Schaefer @ 2020-04-21 14:02 UTC (permalink / raw)
To: Mike Kravetz
Cc: linux-doc, Catalin Marinas, Dave Hansen, Heiko Carstens, Peter Xu,
linux-mm, Paul Mackerras, sparclinux, linux-riscv, Will Deacon,
Mina Almasry, linux-s390, Jonathan Corbet, Christian Borntraeger,
Ingo Molnar, Gerald Schaefer, Longpeng, Albert Ou, Vasily Gorbik,
Paul Walmsley, Thomas Gleixner, linux-arm-kernel,
Nitesh Narayan Lal, Randy Dunlap, linux-kernel, Palmer Dabbelt,
Andrew Morton, linuxppc-dev, David S . Miller
In-Reply-To: <20200417185049.275845-1-mike.kravetz@oracle.com>
On Fri, 17 Apr 2020 11:50:45 -0700
Mike Kravetz <mike.kravetz@oracle.com> wrote:
> v3 -
> Used weak attribute method of defining arch_hugetlb_valid_size.
> This eliminates changes to arch specific hugetlb.h files (Peter)
> Updated documentation (Peter, Randy)
> Fixed handling of implicitly specified gigantic page preallocation
> in existing code and removed documentation of such. There is now
> no difference between handling of gigantic and non-gigantic pages.
> (Peter, Nitesh).
> This requires the most review as there is a small change to
> undocumented behavior. See patch 4 commit message for details.
> Added Acks and Reviews (Mina, Peter)
>
> v2 -
> Fix build errors with patch 1 (Will)
> Change arch_hugetlb_valid_size arg to unsigned long and remove
> irrelevant 'extern' keyword (Christophe)
> Documentation and other misc changes (Randy, Christophe, Mina)
> Do not process command line options if !hugepages_supported()
> (Dave, but it sounds like we may want to additional changes to
> hugepages_supported() for x86? If that is needed I would prefer
> a separate patch.)
>
> Longpeng(Mike) reported a weird message from hugetlb command line processing
> and proposed a solution [1]. While the proposed patch does address the
> specific issue, there are other related issues in command line processing.
> As hugetlbfs evolved, updates to command line processing have been made to
> meet immediate needs and not necessarily in a coordinated manner. The result
> is that some processing is done in arch specific code, some is done in arch
> independent code and coordination is problematic. Semantics can vary between
> architectures.
>
> The patch series does the following:
> - Define arch specific arch_hugetlb_valid_size routine used to validate
> passed huge page sizes.
> - Move hugepagesz= command line parsing out of arch specific code and into
> an arch independent routine.
> - Clean up command line processing to follow desired semantics and
> document those semantics.
>
> [1] https://lore.kernel.org/linux-mm/20200305033014.1152-1-longpeng2@huawei.com
>
> Mike Kravetz (4):
> hugetlbfs: add arch_hugetlb_valid_size
> hugetlbfs: move hugepagesz= parsing to arch independent code
> hugetlbfs: remove hugetlb_add_hstate() warning for existing hstate
> hugetlbfs: clean up command line processing
>
> .../admin-guide/kernel-parameters.txt | 40 ++--
> Documentation/admin-guide/mm/hugetlbpage.rst | 35 ++++
> arch/arm64/mm/hugetlbpage.c | 30 +--
> arch/powerpc/mm/hugetlbpage.c | 30 +--
> arch/riscv/mm/hugetlbpage.c | 24 +--
> arch/s390/mm/hugetlbpage.c | 24 +--
> arch/sparc/mm/init_64.c | 43 +---
> arch/x86/mm/hugetlbpage.c | 23 +--
> include/linux/hugetlb.h | 2 +-
> mm/hugetlb.c | 190 +++++++++++++++---
> 10 files changed, 271 insertions(+), 170 deletions(-)
>
Looks good and works fine for s390, thanks for cleaning up!
Acked-by: Gerald Schaefer <gerald.schaefer@de.ibm.com> # s390
^ permalink raw reply
* Re: [PATCH 1/3] kexec: Prevent removal of memory in use by a loaded kexec image
From: Eric W. Biederman @ 2020-04-21 13:59 UTC (permalink / raw)
To: David Hildenbrand
Cc: piliu, Baoquan He, Anshuman Khandual, Catalin Marinas,
Bhupesh Sharma, linuxppc-dev, kexec,
Russell King - ARM Linux admin, linux-mm, James Morse,
Andrew Morton, Will Deacon, linux-arm-kernel
In-Reply-To: <9a4eb1d7-33bf-8707-9c0c-1ca657c3e502@redhat.com>
David Hildenbrand <david@redhat.com> writes:
>>> ACPI SRAT is embeded into efi, need read out the rsdp pointer. If we don't
>>> pass the efi, it won't get the SRAT table correctly, if I remember
>>> correctly. Yeah, I remeber kvm guest can get memory hotplugged with
>>> ACPI only, this won't happen on bare metal though. Need check carefully.
>>> I have been using kvm guest with uefi firmwire recently.
>>
>> Yeah, I can imagine that bare metal is different. kvm only uses ACPI.
>>
>> I'm also asking because of virtio-mem. Memory added via virtio-mem is
>> not part of any efi tables or whatsoever. So I assume the kexec kernel
>> will not detect it automatically (good!), instead load the virtio-mem
>> driver and let it add memory back to the system.
>>
>> I should probably play with kexec and virtio-mem once I have some spare
>> cycles ... to find out what's broken and needs to be addressed :)
>
> FWIW, I just gave virtio-mem and kexec/kdump a try.
>
> a) kdump seems to work. Memory added by virtio-mem is getting dumped.
> The kexec kernel only uses memory in the crash region. The virtio-mem
> driver properly bails out due to is_kdump_kernel().
>
> b) "kexec -s -l" seems to work fine. For now, the kernel does not seem
> to get placed on virtio-mem memory (pure luck due to the left-to-right
> search). Memory added by virtio-mem is not getting added to the e820
> map. Once the virtio-mem driver comes back up in the kexec kernel, the
> right memory is readded.
This sounds like a bug.
> c) "kexec -c -l" does not work properly. All memory added by virtio-mem
> is added to the e820 map, which is wrong. Memory that should not be
> touched will be touched by the kexec kernel. I assume kexec-tools just
> goes ahead and adds anything it can find in /proc/iomem (or
> /sys/firmware/memmap/) to the e820 map of the new kernel.
>
> Due to c), I assume all hotplugged memory (e.g., ACPI DIMMs) is
> similarly added to the e820 map and, therefore, won't be able to be
> onlined MOVABLE easily.
This sounds like correct behavior to me. If you add memory to the
system it is treated as memory to the system.
If we need to make it a special kind of memory with special rules we can
have some kind of special marking for the memory. But hotplugged is not
in itself a sufficient criteria to say don't use this as normal memory.
If take a huge server and I plug in an extra dimm it is just memory.
For a similarly huge server I might want to have memory that the system
booted with unpluggable, in case hardware error reporting notices
a dimm generating a lot of memory errors.
Now perhaps virtualization needs a special tier of memory that should
only be used for cases where the memory is easily movable.
I am not familiar with virtio-mem but my skim of the initial design
is that virtio-mem was not designed to be such a special tier of memory.
Perhaps something has changed?
https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03870.html
> At least for virtio-mem, I would either have to
> a) Not support "kexec -c -l". A viable option if we would be planning on
> not supporting it either way in the long term. I could block this
> in-kernel somehow eventually.
No.
> b) Teach kexec-tools to leave virtio-mem added memory alone. E.g., by
> indicating it in /proc/iomem in a special way ("System RAM
> (hotplugged)"/"System RAM (virtio-mem)").
How does the kernel memory allocator treat this memory?
The logic is simple. If the kernel memory allocator treats that memory
as ordinary memory available for all uses it should be presented as
ordinary memory available for all uses.
If the kernel memory allocator treats that memory as special memory
only available for uses that we can easily free later and give back to
the system. AKA it is special and not oridinary memory we should mark
it as such.
Eric
p.s. Please excuse me for jumping in I may be missing some important
context, but what I read when I saw this message in my inbox just seemed
very wrong.
^ permalink raw reply
* Re: [PATCH v11 01/14] powerpc/xive: Define xive_native_alloc_irq_on_chip()
From: Michael Ellerman @ 2020-04-21 14:09 UTC (permalink / raw)
To: Haren Myneni
Cc: mikey, ajd, frederic.barrat, linux-kernel, npiggin, hch, oohall,
clg, herbert, sukadev, linuxppc-dev, srikar
In-Reply-To: <1587016720.2275.1047.camel@hbabu-laptop>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain, Size: 416 bytes --]
On Thu, 2020-04-16 at 05:58:40 UTC, Haren Myneni wrote:
>
> This function allocates IRQ on a specific chip. VAS needs per chip
> IRQ allocation and will have IRQ handler per VAS instance.
>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/8d0ea29db5aefd0d94fa4b6ca6124c68998f3c6a
cheers
^ permalink raw reply
* Re: [PATCH v6 1/9] powerpc/vas: Initialize window attributes for GZIP coprocessor type
From: Michael Ellerman @ 2020-04-21 14:09 UTC (permalink / raw)
To: Haren Myneni
Cc: mikey, herbert, npiggin, linux-crypto, sukadev, linuxppc-dev, dja
In-Reply-To: <1587114029.2275.1103.camel@hbabu-laptop>
On Fri, 2020-04-17 at 09:00:29 UTC, Haren Myneni wrote:
>
> Initialize send and receive window attributes for GZIP high and
> normal priority types.
>
> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/a8c0c69b5e95e8f155480d5203a7bafb8024fd93
cheers
^ permalink raw reply
* Re: [PATCH V4 1/5] selftests/powerpc: Add header files for GZIP engine test
From: Michael Ellerman @ 2020-04-21 14:09 UTC (permalink / raw)
To: Raphael Moreira Zinsly, linuxppc-dev, linux-crypto, dja
Cc: haren, abali, herbert, rzinsly
In-Reply-To: <20200420205538.25181-2-rzinsly@linux.ibm.com>
On Mon, 2020-04-20 at 20:55:34 UTC, Raphael Moreira Zinsly wrote:
> Add files to access the powerpc NX-GZIP engine in user space.
>
> Signed-off-by: Bulent Abali <abali@us.ibm.com>
> Signed-off-by: Raphael Moreira Zinsly <rzinsly@linux.ibm.com>
Series applied to powerpc next, thanks.
https://git.kernel.org/powerpc/c/d53979b589609d87036d8daf9500f7eccb0c6317
cheers
^ permalink raw reply
* Re: [PATCH 1/3] kexec: Prevent removal of memory in use by a loaded kexec image
From: David Hildenbrand @ 2020-04-21 14:30 UTC (permalink / raw)
To: Eric W. Biederman
Cc: piliu, Baoquan He, Anshuman Khandual, Catalin Marinas,
Bhupesh Sharma, linuxppc-dev, kexec,
Russell King - ARM Linux admin, linux-mm, James Morse,
Andrew Morton, Will Deacon, linux-arm-kernel
In-Reply-To: <87a735548w.fsf@x220.int.ebiederm.org>
>> b) "kexec -s -l" seems to work fine. For now, the kernel does not seem
>> to get placed on virtio-mem memory (pure luck due to the left-to-right
>> search). Memory added by virtio-mem is not getting added to the e820
>> map. Once the virtio-mem driver comes back up in the kexec kernel, the
>> right memory is readded.
>
> This sounds like a bug.
This is how virtio-mem wants its memory to get handled.
>
>> c) "kexec -c -l" does not work properly. All memory added by virtio-mem
>> is added to the e820 map, which is wrong. Memory that should not be
>> touched will be touched by the kexec kernel. I assume kexec-tools just
>> goes ahead and adds anything it can find in /proc/iomem (or
>> /sys/firmware/memmap/) to the e820 map of the new kernel.
>>
>> Due to c), I assume all hotplugged memory (e.g., ACPI DIMMs) is
>> similarly added to the e820 map and, therefore, won't be able to be
>> onlined MOVABLE easily.
>
> This sounds like correct behavior to me. If you add memory to the
> system it is treated as memory to the system.
Yeah, I would agree if we are talking about DIMMs, but this memory is
special. It's added via a paravirtualized interface and will contain
holes, especially after unplug. While memory in these holes can usually
be read, it should not be written. More on that below.
>
> If we need to make it a special kind of memory with special rules we can
> have some kind of special marking for the memory. But hotplugged is not
> in itself a sufficient criteria to say don't use this as normal memory.
Agreed. It is special, though.
>
> If take a huge server and I plug in an extra dimm it is just memory.
Agreed.
[...]
>
> Now perhaps virtualization needs a special tier of memory that should
> only be used for cases where the memory is easily movable.
>
> I am not familiar with virtio-mem but my skim of the initial design
> is that virtio-mem was not designed to be such a special tier of memory.
> Perhaps something has changed?
> https://lists.gnu.org/archive/html/qemu-devel/2017-06/msg03870.html
Yes, a lot changed. See
https://lkml.kernel.org/r/20200311171422.10484-1-david@redhat.com for
the latest-greatest design overview.
>
>> b) Teach kexec-tools to leave virtio-mem added memory alone. E.g., by
>> indicating it in /proc/iomem in a special way ("System RAM
>> (hotplugged)"/"System RAM (virtio-mem)").
>
> How does the kernel memory allocator treat this memory?
So what virtio-mem does is add memory sections on demand and populate
within these sections the requested amount of memory. E.g., if 64MB are
requested, it will add a 128MB section/resource but only make the first
64MB accessible (via the hypervisor) and only give the first 64MB to the
buddy. This way of adding memory is similar to what XEN and hypver-v
balloon drivers do when hotplugging memory.
When requested to plug more memory, it might go ahead and make (parts
of) the remaining 64MB accessible and give them to the buddy. In case it
cannot "fill any holes", it will add a new section.
When requested to unplug memory, it will try to remove memory from the
added (here 64MB) memory from the buddy and tell the hypervisor about it.
So, it has some similarity to ballooning in virtual environment,
however, it manages its own device memory only and can therefore give
better guarantees and detect malicious guests.
Right now, I think the right approach would be to not create
/sys/firmware/memmap entries from memory virtio-mem added.
[...]
>
> p.s. Please excuse me for jumping in I may be missing some important
> context, but what I read when I saw this message in my inbox just seemed
> very wrong.
Yeah, still, thanks for having a look. Please let me know if you need
more information.
--
Thanks,
David / dhildenb
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Rich Felker @ 2020-04-21 14:39 UTC (permalink / raw)
To: David Laight
Cc: libc-alpha@sourceware.org, musl@lists.openwall.com,
'Nicholas Piggin', Adhemerval Zanella,
libc-dev@lists.llvm.org, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <b77fa2dc769d42e1a3e68f5edf90d250@AcuMS.aculab.com>
On Tue, Apr 21, 2020 at 12:28:25PM +0000, David Laight wrote:
> From: Nicholas Piggin
> > Sent: 20 April 2020 02:10
> ...
> > >> 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>
>
> Hmmm... that cmp + jae is unnecessary here.
It's not.. Rather the objdump was just mistakenly done without -r so
it looks like a nop jump rather than a conditional tail call to the
function that sets errno.
> It is also a 32bit offset jump.
> I also suspect it gets predicted very badly.
I doubt that. This is a very standard idiom and the size of the offset
(which is necessarily 32-bit because it has a relocation on it) is
orthogonal to the condition on the jump.
FWIW a syscall like kill takes global kernel-side locks to be able to
address a target process by pid, and the rate of meaningful calls you
can make to it is very low (since it's bounded by time for target
process to act on the signal). Trying to optimize it for speed is
pointless, and even size isn't important locally (although in
aggregate, lots of wasted small size can add up to more pages = more
TLB entries = ...).
> > >> 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.
>
> A suitable cast might get rid of the sign extension.
> Possibly just (unsigned int).
No, it won't. The problem is that there is no representation of the
fact that the kernel is only going to inspect the low 32 bits (by
declaring the kernel-side function as taking an int argument). The
external kill function receives arguments by the ABI, where the upper
bits of int args can contain junk, and the asm register constraints
for syscalls use longs (or rather an abstract syscall-arg type). It
wouldn't even work to have macro magic detect that the expressions
passed are ints and use hacks to avoid that, since it's perfectly
valid to pass an int to a syscall that expects a long argument (e.g.
offset to mmap), in which case it needs to be sign-extended.
The only way to avoid this is encoding somewhere the syscall-specific
knowledge of what arg size the kernel function expects. That's way too
much redundant effort and too error-prone for the incredibly miniscule
size benefit you'd get out of it.
Rich
^ permalink raw reply
* Re: [musl] Powerpc Linux 'scv' system call ABI proposal take 2
From: Adhemerval Zanella @ 2020-04-21 15:00 UTC (permalink / raw)
To: Rich Felker, David Laight
Cc: libc-dev@lists.llvm.org, libc-alpha@sourceware.org,
linuxppc-dev@lists.ozlabs.org, 'Nicholas Piggin',
musl@lists.openwall.com
In-Reply-To: <20200421143941.GJ11469@brightrain.aerifal.cx>
On 21/04/2020 11:39, Rich Felker wrote:
> On Tue, Apr 21, 2020 at 12:28:25PM +0000, David Laight wrote:
>> From: Nicholas Piggin
>>> Sent: 20 April 2020 02:10
>> ...
>>>>> 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>
>>
>> Hmmm... that cmp + jae is unnecessary here.
>
> It's not.. Rather the objdump was just mistakenly done without -r so
> it looks like a nop jump rather than a conditional tail call to the
> function that sets errno.
>
Indeed, the output with -r is:
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>
f: R_X86_64_PLT32 __syscall_error-0x4
13: c3 retq
And for x86_64 __syscall_error is defined as:
0000000000000000 <__syscall_error>:
0: 48 f7 d8 neg %rax
0000000000000003 <__syscall_error_1>:
3: 64 89 04 25 00 00 00 mov %eax,%fs:0x0
a: 00
7: R_X86_64_TPOFF32 errno
b: 48 83 c8 ff or $0xffffffffffffffff,%rax
f: c3 retq
Different than musl, each architecture defines its own error handling
mechanism (some embedded errno setting in syscall itself, other branches
to a __syscall_error like function as x86_64).
This is due most likely from the glibc long history. One of my long
term plan is to just simplify, get rid of the assembly pre-processor,
implement all syscall in C code, and set error handling mechanism in
a platform neutral way using a tail call (most likely you do on musl).
>> It is also a 32bit offset jump.
>> I also suspect it gets predicted very badly.
>
> I doubt that. This is a very standard idiom and the size of the offset
> (which is necessarily 32-bit because it has a relocation on it) is
> orthogonal to the condition on the jump.
>
> FWIW a syscall like kill takes global kernel-side locks to be able to
> address a target process by pid, and the rate of meaningful calls you
> can make to it is very low (since it's bounded by time for target
> process to act on the signal). Trying to optimize it for speed is
> pointless, and even size isn't important locally (although in
> aggregate, lots of wasted small size can add up to more pages = more
> TLB entries = ...).
I agree and I would prefer to focus on code simplicity to have a
platform neutral way to handle error and let the compiler optimize
it than messy with assembly macros to squeeze this kind of
micro-optimizations.
>
>>>>> 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.
>>
>> A suitable cast might get rid of the sign extension.
>> Possibly just (unsigned int).
>
> No, it won't. The problem is that there is no representation of the
> fact that the kernel is only going to inspect the low 32 bits (by
> declaring the kernel-side function as taking an int argument). The
> external kill function receives arguments by the ABI, where the upper
> bits of int args can contain junk, and the asm register constraints
> for syscalls use longs (or rather an abstract syscall-arg type). It
> wouldn't even work to have macro magic detect that the expressions
> passed are ints and use hacks to avoid that, since it's perfectly
> valid to pass an int to a syscall that expects a long argument (e.g.
> offset to mmap), in which case it needs to be sign-extended.
>
> The only way to avoid this is encoding somewhere the syscall-specific
> knowledge of what arg size the kernel function expects. That's way too
> much redundant effort and too error-prone for the incredibly miniscule
> size benefit you'd get out of it.
>
> Rich
>
^ 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