On Wed, 31 Jan 2024 10:08:37 -0800
Linus Torvalds wrote:
> On Wed, 31 Jan 2024 at 05:14, Steven Rostedt wrote:
> >
> > If you also notice, tracefs only allows mkdir/rmdir to be assigned to
> > one directory. Once it is assigned, no other directories can have mkdir
> > rmdir functionality.
>
>
On Wed, 31 Jan 2024 at 05:14, Steven Rostedt wrote:
>
> If you also notice, tracefs only allows mkdir/rmdir to be assigned to
> one directory. Once it is assigned, no other directories can have mkdir
> rmdir functionality.
I think that does limit the damage, but it's not clear that it is actually
On Wed, 31 Jan 2024 07:57:40 -0500
Steven Rostedt wrote:
> static int instance_rmdir(const char *name)
> {
> struct trace_array *tr;
> int ret;
>
> mutex_lock(&event_mutex);
Note, event_mutex prevents dynamic events from being created. No kprobe
can be added while the ev
On Tue, 30 Jan 2024 22:20:24 -0800
Linus Torvalds wrote:
> On Tue, 30 Jan 2024 at 21:57, Linus Torvalds
> wrote:
> >
> > Ugh.
>
> Oh, and double-ugh on that tracefs_syscall_mkdir/rmdir(). I hate how
> it does that "unlock and re-lock inode" thing.
I'd figured you'd like that one.
>
> It's
On Tue, 30 Jan 2024 at 21:57, Linus Torvalds
wrote:
>
> Ugh.
Oh, and double-ugh on that tracefs_syscall_mkdir/rmdir(). I hate how
it does that "unlock and re-lock inode" thing.
It's a disease, and no, it's not an acceptable response to "lockdep
shows there's a locking problem".
The comment says
On Tue, 30 Jan 2024 at 21:33, Steven Rostedt wrote:
>
> With even the last patch included, without the d_invalidate() I get errors
> with simply doing:
>
> # cd /sys/kernel/tracing
> # mkdir instances/foo
> # ls instances/foo/events
> # rmdir instances/foo
>
> As the rmdir calls tracefs_remove
On Tue, 30 Jan 2024 21:25:30 -0800
Linus Torvalds wrote:
> > Does this work:
> >
> > d_invalidate(dentry);
>
> It does, but it's basically irrelevant with the d_revalidate approach.
>
> Basically, once you have d_revalidate(), the unhashing happens there,
> and it's just extra work an
On Tue, 30 Jan 2024 at 21:09, Steven Rostedt wrote:
>
> I would think that the last "put" would always have the "is_freed" set. The
> WARN_ON is to catch things doing too many put_ei().
Yes, that looks sane to me.
> > + simple_recursive_removal(dentry, NULL);
>
> Actually, this doesn't work.
On Tue, 30 Jan 2024 11:03:54 -0800
Linus Torvalds wrote:
> +/*
> + * eventfs_inode reference count management.
> + *
> + * NOTE! We count only references from dentries, in the
> + * form 'dentry->d_fsdata'. There are also references from
> + * directory inodes ('ti->private'), but the dentry refe
On Tue, Jan 30, 2024 at 11:03:54AM -0800, Linus Torvalds wrote:
> inode = tracefs_get_inode(dentry->d_sb);
> if (unlikely(!inode))
> - return eventfs_failed_creating(dentry);
> + return ERR_PTR(-ENOMEM);
That belongs in the lookup crapectomy patch - bisect haza
On Tue, 30 Jan 2024 at 15:10, Steven Rostedt wrote:
> >
> > At which point a name pointer would *just* fit in 96 bytes.
>
> Does that mean I should keep the kstrdup_const()?
You should check that my math matches reality and relevant
configurations, but yes, at 96 bytes that should fit exactly in
On Tue, 30 Jan 2024 15:06:13 -0800
Linus Torvalds wrote:
> On Tue, 30 Jan 2024 at 14:56, Linus Torvalds
> wrote:
> >
> > With that, the base size of 'struct eventfs_inode' actually becomes 96
> > bytes for me.
>
> It can be shrunk some more.
>
> The field ordering is suboptimal. Pointers are
On Tue, 30 Jan 2024 at 14:56, Linus Torvalds
wrote:
>
> With that, the base size of 'struct eventfs_inode' actually becomes 96
> bytes for me.
It can be shrunk some more.
The field ordering is suboptimal. Pointers are 8 bytes and 8-byte
aligned, but 'struct kref' is just 4 bytes, and 'struct eve
On Tue, 30 Jan 2024 14:58:26 -0800
Linus Torvalds wrote:
> On Tue, 30 Jan 2024 at 14:55, Steven Rostedt wrote:
> >
> > Remember, the files don't have an ei allocated. Only directories.
>
> Crossed emails.
>
> > I then counted the length of the string - 7 bytes (which is the same as the
> > l
On Tue, 30 Jan 2024 at 14:55, Steven Rostedt wrote:
>
> Remember, the files don't have an ei allocated. Only directories.
Crossed emails.
> I then counted the length of the string - 7 bytes (which is the same as the
> length of the string plus the '\0' character - 8 bytes) to accommodate the
> p
On Tue, 30 Jan 2024 at 13:52, Linus Torvalds
wrote:
>
> Btw, you can look at name lengths in tracefs with something stupid like this:
>
> find . | sed 's:^.*/::' | tr -c '\n' . | sort | uniq -c
Actually, since only directories have these 'ei' fields, that find
should have "-type d", and then
On Tue, 30 Jan 2024 13:52:05 -0800
Linus Torvalds wrote:
> On Tue, 30 Jan 2024 at 12:55, Steven Rostedt wrote:
> >
> > I'm going to be putting back the ei->name pointer as the above actually
> > adds more memory usage.
>
> I did it mainly because I hate having multiple different allocation
>
On Tue, 30 Jan 2024 at 12:55, Steven Rostedt wrote:
>
> I'm going to be putting back the ei->name pointer as the above actually
> adds more memory usage.
I did it mainly because I hate having multiple different allocation
sites that then have to do that kref_init() etc individually, and once
ther
On Tue, 30 Jan 2024 11:03:54 -0800
Linus Torvalds wrote:
> -static void free_ei(struct eventfs_inode *ei)
> +static inline struct eventfs_inode *alloc_ei(const char *name)
> {
> - kfree_const(ei->name);
> - kfree(ei->d_children);
> - kfree(ei->entry_attrs);
> - kfree(ei);
> +
The eventfs inode had pointers to dentries (and child dentries) without
actually holding a refcount on said pointer. That is fundamentally
broken, and while eventfs tried to then maintain coherence with dentries
going away by hooking into the '.d_iput' callback, that doesn't actually
work since it
20 matches
Mail list logo
