Messages in this thread | | | Date | Tue, 18 Jan 2022 11:29:54 +0100 | Subject | Re: [PATCH 3/3] status: Display an informational message when the VSTATUS character is pressed or TIOCSTAT ioctl is called. | From | Jiri Slaby <> |
| |
Cc Oleg, scheduler and mm guys.
Hi,
The processes and their mm handling don't look right to me, but I don't know that area that well.
Overall, is this really worth the hassle?
BTW I haven't received 2/3.
On 18. 01. 22, 5:43, Walt Drummond wrote: > When triggered by pressing the VSTATUS key or calling the TIOCSTAT > ioctl, the n_tty line discipline will display a message on the user's > tty that provides basic information about the system and an > 'interesting' process in the current foreground process group, eg: > > load: 0.58 cmd: sleep 744474 [sleeping] 0.36r 0.00u 0.00s 0% 772k > > The status message provides: > - System load average > - Command name and process id (from the perspective of the session) > - Scheduler state > - Total wall-clock run time > - User space run time > - System space run time > - Percentage of on-cpu time > - Resident set size > > The message is only displayed when the tty has the VSTATUS character > set and the local flag NOKERNINFO disabled; it is always displayed > when TIOCSTAT is called regardless of tty settings. > > Signed-off-by: Walt Drummond <walt@drummond.us> > --- > drivers/tty/Makefile | 2 +- > drivers/tty/n_tty.c | 37 +++++++++ > drivers/tty/n_tty_status.c | 162 +++++++++++++++++++++++++++++++++++++ > drivers/tty/tty_io.c | 2 +- > include/linux/tty.h | 123 ++++++++++++++-------------- > 5 files changed, 265 insertions(+), 61 deletions(-) > create mode 100644 drivers/tty/n_tty_status.c > > diff --git a/drivers/tty/Makefile b/drivers/tty/Makefile > index 07aca5184a55..84bc99aebcff 100644 > --- a/drivers/tty/Makefile > +++ b/drivers/tty/Makefile > @@ -2,7 +2,7 @@ > obj-$(CONFIG_TTY) += tty_io.o n_tty.o tty_ioctl.o tty_ldisc.o \ > tty_buffer.o tty_port.o tty_mutex.o \ > tty_ldsem.o tty_baudrate.o tty_jobctrl.o \ > - n_null.o > + n_null.o n_tty_status.o > obj-$(CONFIG_LEGACY_PTYS) += pty.o > obj-$(CONFIG_UNIX98_PTYS) += pty.o > obj-$(CONFIG_AUDIT) += tty_audit.o > diff --git a/drivers/tty/n_tty.c b/drivers/tty/n_tty.c > index 6a6e7da80095..2e9b038e84e0 100644 > --- a/drivers/tty/n_tty.c > +++ b/drivers/tty/n_tty.c > @@ -80,6 +80,7 @@ > #define ECHO_BLOCK 256 > #define ECHO_DISCARD_WATERMARK N_TTY_BUF_SIZE - (ECHO_BLOCK + 32) > > +#define STATUS_LINE_LEN 160 /* tty status line will truncate at this length */ > > #undef N_TTY_TRACE > #ifdef N_TTY_TRACE > @@ -127,6 +128,8 @@ struct n_tty_data { > struct mutex output_lock; > }; > > +static int n_tty_status(struct tty_struct *tty); > + > #define MASK(x) ((x) & (N_TTY_BUF_SIZE - 1)) > > static inline size_t read_cnt(struct n_tty_data *ldata) > @@ -1334,6 +1337,11 @@ static void n_tty_receive_char_special(struct tty_struct *tty, unsigned char c) > commit_echoes(tty); > return; > } > + if (c == STATUS_CHAR(tty)) { > + if (!L_NOKERNINFO(tty)) > + n_tty_status(tty); > + return; > + } > if (c == '\n') { > if (L_ECHO(tty) || L_ECHONL(tty)) { > echo_char_raw('\n', ldata); > @@ -1763,6 +1771,7 @@ static void n_tty_set_termios(struct tty_struct *tty, struct ktermios *old) > set_bit(EOF_CHAR(tty), ldata->char_map); > set_bit('\n', ldata->char_map); > set_bit(EOL_CHAR(tty), ldata->char_map); > + set_bit(STATUS_CHAR(tty), ldata->char_map); > if (L_IEXTEN(tty)) { > set_bit(WERASE_CHAR(tty), ldata->char_map); > set_bit(LNEXT_CHAR(tty), ldata->char_map); > @@ -2412,6 +2421,29 @@ static unsigned long inq_canon(struct n_tty_data *ldata) > return nr; > } > > +static int n_tty_status(struct tty_struct *tty) > +{ > + struct n_tty_data *ldata = tty->disc_data; > + char *buf, msg[STATUS_LINE_LEN] = {0};
160 B on stack?
> + int ret = 0; > + size_t len = STATUS_LINE_LEN - 1; > + > + buf = (char *) &msg; > + ret = n_tty_get_status(tty, buf + 1, &len); > + if (ret) > + return ret; > + > + if (ldata->column != 0) { > + msg[0] = '\n'; > + len++;
It's not clear to me why this is after n_tty_get_status and therefore you need buf? If you stored \n to msg[0], you could just pass msg (to rewrite \n) or (msg + 1) to n_tty_get_status() -- depending on ldata->column, right?
> + } else { > + buf++; > + } > + > + do_n_tty_write(tty, NULL, buf, len); > + return 0; > +} > + > static int n_tty_ioctl(struct tty_struct *tty, struct file *file, > unsigned int cmd, unsigned long arg) > { > @@ -2429,6 +2461,11 @@ static int n_tty_ioctl(struct tty_struct *tty, struct file *file, > retval = read_cnt(ldata); > up_write(&tty->termios_rwsem); > return put_user(retval, (unsigned int __user *) arg); > + case TIOCSTAT: > + down_read(&tty->termios_rwsem); > + retval = n_tty_status(tty); > + up_read(&tty->termios_rwsem); > + return retval; > default: > return n_tty_ioctl_helper(tty, cmd, arg); > } > diff --git a/drivers/tty/n_tty_status.c b/drivers/tty/n_tty_status.c > new file mode 100644 > index 000000000000..269776db0640 > --- /dev/null > +++ b/drivers/tty/n_tty_status.c > @@ -0,0 +1,162 @@ > +// SPDX-License-Identifier: GPL-1.0+ > +/* > + * n_tty_status.c --- implements VSTATUS and TIOCSTAT from BSD > + * > + * Display a basic status message containing information about the > + * foreground process and system load on the users tty, triggered by > + * the VSTATUS character or TIOCSTAT. Ex, > + * > + * load: 14.11 cmd: tcsh 19623 [running] 185756.62r 88.00u 17.50s 0% 4260k > + * > + */ > + > +#include <linux/tty.h> > +#include <linux/mm.h> > +#include <linux/sched/loadavg.h> > + > +static const char * const task_state_unknown = "unknown";
Why do you need a variable for this, actually? Plus why do you need also a pointer (another 4/8B) to the storage?
> +static const char * const task_state_array[] = { > + "running", > + "sleeping", > + "disk sleep", > + "stopped", > + "tracing stop", > + "dead", > + "zombie", > + "parked", > + "idle", > +}; > + > +static inline unsigned long getRSSk(struct mm_struct *mm) > +{ > + if (mm == NULL) > + return 0; > + return get_mm_rss(mm) * PAGE_SIZE / 1024; > +} > + > +static inline long frac_sec(long l)
I don't understand what this does. The name should be more descriptive.
> +{ > + l /= NSEC_PER_MSEC * 10; > + if (l < 10) > + l *= 10; > + return l; > +} > + > +static inline struct task_struct *compare(struct task_struct *new, > + struct task_struct *old) > +{ > + unsigned int ostate, nstate; > + > + if (old == NULL) > + return new; > + > + ostate = task_state_index(old); > + nstate = task_state_index(new); > + > + if (ostate == nstate) { > + if (old->start_time > new->start_time) > + return old; > + return new; > + } > + > + if (ostate < nstate) > + return old; > + > + return new; > +} > + > +static struct task_struct *pick_process(struct pid *pgrp) > +{ > + struct task_struct *new, *winner = NULL; > + > + read_lock(&tasklist_lock); > + do_each_pid_task(pgrp, PIDTYPE_PGID, new) { > + winner = compare(new, winner); > + } while_each_pid_task(pgrp, PIDTYPE_PGID, new);
Whys is get_task_struct() not needed?
> + read_unlock(&tasklist_lock);
IOW what happens if winner has just died?
> + return winner; > +} > + > +/* We want the pid from the context of session */ > +static inline pid_t __get_pid(struct task_struct *tsk, struct tty_struct *tty) > +{ > + return __task_pid_nr_ns(tsk, PIDTYPE_PID, ns_of_pid(tty->ctrl.session));
You're holding no locks protecting tty->ctrl.session.
> +} > + > +static inline const char *get_task_state_name(struct task_struct *tsk)
This definitely doesn't belong here. How do you ensure it matches the returned index also in the future. Put it along with task_index_to_char()? Or simply use task_state_to_char()?
> +{ > + > + int index; > + > + index = task_state_index(tsk); > + if (index > ARRAY_SIZE(task_state_array))
Should be >=, or?
> + return task_state_unknown; > + return task_state_array[index]; > +} > + > +int n_tty_get_status(struct tty_struct *tty, char *msg, size_t *msglen) > +{ > + unsigned long loadavg[3]; > + uint64_t pcpu, cputime, wallclock; > + struct task_struct *p; > + struct rusage rusage; > + struct timespec64 utime, stime, rtime; > + char tname[TASK_COMM_LEN];
How much stack did you consume in sum with its caller n_tty_status()?
> + size_t len; > + > + if (tty == NULL) > + return -ENOTTY;
How can this happen?
> + get_avenrun(loadavg, FIXED_1/200, 0); > + len = scnprintf(msg + len, *msglen - len, "load: %lu.%02lu ", > + LOAD_INT(loadavg[0]), LOAD_FRAC(loadavg[0])); > + > + if (tty->ctrl.session == NULL) { > + len += scnprintf(msg + len, *msglen - len, > + "not a controlling terminal\n"); > + goto out; > + } > + > + if (tty->ctrl.pgrp == NULL) { > + len += scnprintf(msg + len, *msglen - len, > + "no foreground process group\n"); > + goto out; > + } > + > + p = pick_process(tty->ctrl.pgrp);
Why is no lock needed?
> + if (p == NULL) { > + len += scnprintf(msg + len, *msglen - len, > + "empty foreground process group\n"); > + goto out; > + } > + > + get_task_comm(tname, p); > + getrusage(p, RUSAGE_BOTH, &rusage); > + wallclock = ktime_get_ns() - p->start_time; > + > + utime.tv_sec = rusage.ru_utime.tv_sec; > + utime.tv_nsec = rusage.ru_utime.tv_usec * NSEC_PER_USEC; > + stime.tv_sec = rusage.ru_stime.tv_sec; > + stime.tv_nsec = rusage.ru_stime.tv_usec * NSEC_PER_USEC; > + rtime = ns_to_timespec64(wallclock); > + > + cputime = timespec64_to_ns(&utime) + timespec64_to_ns(&stime); > + pcpu = div64_u64(cputime * 100, wallclock); > + > + len += scnprintf(msg + len, *msglen - len, > + /* task, PID, task state */ > + "cmd: %s %d [%s] " > + /* rtime, utime, stime, %cpu, rss */ > + "%llu.%02lur %llu.%02luu %llu.%02lus %llu%% %luk\n", > + tname, __get_pid(p, tty), > + (char *)get_task_state_name(p), > + rtime.tv_sec, frac_sec(rtime.tv_nsec), > + utime.tv_sec, frac_sec(utime.tv_nsec), > + stime.tv_sec, frac_sec(stime.tv_nsec), > + pcpu, getRSSk(p->mm));
Why do you think p->mm is still alive (even after the getRSSk()'s check)? So no get_task_mm() needed?
> +out: > + *msglen = len; > + return 0; > +} > diff --git a/drivers/tty/tty_io.c b/drivers/tty/tty_io.c > index 6616d4a0d41d..f2f4f48ea502 100644 > --- a/drivers/tty/tty_io.c > +++ b/drivers/tty/tty_io.c > @@ -125,7 +125,7 @@ struct ktermios tty_std_termios = { /* for the benefit of tty drivers */ > .c_oflag = OPOST | ONLCR, > .c_cflag = B38400 | CS8 | CREAD | HUPCL, > .c_lflag = ISIG | ICANON | ECHO | ECHOE | ECHOK | > - ECHOCTL | ECHOKE | IEXTEN, > + ECHOCTL | ECHOKE | IEXTEN | NOKERNINFO, > .c_cc = INIT_C_CC, > .c_ispeed = 38400, > .c_ospeed = 38400, > diff --git a/include/linux/tty.h b/include/linux/tty.h > index 5dbd7c5afac7..e6ba6ce2efcb 100644 > --- a/include/linux/tty.h > +++ b/include/linux/tty.h > @@ -49,71 +49,73 @@ > #define WERASE_CHAR(tty) ((tty)->termios.c_cc[VWERASE]) > #define LNEXT_CHAR(tty) ((tty)->termios.c_cc[VLNEXT]) > #define EOL2_CHAR(tty) ((tty)->termios.c_cc[VEOL2]) > +#define STATUS_CHAR(tty) ((tty)->termios.c_cc[VSTATUS]) > > #define _I_FLAG(tty, f) ((tty)->termios.c_iflag & (f)) > #define _O_FLAG(tty, f) ((tty)->termios.c_oflag & (f)) > #define _C_FLAG(tty, f) ((tty)->termios.c_cflag & (f)) > #define _L_FLAG(tty, f) ((tty)->termios.c_lflag & (f)) > > -#define I_IGNBRK(tty) _I_FLAG((tty), IGNBRK) > -#define I_BRKINT(tty) _I_FLAG((tty), BRKINT) > -#define I_IGNPAR(tty) _I_FLAG((tty), IGNPAR) > -#define I_PARMRK(tty) _I_FLAG((tty), PARMRK) > -#define I_INPCK(tty) _I_FLAG((tty), INPCK) > -#define I_ISTRIP(tty) _I_FLAG((tty), ISTRIP) > -#define I_INLCR(tty) _I_FLAG((tty), INLCR) > -#define I_IGNCR(tty) _I_FLAG((tty), IGNCR) > -#define I_ICRNL(tty) _I_FLAG((tty), ICRNL) > -#define I_IUCLC(tty) _I_FLAG((tty), IUCLC) > -#define I_IXON(tty) _I_FLAG((tty), IXON) > -#define I_IXANY(tty) _I_FLAG((tty), IXANY) > -#define I_IXOFF(tty) _I_FLAG((tty), IXOFF) > -#define I_IMAXBEL(tty) _I_FLAG((tty), IMAXBEL) > -#define I_IUTF8(tty) _I_FLAG((tty), IUTF8) > - > -#define O_OPOST(tty) _O_FLAG((tty), OPOST) > -#define O_OLCUC(tty) _O_FLAG((tty), OLCUC) > -#define O_ONLCR(tty) _O_FLAG((tty), ONLCR) > -#define O_OCRNL(tty) _O_FLAG((tty), OCRNL) > -#define O_ONOCR(tty) _O_FLAG((tty), ONOCR) > -#define O_ONLRET(tty) _O_FLAG((tty), ONLRET) > -#define O_OFILL(tty) _O_FLAG((tty), OFILL) > -#define O_OFDEL(tty) _O_FLAG((tty), OFDEL) > -#define O_NLDLY(tty) _O_FLAG((tty), NLDLY) > -#define O_CRDLY(tty) _O_FLAG((tty), CRDLY) > -#define O_TABDLY(tty) _O_FLAG((tty), TABDLY) > -#define O_BSDLY(tty) _O_FLAG((tty), BSDLY) > -#define O_VTDLY(tty) _O_FLAG((tty), VTDLY) > -#define O_FFDLY(tty) _O_FLAG((tty), FFDLY) > - > -#define C_BAUD(tty) _C_FLAG((tty), CBAUD) > -#define C_CSIZE(tty) _C_FLAG((tty), CSIZE) > -#define C_CSTOPB(tty) _C_FLAG((tty), CSTOPB) > -#define C_CREAD(tty) _C_FLAG((tty), CREAD) > -#define C_PARENB(tty) _C_FLAG((tty), PARENB) > -#define C_PARODD(tty) _C_FLAG((tty), PARODD) > -#define C_HUPCL(tty) _C_FLAG((tty), HUPCL) > -#define C_CLOCAL(tty) _C_FLAG((tty), CLOCAL) > -#define C_CIBAUD(tty) _C_FLAG((tty), CIBAUD) > -#define C_CRTSCTS(tty) _C_FLAG((tty), CRTSCTS) > -#define C_CMSPAR(tty) _C_FLAG((tty), CMSPAR) > - > -#define L_ISIG(tty) _L_FLAG((tty), ISIG) > -#define L_ICANON(tty) _L_FLAG((tty), ICANON) > -#define L_XCASE(tty) _L_FLAG((tty), XCASE) > -#define L_ECHO(tty) _L_FLAG((tty), ECHO) > -#define L_ECHOE(tty) _L_FLAG((tty), ECHOE) > -#define L_ECHOK(tty) _L_FLAG((tty), ECHOK) > -#define L_ECHONL(tty) _L_FLAG((tty), ECHONL) > -#define L_NOFLSH(tty) _L_FLAG((tty), NOFLSH) > -#define L_TOSTOP(tty) _L_FLAG((tty), TOSTOP) > -#define L_ECHOCTL(tty) _L_FLAG((tty), ECHOCTL) > -#define L_ECHOPRT(tty) _L_FLAG((tty), ECHOPRT) > -#define L_ECHOKE(tty) _L_FLAG((tty), ECHOKE) > -#define L_FLUSHO(tty) _L_FLAG((tty), FLUSHO) > -#define L_PENDIN(tty) _L_FLAG((tty), PENDIN) > -#define L_IEXTEN(tty) _L_FLAG((tty), IEXTEN) > -#define L_EXTPROC(tty) _L_FLAG((tty), EXTPROC) > +#define I_IGNBRK(tty) _I_FLAG((tty), IGNBRK) > +#define I_BRKINT(tty) _I_FLAG((tty), BRKINT) > +#define I_IGNPAR(tty) _I_FLAG((tty), IGNPAR) > +#define I_PARMRK(tty) _I_FLAG((tty), PARMRK) > +#define I_INPCK(tty) _I_FLAG((tty), INPCK) > +#define I_ISTRIP(tty) _I_FLAG((tty), ISTRIP) > +#define I_INLCR(tty) _I_FLAG((tty), INLCR) > +#define I_IGNCR(tty) _I_FLAG((tty), IGNCR) > +#define I_ICRNL(tty) _I_FLAG((tty), ICRNL) > +#define I_IUCLC(tty) _I_FLAG((tty), IUCLC) > +#define I_IXON(tty) _I_FLAG((tty), IXON) > +#define I_IXANY(tty) _I_FLAG((tty), IXANY) > +#define I_IXOFF(tty) _I_FLAG((tty), IXOFF) > +#define I_IMAXBEL(tty) _I_FLAG((tty), IMAXBEL) > +#define I_IUTF8(tty) _I_FLAG((tty), IUTF8) > + > +#define O_OPOST(tty) _O_FLAG((tty), OPOST) > +#define O_OLCUC(tty) _O_FLAG((tty), OLCUC) > +#define O_ONLCR(tty) _O_FLAG((tty), ONLCR) > +#define O_OCRNL(tty) _O_FLAG((tty), OCRNL) > +#define O_ONOCR(tty) _O_FLAG((tty), ONOCR) > +#define O_ONLRET(tty) _O_FLAG((tty), ONLRET) > +#define O_OFILL(tty) _O_FLAG((tty), OFILL) > +#define O_OFDEL(tty) _O_FLAG((tty), OFDEL) > +#define O_NLDLY(tty) _O_FLAG((tty), NLDLY) > +#define O_CRDLY(tty) _O_FLAG((tty), CRDLY) > +#define O_TABDLY(tty) _O_FLAG((tty), TABDLY) > +#define O_BSDLY(tty) _O_FLAG((tty), BSDLY) > +#define O_VTDLY(tty) _O_FLAG((tty), VTDLY) > +#define O_FFDLY(tty) _O_FLAG((tty), FFDLY) > + > +#define C_BAUD(tty) _C_FLAG((tty), CBAUD) > +#define C_CSIZE(tty) _C_FLAG((tty), CSIZE) > +#define C_CSTOPB(tty) _C_FLAG((tty), CSTOPB) > +#define C_CREAD(tty) _C_FLAG((tty), CREAD) > +#define C_PARENB(tty) _C_FLAG((tty), PARENB) > +#define C_PARODD(tty) _C_FLAG((tty), PARODD) > +#define C_HUPCL(tty) _C_FLAG((tty), HUPCL) > +#define C_CLOCAL(tty) _C_FLAG((tty), CLOCAL) > +#define C_CIBAUD(tty) _C_FLAG((tty), CIBAUD) > +#define C_CRTSCTS(tty) _C_FLAG((tty), CRTSCTS) > +#define C_CMSPAR(tty) _C_FLAG((tty), CMSPAR) > + > +#define L_ISIG(tty) _L_FLAG((tty), ISIG) > +#define L_ICANON(tty) _L_FLAG((tty), ICANON) > +#define L_XCASE(tty) _L_FLAG((tty), XCASE) > +#define L_ECHO(tty) _L_FLAG((tty), ECHO) > +#define L_ECHOE(tty) _L_FLAG((tty), ECHOE) > +#define L_ECHOK(tty) _L_FLAG((tty), ECHOK) > +#define L_ECHONL(tty) _L_FLAG((tty), ECHONL) > +#define L_NOFLSH(tty) _L_FLAG((tty), NOFLSH) > +#define L_TOSTOP(tty) _L_FLAG((tty), TOSTOP) > +#define L_ECHOCTL(tty) _L_FLAG((tty), ECHOCTL) > +#define L_ECHOPRT(tty) _L_FLAG((tty), ECHOPRT) > +#define L_ECHOKE(tty) _L_FLAG((tty), ECHOKE) > +#define L_FLUSHO(tty) _L_FLAG((tty), FLUSHO) > +#define L_PENDIN(tty) _L_FLAG((tty), PENDIN) > +#define L_IEXTEN(tty) _L_FLAG((tty), IEXTEN) > +#define L_EXTPROC(tty) _L_FLAG((tty), EXTPROC) > +#define L_NOKERNINFO(tty) _L_FLAG((tty), NOKERNINFO)
Huh, no. Don't do this in this patch. It's unclear what you are actually doing here -- it's lost in all those whitespace or whatnot changes.
> struct device; > struct signal_struct; > @@ -388,6 +390,9 @@ void __init n_tty_init(void); > static inline void n_tty_init(void) { } > #endif > > +/* n_tty_status.c */ > +extern int n_tty_get_status(struct tty_struct *tty, char *msg, size_t *msglen);
Don't put any externs here to be consistent with the rest of the file.
> /* tty_audit.c */ > #ifdef CONFIG_AUDIT > void tty_audit_exit(void);
thanks, -- js suse labs
| |