Page MenuHomeFreeBSD
Differential D40723

radix_trie: pass fewer params to node_get
ClosedPublic
Actions

Authored by dougm on Jun 23 2023, 8:50 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Mar 7, 9:07 AM
Unknown Object (File)
Feb 5 2025, 11:16 AM
Unknown Object (File)
Feb 3 2025, 10:11 PM
Unknown Object (File)
Jan 26 2025, 5:41 AM
Unknown Object (File)
Jan 24 2025, 9:53 PM
Unknown Object (File)
Jan 21 2025, 4:54 PM
Unknown Object (File)
Jan 21 2025, 8:09 AM
Unknown Object (File)
Jan 10 2025, 5:33 AM
View All 56 Files
Subscribers
imp

Details

Reviewers
alc
markj
Commits
rGda72505f9c6a: radix_trie: pass fewer params to node_get
Summary

Let node_get calculate it's own owner value. Don't pass the count parameter, since it's always 2. Save 16 bytes in insert(). Move, without modifying, slot and trimkey to handle use-before-declaration problem.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

dougm requested review of this revision.Jun 23 2023, 8:50 AM
dougm created this revision.
dougm updated this revision to Diff 123789.Jun 26 2023, 5:29 PM
dougm added a reviewer: markj.

This patch needs a refresh after the trimkey patch.

markj accepted this revision.Jun 27 2023, 2:00 PM
markj added inline comments.
sys/kern/subr_pctrie.c
101

Perhaps remove the extra newline here since you're moving this function anyway.

This revision is now accepted and ready to land.Jun 27 2023, 2:00 PM
Closed by commit rGda72505f9c6a: radix_trie: pass fewer params to node_get (authored by dougm). · Explain WhyJun 27 2023, 5:22 PM
This revision was automatically updated to reflect the committed changes.
dougm added a commit: rGda72505f9c6a: radix_trie: pass fewer params to node_get.
Herald added a subscriber: imp. · View Herald TranscriptJun 27 2023, 5:22 PM

Revision Contents
Changeset List

PathSize
sys/
kern/
48 lines
vm/
45 lines

Diff 123789

View Options

sys/kern/subr_pctrie.c

Show First 20 Lines • Show All 47 Lines • ▼ Show 20 Lines
.Fa fd .Fa fd
is a descriptor to be operated on by is a descriptor to be operated on by
.Fa cmd .Fa cmd
as described below. as described below.
Depending on the value of Depending on the value of
.Fa cmd , .Fa cmd ,
.Fn fcntl .Fn fcntl
can take an additional third argument can take an additional third argument
.Fa "long arg" . .Fa arg .
jhbUnsubmitted
Done
Inline Actions

Maybe just say the type of the 3rd arg depends on the operation (and document the type for each operation that takes the third arg) instead of trying to assume it's a single type? In CheriBSD we have a wrapper in libc to deal with this and have fcntl() as a truly var-args function. On existing platforms var-args is close enough to work for passing syscall arguments, but not all ABIs require this. (var-args are passed as a block in a bounded pointer on CHERI platforms and not just a continuation of register arguments).

jhb: Maybe just say the type of the 3rd arg depends on the operation (and document the type for each…
kibAuthorUnsubmitted
Done
Inline Actions

I pushed everything but this change, the review is updated with the man page changes.

kib: I pushed everything but this change, the review is updated with the man page changes.
Unless otherwise noted below for a specific operation,
emasteUnsubmitted
Done
Inline Actions

maybe "with type dependent on the specific operation"

or maybe

can take an additional third argument
.Fa arg.
Unless otherwise noted below for a specific operation,
.Fa arg
has type
.Vt int .
emaste: maybe "with type dependent on the specific operation" or maybe ``` can take an additional…
.Fa arg
has type
.Vt int .
.Bl -tag -width F_DUP2FD_CLOEXEC .Bl -tag -width F_DUP2FD_CLOEXEC
.It Dv F_DUPFD .It Dv F_DUPFD
Return a new descriptor as follows: Return a new descriptor as follows:
.Pp .Pp
.Bl -bullet -compact -offset 4n .Bl -bullet -compact -offset 4n
.It .It
Lowest numbered available descriptor greater than or equal to Lowest numbered available descriptor greater than or equal to
.Fa arg . .Fa arg .
▲ Show 20 Lines • Show All 121 Lines • ▼ Show 20 Lines
.It Dv F_GET_SEALS .It Dv F_GET_SEALS
Get seals associated with the file, if the underlying filesystem supports seals. Get seals associated with the file, if the underlying filesystem supports seals.
.It Dv F_ISUNIONSTACK .It Dv F_ISUNIONSTACK
Check if the vnode is part of a union stack (either the "union" flag from Check if the vnode is part of a union stack (either the "union" flag from
.Xr mount 2 .Xr mount 2
or unionfs). or unionfs).
This is a hack not intended to be used outside of libc. This is a hack not intended to be used outside of libc.
.It Dv F_KINFO .It Dv F_KINFO
Fills a Fills a
markjUnsubmitted
Done
Inline Actions
.It Dv F_KINFO
- Fills
+ Fills a
.Vt struct kinfo_file
markj:
.Vt struct kinfo_file .Vt struct kinfo_file
for the file referenced by the specified file descriptor. for the file referenced by the specified file descriptor.
The The
.Fa arg .Fa arg
argument should point to the storage for argument should point to the storage for
.Vt struct kinfo_file . .Vt struct kinfo_file .
The The
.Va kf_structsize .Va kf_structsize
▲ Show 20 Lines • Show All 78 Lines • ▼ Show 20 Lines
Seals are on a per-inode basis and require support by the underlying filesystem. Seals are on a per-inode basis and require support by the underlying filesystem.
If the underlying filesystem does not support seals, If the underlying filesystem does not support seals,
.Dv F_ADD_SEALS .Dv F_ADD_SEALS
and and
.Dv F_GET_SEALS .Dv F_GET_SEALS
will fail and return will fail and return
.Dv EINVAL . .Dv EINVAL .
.Pp .Pp
Several commands are available for doing advisory file locking; Several operations are available for doing advisory file locking;
emasteUnsubmitted
Done
Inline Actions

Is "command" or "operation" the better term, in general? I think we should be consistent between this sentence and the added text above/below

emaste: Is "command" or "operation" the better term, in general? I think we should be consistent…
they all operate on the following structure: they all operate on the following structure:
.Bd -literal .Bd -literal
struct flock { struct flock {
off_t l_start; /* starting offset */ off_t l_start; /* starting offset */
off_t l_len; /* len = 0 means until end of file */ off_t l_len; /* len = 0 means until end of file */
pid_t l_pid; /* lock owner */ pid_t l_pid; /* lock owner */
short l_type; /* lock type: read/write, etc. */ short l_type; /* lock type: read/write, etc. */
short l_whence; /* type of l_start */ short l_whence; /* type of l_start */
int l_sysid; /* remote system id or zero for local */ int l_sysid; /* remote system id or zero for local */
}; };
.Ed .Ed
These advisory file locking operations take a pointer to
emasteUnsubmitted
Done
Inline Actions

These commands take a pointer to struct flock as the third argument arg.
or
These advisory file locking commands take a pointer to struct flock as the third argument arg.

emaste: These commands take a pointer to struct flock as the third argument arg. or These advisory file…
emasteUnsubmitted
Done
Inline Actions

Maybe "operations" here too for consistency?
But from the context this one is clear so I don't mind either way.

emaste: Maybe "operations" here too for consistency? But from the context this one is clear so I don't…
.Vt struct flock
as the third argument
.Fa arg .
The commands available for advisory record locking are as follows: The commands available for advisory record locking are as follows:
.Bl -tag -width F_SETLKWX .Bl -tag -width F_SETLKWX
.It Dv F_GETLK .It Dv F_GETLK
Get the first lock that blocks the lock description pointed to by the Get the first lock that blocks the lock description pointed to by the
third argument, third argument,
.Fa arg , .Fa arg ,
taken as a pointer to a taken as a pointer to a
.Fa "struct flock" .Fa "struct flock"
▲ Show 20 Lines • Show All 477 Lines • Show Last 20 Lines
View Options

sys/vm/vm_radix.c

Show First 20 Lines • Show All 47 Lines • ▼ Show 20 Lines
.Fa fd .Fa fd
is a descriptor to be operated on by is a descriptor to be operated on by
.Fa cmd .Fa cmd
as described below. as described below.
Depending on the value of Depending on the value of
.Fa cmd , .Fa cmd ,
.Fn fcntl .Fn fcntl
can take an additional third argument can take an additional third argument
.Fa "long arg" . .Fa arg .
jhbUnsubmitted
Done
Inline Actions

Maybe just say the type of the 3rd arg depends on the operation (and document the type for each operation that takes the third arg) instead of trying to assume it's a single type? In CheriBSD we have a wrapper in libc to deal with this and have fcntl() as a truly var-args function. On existing platforms var-args is close enough to work for passing syscall arguments, but not all ABIs require this. (var-args are passed as a block in a bounded pointer on CHERI platforms and not just a continuation of register arguments).

jhb: Maybe just say the type of the 3rd arg depends on the operation (and document the type for each…
kibAuthorUnsubmitted
Done
Inline Actions

I pushed everything but this change, the review is updated with the man page changes.

kib: I pushed everything but this change, the review is updated with the man page changes.
Unless otherwise noted below for a specific operation,
emasteUnsubmitted
Done
Inline Actions

maybe "with type dependent on the specific operation"

or maybe

can take an additional third argument
.Fa arg.
Unless otherwise noted below for a specific operation,
.Fa arg
has type
.Vt int .
emaste: maybe "with type dependent on the specific operation" or maybe ``` can take an additional…
.Fa arg
has type
.Vt int .
.Bl -tag -width F_DUP2FD_CLOEXEC .Bl -tag -width F_DUP2FD_CLOEXEC
.It Dv F_DUPFD .It Dv F_DUPFD
Return a new descriptor as follows: Return a new descriptor as follows:
.Pp .Pp
.Bl -bullet -compact -offset 4n .Bl -bullet -compact -offset 4n
.It .It
Lowest numbered available descriptor greater than or equal to Lowest numbered available descriptor greater than or equal to
.Fa arg . .Fa arg .
▲ Show 20 Lines • Show All 121 Lines • ▼ Show 20 Lines
.It Dv F_GET_SEALS .It Dv F_GET_SEALS
Get seals associated with the file, if the underlying filesystem supports seals. Get seals associated with the file, if the underlying filesystem supports seals.
.It Dv F_ISUNIONSTACK .It Dv F_ISUNIONSTACK
Check if the vnode is part of a union stack (either the "union" flag from Check if the vnode is part of a union stack (either the "union" flag from
.Xr mount 2 .Xr mount 2
or unionfs). or unionfs).
This is a hack not intended to be used outside of libc. This is a hack not intended to be used outside of libc.
.It Dv F_KINFO .It Dv F_KINFO
Fills a Fills a
markjUnsubmitted
Done
Inline Actions
.It Dv F_KINFO
- Fills
+ Fills a
.Vt struct kinfo_file
markj:
.Vt struct kinfo_file .Vt struct kinfo_file
for the file referenced by the specified file descriptor. for the file referenced by the specified file descriptor.
The The
.Fa arg .Fa arg
argument should point to the storage for argument should point to the storage for
.Vt struct kinfo_file . .Vt struct kinfo_file .
The The
.Va kf_structsize .Va kf_structsize
▲ Show 20 Lines • Show All 78 Lines • ▼ Show 20 Lines
Seals are on a per-inode basis and require support by the underlying filesystem. Seals are on a per-inode basis and require support by the underlying filesystem.
If the underlying filesystem does not support seals, If the underlying filesystem does not support seals,
.Dv F_ADD_SEALS .Dv F_ADD_SEALS
and and
.Dv F_GET_SEALS .Dv F_GET_SEALS
will fail and return will fail and return
.Dv EINVAL . .Dv EINVAL .
.Pp .Pp
Several commands are available for doing advisory file locking; Several operations are available for doing advisory file locking;
emasteUnsubmitted
Done
Inline Actions

Is "command" or "operation" the better term, in general? I think we should be consistent between this sentence and the added text above/below

emaste: Is "command" or "operation" the better term, in general? I think we should be consistent…
they all operate on the following structure: they all operate on the following structure:
.Bd -literal .Bd -literal
struct flock { struct flock {
off_t l_start; /* starting offset */ off_t l_start; /* starting offset */
off_t l_len; /* len = 0 means until end of file */ off_t l_len; /* len = 0 means until end of file */
pid_t l_pid; /* lock owner */ pid_t l_pid; /* lock owner */
short l_type; /* lock type: read/write, etc. */ short l_type; /* lock type: read/write, etc. */
short l_whence; /* type of l_start */ short l_whence; /* type of l_start */
int l_sysid; /* remote system id or zero for local */ int l_sysid; /* remote system id or zero for local */
}; };
.Ed .Ed
These advisory file locking operations take a pointer to
emasteUnsubmitted
Done
Inline Actions

These commands take a pointer to struct flock as the third argument arg.
or
These advisory file locking commands take a pointer to struct flock as the third argument arg.

emaste: These commands take a pointer to struct flock as the third argument arg. or These advisory file…
emasteUnsubmitted
Done
Inline Actions

Maybe "operations" here too for consistency?
But from the context this one is clear so I don't mind either way.

emaste: Maybe "operations" here too for consistency? But from the context this one is clear so I don't…
.Vt struct flock
as the third argument
.Fa arg .
The commands available for advisory record locking are as follows: The commands available for advisory record locking are as follows:
.Bl -tag -width F_SETLKWX .Bl -tag -width F_SETLKWX
.It Dv F_GETLK .It Dv F_GETLK
Get the first lock that blocks the lock description pointed to by the Get the first lock that blocks the lock description pointed to by the
third argument, third argument,
.Fa arg , .Fa arg ,
taken as a pointer to a taken as a pointer to a
.Fa "struct flock" .Fa "struct flock"
▲ Show 20 Lines • Show All 477 Lines • Show Last 20 Lines