Page MenuHomeFreeBSD
Differential D47231

netstat: convert routing statistics to a sysctl
ClosedPublic
Actions

Authored by kevans on Oct 22 2024, 3:49 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 9 2025, 8:33 AM
Unknown Object (File)
Feb 9 2025, 8:33 AM
Unknown Object (File)
Feb 9 2025, 8:32 AM
Unknown Object (File)
Feb 7 2025, 6:10 PM
Unknown Object (File)
Feb 4 2025, 12:43 AM
Unknown Object (File)
Jan 21 2025, 8:23 PM
Unknown Object (File)
Jan 14 2025, 2:41 AM
Unknown Object (File)
Jan 12 2025, 9:45 PM
View All 28 Files
Subscribers
ae
glebius
imp
zlei

Details

Reviewers
melifaro
markj
zlei
glebius
Group Reviewers
network
Commits
rGf3a097d0312c: netstat: switch to using the sysctl-exported stats for live stats
rG3360a15898ce: net: route: convert routing statistics to a sysctl
Summary

Exporting the relevant pcpustat is trivial, so let's do that. Use it
for netstat operation to avoid having to dig around in mem(4) for live
kernel statistics. Based on callers of kresolve_list(), this is the
last live path using mem(4) that could be functional today.

Tested both with netstat -rs and netstat -rs -M.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

kevans created this revision.Oct 22 2024, 3:49 AM
Herald added subscribers: glebius, ae, imp. · View Herald TranscriptOct 22 2024, 3:49 AM
kevans requested review of this revision.Oct 22 2024, 3:49 AM
Harbormaster completed remote builds in B60144: Diff 145330.Oct 22 2024, 3:49 AM
kevans added a child revision: D47210: netstat: strip the binary of sgid.Oct 22 2024, 3:52 AM
zlei added a subscriber: zlei.Oct 22 2024, 6:14 AM
zlei added inline comments.
sys/net/route.c
77

This line seems wrong to me. The following line 80~81 looks good.

kevans added inline comments.Oct 22 2024, 10:01 AM
sys/net/route.c
77

Whoops, forgot to clear out a stale version of the patch on the machine I send them from. Will update in a few hours- this line disappears, the two below remain.

kevans updated this revision to Diff 145344.EditedOct 22 2024, 1:40 PM

Fix the diff; locally, I split this up into

Author: Kyle Evans <kevans@FreeBSD.org>
Date:   Mon Oct 21 22:44:59 2024 -0500

    net: route: convert routing statistics to a sysctl
    
    Exporting the relevant pcpustat is trivial, so let's do that.  We will
    use it in a near-future change in netstat to avoid having to dig around
    in mem(4) for live kernel statistics.
    
    Differential Revision:  https://reviews.freebsd.org/D47231
Author: Kyle Evans <kevans@FreeBSD.org>
Date:   Tue Oct 22 08:33:35 2024 -0500

    netstat: switch to using the sysctl-exported stats for live stats
    
    Now that we export the relevant stats via the net.route.stats sysctl,
    switch to using that to avoid having to dig around in mem(4) for live
    kernel statistics.  Based on callers of kresolve_list(), this is the
    last live path using mem(4) that could be functional today.
    
    Tested both with `netstat -rs` and `netstat -rs -M`.
    
    Differential Revision:  https://reviews.freebsd.org/D47231

I'll commit the first one then wait a week or so to commit the second.

zlei accepted this revision as: zlei.Oct 22 2024, 1:59 PM

Looks good to me.

In D47231#1077100, @kevans wrote:

...

I'll commit the first one then wait a week or so to commit the second.

Why wait a week for the second commit ? Have it in main so it could be get tested as early as possible.

sys/net/route.c
80

A nit here. I'd prefer no period for the description of sysctl vars.

This revision is now accepted and ready to land.Oct 22 2024, 1:59 PM
kevans added a comment.Oct 22 2024, 2:09 PM
In D47231#1077112, @zlei wrote:

Looks good to me.

In D47231#1077100, @kevans wrote:

...

I'll commit the first one then wait a week or so to commit the second.

Why wait a week for the second commit ? Have it in main so it could be get tested as early as possible.

Just to be nicer to folks wanting to bisect and use netstat -rs for a little bit; you can still -M /dev/mem until we strip setgid in another change.

Also, I can probably get away with landing a small/quick sysctl during stabweek but probably better to be conservatie about its use.

markj accepted this revision as: markj.Oct 22 2024, 3:54 PM

I wonder if we want to add some spare fields to struct rtstat before making it part of the user ABI?

sys/net/route.c
80

+1

glebius accepted this revision as: glebius.Oct 22 2024, 5:06 PM
kevans added a comment.Oct 22 2024, 7:17 PM
In D47231#1077231, @markj wrote:

I wonder if we want to add some spare fields to struct rtstat before making it part of the user ABI?

Sure; maybe drop a uint64_t[3] in at the end to round up to a cool 14? Additions are fairly far and few between.

markj added a comment.Oct 22 2024, 8:15 PM
In D47231#1077402, @kevans wrote:
In D47231#1077231, @markj wrote:

I wonder if we want to add some spare fields to struct rtstat before making it part of the user ABI?

Sure; maybe drop a uint64_t[3] in at the end to round up to a cool 14? Additions are fairly far and few between.

I'd make it an even 16 but it's up to you. :)

kevans marked 2 inline comments as done.Oct 24 2024, 3:12 AM
Closed by commit rG3360a15898ce: net: route: convert routing statistics to a sysctl (authored by kevans). · Explain WhyOct 24 2024, 3:17 AM
This revision was automatically updated to reflect the committed changes.
kevans added a commit: rG3360a15898ce: net: route: convert routing statistics to a sysctl.
kevans added a commit: rGf3a097d0312c: netstat: switch to using the sysctl-exported stats for live stats.Oct 24 2024, 4:55 PM

Revision Contents
Changeset List

PathSize
sys/
net/
4 lines
usr.bin/
netstat/
7 lines
8 lines

Diff 145344

View Options

sys/net/route.c

Context not available.
#include <linux/dcache.h> #include <linux/dcache.h>
#include <linux/capability.h> #include <linux/capability.h>
#include <linux/wait_bit.h> #include <linux/wait_bit.h>
#include <linux/kernel.h>
#include <linux/mutex.h>
struct module; struct module;
struct kiocb; struct kiocb;
Context not available.
static inline int static inline int
simple_open(struct inode *inode, struct file *filp) simple_open(struct inode *inode, struct file *filp)
{ {
filp->private_data = inode->i_private;
return 0; return 0;
} }
Context not available.
return (-ESPIPE); return (-ESPIPE);
} }
#define default_llseek no_llseek
hselaskyUnsubmitted
Done
Inline Actions

Can you make this a static inline function, instead of using a macro?

static inline loff_t
default_llseek(....)

This avoids the macro, which may match more than it should.

hselasky: Can you make this a static inline function, instead of using a macro? static inline loff_t…
static inline loff_t static inline loff_t
noop_llseek(struct linux_file *file, loff_t offset, int whence) noop_llseek(struct linux_file *file, loff_t offset, int whence)
Context not available.
return (file->f_op->mmap(file, vma)); return (file->f_op->mmap(file, vma));
} }
static inline void
i_size_write(struct inode *inode, loff_t i_size)
{
/* inode->i_size = i_size; */
markjUnsubmitted
Done
Inline Actions

Why is this commented out?

markj: Why is this commented out?
hselaskyUnsubmitted
Done
Inline Actions

Please remove this comment.

hselasky: Please remove this comment.
}
/*
* simple_read_from_buffer: copy data from kernel-space origin
* buffer into user-space destination buffer
*
* @dest: destination buffer
* @read_size: number of bytes to be transferred
* @ppos: starting transfer position pointer
* @orig: origin buffer
* @buf_size: size of destination and origin buffers
*
* Return value:
* On success, total bytes copied with *ppos incremented accordingly.
* On failure, negative value.
*/
static inline ssize_t
simple_read_from_buffer(void __user *dest, size_t read_size, loff_t *ppos,
void *orig, size_t buf_size)
{
void *read_pos = ((char *) orig) + *ppos;
size_t buf_remain = buf_size - *ppos;
ssize_t num_read;
if (buf_remain < 0 || buf_remain > buf_size)
return -EINVAL;
if (read_size > buf_remain)
read_size = buf_remain;
/* copy_to_user returns number of bytes NOT read */
num_read = read_size - copy_to_user(dest, read_pos, read_size);
if (num_read == 0)
return -EFAULT;
*ppos += num_read;
return (num_read);
}
MALLOC_DECLARE(M_LSATTR);
#define DEFINE_SIMPLE_ATTRIBUTE(__fops, __get, __set, __fmt) \
static inline int \
__fops ## _open(struct inode *inode, struct file *filp) \
{ \
return (simple_attr_open(inode, filp, __get, __set, __fmt)); \
} \
static const struct file_operations __fops = { \
.owner = THIS_MODULE, \
.open = __fops ## _open, \
.release = simple_attr_release, \
.read = simple_attr_read, \
.write = simple_attr_write, \
.llseek = no_llseek \
}
int simple_attr_open(struct inode *inode, struct file *filp,
int (*get)(void *, uint64_t *), int (*set)(void *, uint64_t),
const char *fmt);
int simple_attr_release(struct inode *inode, struct file *filp);
ssize_t simple_attr_read(struct file *filp, const char *buf, size_t read_size, loff_t *ppos);
ssize_t simple_attr_write(struct file *filp, const char *buf, size_t write_size, loff_t *ppos);
#endif /* _LINUXKPI_LINUX_FS_H_ */ #endif /* _LINUXKPI_LINUX_FS_H_ */
Context not available.
View Options

usr.bin/netstat/main.c

Context not available.
#include <linux/dcache.h> #include <linux/dcache.h>
#include <linux/capability.h> #include <linux/capability.h>
#include <linux/wait_bit.h> #include <linux/wait_bit.h>
#include <linux/kernel.h>
#include <linux/mutex.h>
struct module; struct module;
struct kiocb; struct kiocb;
Context not available.
static inline int static inline int
simple_open(struct inode *inode, struct file *filp) simple_open(struct inode *inode, struct file *filp)
{ {
filp->private_data = inode->i_private;
return 0; return 0;
} }
Context not available.
return (-ESPIPE); return (-ESPIPE);
} }
#define default_llseek no_llseek
hselaskyUnsubmitted
Done
Inline Actions

Can you make this a static inline function, instead of using a macro?

static inline loff_t
default_llseek(....)

This avoids the macro, which may match more than it should.

hselasky: Can you make this a static inline function, instead of using a macro? static inline loff_t…
static inline loff_t static inline loff_t
noop_llseek(struct linux_file *file, loff_t offset, int whence) noop_llseek(struct linux_file *file, loff_t offset, int whence)
Context not available.
return (file->f_op->mmap(file, vma)); return (file->f_op->mmap(file, vma));
} }
static inline void
i_size_write(struct inode *inode, loff_t i_size)
{
/* inode->i_size = i_size; */
markjUnsubmitted
Done
Inline Actions

Why is this commented out?

markj: Why is this commented out?
hselaskyUnsubmitted
Done
Inline Actions

Please remove this comment.

hselasky: Please remove this comment.
}
/*
* simple_read_from_buffer: copy data from kernel-space origin
* buffer into user-space destination buffer
*
* @dest: destination buffer
* @read_size: number of bytes to be transferred
* @ppos: starting transfer position pointer
* @orig: origin buffer
* @buf_size: size of destination and origin buffers
*
* Return value:
* On success, total bytes copied with *ppos incremented accordingly.
* On failure, negative value.
*/
static inline ssize_t
simple_read_from_buffer(void __user *dest, size_t read_size, loff_t *ppos,
void *orig, size_t buf_size)
{
void *read_pos = ((char *) orig) + *ppos;
size_t buf_remain = buf_size - *ppos;
ssize_t num_read;
if (buf_remain < 0 || buf_remain > buf_size)
return -EINVAL;
if (read_size > buf_remain)
read_size = buf_remain;
/* copy_to_user returns number of bytes NOT read */
num_read = read_size - copy_to_user(dest, read_pos, read_size);
if (num_read == 0)
return -EFAULT;
*ppos += num_read;
return (num_read);
}
MALLOC_DECLARE(M_LSATTR);
#define DEFINE_SIMPLE_ATTRIBUTE(__fops, __get, __set, __fmt) \
static inline int \
__fops ## _open(struct inode *inode, struct file *filp) \
{ \
return (simple_attr_open(inode, filp, __get, __set, __fmt)); \
} \
static const struct file_operations __fops = { \
.owner = THIS_MODULE, \
.open = __fops ## _open, \
.release = simple_attr_release, \
.read = simple_attr_read, \
.write = simple_attr_write, \
.llseek = no_llseek \
}
int simple_attr_open(struct inode *inode, struct file *filp,
int (*get)(void *, uint64_t *), int (*set)(void *, uint64_t),
const char *fmt);
int simple_attr_release(struct inode *inode, struct file *filp);
ssize_t simple_attr_read(struct file *filp, const char *buf, size_t read_size, loff_t *ppos);
ssize_t simple_attr_write(struct file *filp, const char *buf, size_t write_size, loff_t *ppos);
#endif /* _LINUXKPI_LINUX_FS_H_ */ #endif /* _LINUXKPI_LINUX_FS_H_ */
Context not available.
View Options

usr.bin/netstat/route.c

Context not available.
#include <linux/dcache.h> #include <linux/dcache.h>
#include <linux/capability.h> #include <linux/capability.h>
#include <linux/wait_bit.h> #include <linux/wait_bit.h>
#include <linux/kernel.h>
#include <linux/mutex.h>
struct module; struct module;
struct kiocb; struct kiocb;
Context not available.
static inline int static inline int
simple_open(struct inode *inode, struct file *filp) simple_open(struct inode *inode, struct file *filp)
{ {
filp->private_data = inode->i_private;
return 0; return 0;
} }
Context not available.
return (-ESPIPE); return (-ESPIPE);
} }
#define default_llseek no_llseek
hselaskyUnsubmitted
Done
Inline Actions

Can you make this a static inline function, instead of using a macro?

static inline loff_t
default_llseek(....)

This avoids the macro, which may match more than it should.

hselasky: Can you make this a static inline function, instead of using a macro? static inline loff_t…
static inline loff_t static inline loff_t
noop_llseek(struct linux_file *file, loff_t offset, int whence) noop_llseek(struct linux_file *file, loff_t offset, int whence)
Context not available.
return (file->f_op->mmap(file, vma)); return (file->f_op->mmap(file, vma));
} }
static inline void
i_size_write(struct inode *inode, loff_t i_size)
{
/* inode->i_size = i_size; */
markjUnsubmitted
Done
Inline Actions

Why is this commented out?

markj: Why is this commented out?
hselaskyUnsubmitted
Done
Inline Actions

Please remove this comment.

hselasky: Please remove this comment.
}
/*
* simple_read_from_buffer: copy data from kernel-space origin
* buffer into user-space destination buffer
*
* @dest: destination buffer
* @read_size: number of bytes to be transferred
* @ppos: starting transfer position pointer
* @orig: origin buffer
* @buf_size: size of destination and origin buffers
*
* Return value:
* On success, total bytes copied with *ppos incremented accordingly.
* On failure, negative value.
*/
static inline ssize_t
simple_read_from_buffer(void __user *dest, size_t read_size, loff_t *ppos,
void *orig, size_t buf_size)
{
void *read_pos = ((char *) orig) + *ppos;
size_t buf_remain = buf_size - *ppos;
ssize_t num_read;
if (buf_remain < 0 || buf_remain > buf_size)
return -EINVAL;
if (read_size > buf_remain)
read_size = buf_remain;
/* copy_to_user returns number of bytes NOT read */
num_read = read_size - copy_to_user(dest, read_pos, read_size);
if (num_read == 0)
return -EFAULT;
*ppos += num_read;
return (num_read);
}
MALLOC_DECLARE(M_LSATTR);
#define DEFINE_SIMPLE_ATTRIBUTE(__fops, __get, __set, __fmt) \
static inline int \
__fops ## _open(struct inode *inode, struct file *filp) \
{ \
return (simple_attr_open(inode, filp, __get, __set, __fmt)); \
} \
static const struct file_operations __fops = { \
.owner = THIS_MODULE, \
.open = __fops ## _open, \
.release = simple_attr_release, \
.read = simple_attr_read, \
.write = simple_attr_write, \
.llseek = no_llseek \
}
int simple_attr_open(struct inode *inode, struct file *filp,
int (*get)(void *, uint64_t *), int (*set)(void *, uint64_t),
const char *fmt);
int simple_attr_release(struct inode *inode, struct file *filp);
ssize_t simple_attr_read(struct file *filp, const char *buf, size_t read_size, loff_t *ppos);
ssize_t simple_attr_write(struct file *filp, const char *buf, size_t write_size, loff_t *ppos);
#endif /* _LINUXKPI_LINUX_FS_H_ */ #endif /* _LINUXKPI_LINUX_FS_H_ */
Context not available.