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

[Thomas Gleixner <tglx@linutronix.de>]

Manish,

On Mon, Mar 01 2021 at 19:49, Manish Varma wrote:

> All together, that will give us names like the following:
>
> 1) timerfd file descriptor: [timerfd14:system_server]
> 2) eventpoll top-level per-process wakesource: epoll:system_server
> 3) eventpoll-on-timerfd per-descriptor wakesource:
> epollitem:system_server.[timerfd14:system_server]

All together that should be splitted up into a change to eventpoll and
timerfd.

> diff --git a/fs/timerfd.c b/fs/timerfd.c
> index c5509d2448e3..4249e8c9a38c 100644
> --- a/fs/timerfd.c
> +++ b/fs/timerfd.c
> @@ -46,6 +46,8 @@ struct timerfd_ctx {
>  	bool might_cancel;
>  };
>  
> +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.

>  static LIST_HEAD(cancel_list);
>  static DEFINE_SPINLOCK(cancel_lock);
>  
> @@ -391,6 +393,9 @@ SYSCALL_DEFINE2(timerfd_create, int, clockid, int, flags)
>  {
>  	int ufd;
>  	struct timerfd_ctx *ctx;
> +	char task_comm_buf[sizeof(current->comm)];
> +	char file_name_buf[32];
> +	int instance;
>  
>  	/* Check the TFD_* constants for consistency.  */
>  	BUILD_BUG_ON(TFD_CLOEXEC != O_CLOEXEC);
> @@ -427,7 +432,11 @@ SYSCALL_DEFINE2(timerfd_create, int, clockid, int, flags)
>  
>  	ctx->moffs = ktime_mono_to_real(0);
>  
> -	ufd = anon_inode_getfd("[timerfd]", &timerfd_fops, ctx,
> +	instance = atomic_inc_return(&instance_count);
> +	get_task_comm(task_comm_buf, current);

How is current->comm supposed to be unique? And with a wrapping counter
like the above you can end up with identical file descriptor names.

What's wrong with simply using the PID which is guaranteed to be unique
for the life time of a process/task?

> +	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*().

Aside of that this is a user space visible change both for eventpoll and
timerfd.

Have you carefully investigated whether there is existing user space
which might depend on the existing naming conventions?

The changelog is silent about this...

Thanks,

        tglx