eventfd: Rename `struct eventfd` to `struct eventfd_ctx`

[dumbbell]

Summary

This is the name of the structure on Linux. I'm about to expose it through linuxkpi as an opaque type. Therefore it allows to use it directly without further compatibility code in linuxkpi.

This structure is otherwise internal to sys/kern/sys_eventfd.c and not used anywhere else. Thus it is safe to change its name without breaking anything.

This is a requirement to the following patch to drm-kmod:
https://github.com/freebsd/drm-kmod/pull/358

Sponsored by: The FreeBSD Foundation

[emaste]

Hmm, OK. The struct is different and we might end up with a slightly puzzling build failure if things end up cross-threaded, but I suppose it's not really different than the status quo.

[dumbbell]

In D50848#1164156, @emaste wrote:

Hmm, OK. The struct is different and we might end up with a slightly puzzling build failure if things end up cross-threaded, but I suppose it's not really different than the status quo.

What do you mean by "cross-threaded"?

[emaste]

What do you mean by "cross-threaded"?

Sorry, it's a colloquialism. I mean that if we somehow end up including Linuxy and FreeBSD headers in the same context we could have an unclear view of what struct eventfd_ctx looks like. But yeah, if it's only ever used as an opaque pointer in Linux code it won't be an issue.

[christos]

Linux defines eventfd_ctx as:

struct eventfd_ctx {
	struct kref kref;
	wait_queue_head_t wqh;
	/*
	 * Every time that a write(2) is performed on an eventfd, the
	 * value of the __u64 being written is added to "count" and a
	 * wakeup is performed on "wqh". If EFD_SEMAPHORE flag was not
	 * specified, a read(2) will return the "count" value to userspace,
	 * and will reset "count" to zero. The kernel side eventfd_signal()
	 * also, adds to the "count" counter and issue a wakeup.
	 */
	__u64 count;
	unsigned int flags;
	int id;
};

This is quite different from our version. Although ugly, wouldn't it be safer to either explicitly alias it in LinuxKPI or even translate it there?

[emaste]

This is quite different from our version.

Yes, this is what I was commenting on as well, although it doesn't really matter -- this eventually gets used in D50853, which only uses it as an opaque type struct eventfd_ctx. Then LinuxKPI just passes the pointer through its calls to native FreeBSD functions e,g, eventfd_signal to eventfd_signnal_mask.

We could instead just do something like, in LinuxKPI:

struct evenfd;
struct efentfd_ctx;

static inline void
eventfd_signal(struct eventfd_ctx *ctx)
{
	eventfd_signal_mask((struct eventfd *)ctx, 0);
}

But renaming the FreeBSD struct maybe makes things a tiny bit clearer and I don't object, but I'll see if @kib @imp or @jhb have an opinion.

[kib]

It is really eventfd data, so might be slightly better to call it eventfd_data. I do not object to proposal to rename the FreeBSD own structure,

[bz]

I am a bit confused about the comments when scrolling through this stack.

I would highly suggest to not use overlapping names if they are different.
Just because in LinuxKPI it is currently used in an opaque way does not mean it will stay like this forever if the Linux version of the struct has (different) fields.

But if I get it right, the idea is to expose the native implementation with the same struct name directly to LinuxKPI?
If that is the case I would strongly say "NO"!

Everywhere where we had (have) native types for something for direct use in LinuxKPI which is not and will not be 100% 1:1 (let me call these "shortcuts"), we ran into problems: netdev was netif, page is vm_page, ... I "un-twisted" too many of these over the last years (or still have to like USB). Even when I tried to do this in wireless for a simple enum I ended up reverting it and making it two different names in LinuxKPI and native. It may work if it's a simple struct that gets introduced to both worlds at the same time which, say, describes some protocol fields which are baked in stone, but otherwise, no.
I just leave this here for the stack.

[emaste]

It is used only as an opaque pointer in LinuxKPI code. If we want to ensure the native struct is not exposed in LinuxKPI we'll just end up with a bunch of casts. Your concern is the same thing I was getting at with the cross-threaded comment above -- right now it is fine, but is a potential future issue.

[bz]

In D50848#1238144, @emaste wrote:

It is used only as an opaque pointer in LinuxKPI code. If we want to ensure the native struct is not exposed in LinuxKPI we'll just end up with a bunch of casts. Your concern is the same thing I was getting at with the cross-threaded comment above -- right now it is fine, but is a potential future issue.

Can we avoid the potential future issue by making sure the native struct now does not get called the same way the Linux struct is called? I'd rather have a hack inside LinuxKPI for now with a comment rather than a headache in the future. So if your prototype + simple cast is really all that it is, it has my vote.

[dumbbell]

Thank you for all your feedbacks!

I implemented the approach suggested by @emaste:

• struct eventfd is not renamed
• The linuxkpi code casts between struct eventfd and struct eventfd_ctx

I will abandon this revision. Patches in D50849, D50850 and D50853 were refreshed accordingly.

[dumbbell]

This patch was superseded by the refactoring of D50849 and D50850.