Re: [PATCH] fs: Improve eventpoll logging to stop indicting timerfd

[Manish Varma]

Hi Thomas,

On Mon, Mar 22, 2021 at 2:40 PM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
>
> Manish,
>
> On Mon, Mar 22 2021 at 10:15, Manish Varma wrote:
> > On Thu, Mar 18, 2021 at 6:04 AM Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:
> >> > +static atomic_t instance_count = ATOMIC_INIT(0);
> >>
> >> instance_count is misleading as it does not do any accounting of
> >> instances as the name suggests.
> >>
> >
> > Not sure if I am missing a broader point here, but the objective of this
> > patch is to:
> > A. To help find the process a given timerfd associated with, and
> > B. one step further, if there are multiple fds created by a single
> > process then label each instance using monotonically increasing integer
> > i.e. "instance_count" to help identify each of them separately.
> >
> > So, instance_count in my mind helps with "B", i.e. to keep track and
> > identify each instance of timerfd individually.
>
> I know what you want to do. The point is that instance_count is the
> wrong name as it suggests that it counts instances, and that in most
> cases implies active instances.
>
> It's not a counter, it's a token generator which allows you to create
> unique ids. The fact that it is just incrementing once per created file
> descriptor does not matter. That's just an implementation detail.
>
> Name it something like timerfd_create_id or timerfd_session_id which
> clearly tells that this is not counting any thing. It immediately tells
> the purpose of generating an id.
>
> Naming matters when reading code, really.
>

Noted, and thanks for the clarification!

> >> > +     snprintf(file_name_buf, sizeof(file_name_buf), "[timerfd%d:%s]",
> >> > +              instance, task_comm_buf);
> >> > +     ufd = anon_inode_getfd(file_name_buf, &timerfd_fops, ctx,
> >> >                              O_RDWR | (flags & TFD_SHARED_FCNTL_FLAGS));
> >> >       if (ufd < 0)
> >> >               kfree(ctx);
> >>
> >> I actually wonder, whether this should be part of anon_inode_get*().
> >>
> >
> > I am curious (and open at the same time) if that will be helpful..
> > In the case of timerfd, I could see it adds up value by stuffing more
> > context to the file descriptor name as eventpoll is using the same file
> > descriptor names as wakesource name, and hence the cost of slightly
> > longer file descriptor name justifies. But I don't have a solid reason
> > if this additional cost (of longer file descriptor names) will be
> > helpful in general with other file descriptors.
>
> Obviously you want to make that depend on a flag handed to anon_...().

Unfortunately, changing file descriptor names does not seem to be a viable
option here (more details in my answer in the next section), and
hence changes in anon_...() does not seem to be required.

>
> The point is that there will be the next anonfd usecase which needs
> unique identification at some point. That is going to copy&pasta that
> timerfd code and then make it slightly different just because and then
> userspace needs to parse yet another format.
>
> >> Aside of that this is a user space visible change both for eventpoll and
> >> timerfd.
>
> Not when done right.
>
> >> Have you carefully investigated whether there is existing user space
> >> which might depend on the existing naming conventions?
> >>
> > I am not sure how I can confirm that for all userspace, but open for
> > suggestions if you can share some ideas.
> >
> > However, I have verified and can confirm for Android userspace that
> > there is no dependency on existing naming conventions for timerfd and
> > eventpoll wakesource names, if that helps.
>
> Well, there is a world outside Android and you're working for a company
> which should have tools to search for '[timerfd]' usage in a gazillion of
> projects. The obvious primary targets are distros of all sorts. I'm sure
> there are ways to figure this out without doing it manually.
>
> Not that I expect any real dependencies on it, but as always the devil
> is in the details.
>

Right, there are some userspace which depends on "[timerfd]" string
https://codesearch.debian.net/search?q=%22%5Btimerfd%5D%22&literal=1

So, modifying file descriptor names at-least for timerfd will definitely
break those.

With that said, I am now thinking about leaving alone the file descriptor
names as is, and instead, adding those extra information about the
associated processes (i.e. process name or rather PID of the
process) along with token ID directly into wakesource name, at the
time of creating new wakesource i.e. in ep_create_wakeup_source().

So, the wakesource names, that currently named as "[timerfd]", will be
named something like:
"epollitem<N>:<PID>.[timerfd]"

Where N is the number of wakesource created since boot.

This way we can still associate the process with the wakesource
name and also distinguish multiple instances of wakesources using
the integer identifier.

Please share your thoughts!

> Thanks,
>
>         tglx

Thanks,
Manish
--
Manish Varma | Software Engineer | varmam@xxxxxxxxxx | 650-686-0858