Re: top(1): display uptime in HH:MM:SS format
On Fri, Sep 18, 2020 at 03:59:05PM -0500, Scott Cheloha wrote:
>
> [...]
>
> - An HH:MM:SS format uptime is useful in top(1). It's also more
> visually consistent with the local timestamp printed on the line
> above it, so it is easier to read at a glance.
>
> - The variable printing of "days" is annoying. I would rather it
> just told me "0 days" if it had been less than one day. It sucks
> when the information you want moves around or isn't shown at all.
> It's clever, sure, but I'd rather it be consistent.
>
> This patch changes the uptime format string to "up D days HH:MM:SS".
> The format string does not vary with the elapsed uptime. There is no
> inclusion/omission of the plural suffix depending on whether days is
> equal to one.
>
> [...]
Whoops, forgot about this one. September 18, 2020. What a time to be
alive.
Let's try this again. 98 week bump.
To recap, this patch makes the uptime formatting in top(1) produce
more constant-width results. The formatting is now always:
up D days HH:MM:SS
so only the day-count changes size. The day-count is also always
printed: if the machine has not been up for a full day it prints
up 0 days HH:MM:SS
For example, the upper lines on the top(1) running on my machine
currently look like this:
load averages: 0.29, 0.29, 0.27 jetsam.attlocal.net 18:12:16
82 processes: 81 idle, 1 on processorup 3 days 07:14:01
I have been running with this for almost two years and I love it.
I would like to commit it.
The only feedback I got when I originally posted this was that the
output formatting would no longer be the same as uptime(1)'s. I don't
think that matters very much. The person who offered the feedback
didn't think it mattered either, they were just hypothesizing
objections.
ok?
Index: display.c
===
RCS file: /cvs/src/usr.bin/top/display.c,v
retrieving revision 1.65
diff -u -p -r1.65 display.c
--- display.c 26 Aug 2020 16:21:28 - 1.65
+++ display.c 7 Aug 2022 23:14:25 -
@@ -208,31 +208,28 @@ display_init(struct statics * statics)
return (display_lines);
}
+/*
+ * Print the time elapsed since the system booted.
+ */
static void
format_uptime(char *buf, size_t buflen)
{
- time_t uptime;
- int days, hrs, mins;
struct timespec boottime;
+ time_t uptime;
+ unsigned int days, hrs, mins, secs;
+
+ if (clock_gettime(CLOCK_BOOTTIME, &boottime) == -1)
+ err(1, "clock_gettime");
- /*
-* Print how long system has been up.
-*/
- if (clock_gettime(CLOCK_BOOTTIME, &boottime) != -1) {
- uptime = boottime.tv_sec;
- uptime += 30;
- days = uptime / (3600 * 24);
- uptime %= (3600 * 24);
- hrs = uptime / 3600;
- uptime %= 3600;
- mins = uptime / 60;
- if (days > 0)
- snprintf(buf, buflen, "up %d day%s, %2d:%02d",
- days, days > 1 ? "s" : "", hrs, mins);
- else
- snprintf(buf, buflen, "up %2d:%02d",
- hrs, mins);
- }
+ uptime = boottime.tv_sec;
+ days = uptime / (3600 * 24);
+ uptime %= (3600 * 24);
+ hrs = uptime / 3600;
+ uptime %= 3600;
+ mins = uptime / 60;
+ secs = uptime % 60;
+ snprintf(buf, buflen, "up %u days %02u:%02u:%02u",
+ days, hrs, mins, secs);
}
Re: Split (*pr_usrreq)() by multiple handlers
On Mon, Aug 08, 2022 at 01:03:13AM +0300, Vitaliy Makkoveev wrote: > > I prefer the first idiom. If there is an error, do something. We > > should not change the style in opposite direction. This will prevent > > consistency. > > > > I???m not entirely understand you. When we have something to do in the > error path, I use "goto out??? and it works like the first idiom. Ah, sorry. I was speaking about the != 0. The break, return, goto is fine. error != 0 is just style and does not affect correctness. > >> - if (error) > >> + if (error != 0)
Re: Split (*pr_usrreq)() by multiple handlers
> On 7 Aug 2022, at 23:18, Alexander Bluhm wrote:
>
> On Sun, Aug 07, 2022 at 03:53:27AM +0300, Vitaliy Makkoveev wrote:
>> Please note, the `so_pcb' can't be NULL. We nullify it only on dead
>> socket, which should not be passed to (*pr_userreq)(). The newly created
>> socket has `so_pcb' set to NULL only until we pass it to (*pr_attach)()
>> and we don't use sockets if attach failed. So I use KASSERT() instead of
>> pcb != NULL check.
>
> For TCP this is not true. A reset packet in tcp_input() calls
> tcp_drop() -> tcp_close() -> in_pcbdetach() -> so->so_pcb = NULL
> This cannot happen in any TCP state. But KASSERT(inp != NULL) in
> tcp_disconnect() wrong. On the other hand if (tp == NULL) is not
> necessary everywhere.
Thanks, will fix it.
>
>> -if (error)
>> -break;
>
>> +if (error != 0)
>> +return (error);
>
> I prefer the first idiom. If there is an error, do something. We
> should not change the style in opposite direction. This will prevent
> consistency.
>
I’m not entirely understand you. When we have something to do in the
error path, I use "goto out” and it works like the first idiom.
+int
+tcp_connect(struct socket *so, struct mbuf *nam, struct proc *p)
+{
[ skip ]
+#ifdef INET6
+ if (inp->inp_flags & INP_IPV6) {
+ struct sockaddr_in6 *sin6;
+
+ if ((error = in6_nam2sin6(nam, &sin6)))
+ goto out;
+ if (IN6_IS_ADDR_UNSPECIFIED(&sin6->sin6_addr) ||
+ IN6_IS_ADDR_MULTICAST(&sin6->sin6_addr)) {
+ error = EINVAL;
+ goto out;
+ }
+ error = in6_pcbconnect(inp, nam);
+ } else
+#endif /* INET6 */
+ {
+ struct sockaddr_in *sin;
+
+ if ((error = in_nam2sin(nam, &sin)))
+ goto out;
+ if ((sin->sin_addr.s_addr == INADDR_ANY) ||
+ (sin->sin_addr.s_addr == INADDR_BROADCAST) ||
+ IN_MULTICAST(sin->sin_addr.s_addr) ||
+ in_broadcast(sin->sin_addr, inp->inp_rtableid)) {
+ error = EINVAL;
+ goto out;
+ }
+ error = in_pcbconnect(inp, nam);
+ }
+ if (error)
+ goto out;
+
[ skip ]
+
+out:
+ if (otp)
+ tcp_trace(TA_USER, ostate, tp, otp, NULL, PRU_CONNECT, 0);
+
+ return (error);
+}
But we also have the error paths where we have nothing to do. Do you
mean you prefer goto instead of return in the cases like this?
+int
+rip6_connect(struct socket *so, struct mbuf *nam, struct proc *p)
+{
[ skip ]
+ error = in6_nam2sin6(nam, &addr);
+ if (error != 0)
+ return (error);
+ /* Source address selection. XXX: need pcblookup? */
+ error = in6_pcbselsrc(&in6a, addr, in6p, in6p->inp_outputopts6);
+ if (error != 0)
+ return (error);
+
+ in6p->inp_laddr6 = *in6a;
+ in6p->inp_faddr6 = addr->sin6_addr;
+ soisconnected(so);
+
+ return (0);
+}
+int
+rip6_connect(struct socket *so, struct mbuf *nam, struct proc *p)
+{
[ skip ]
+ error = in6_nam2sin6(nam, &addr);
+ if (error != 0)
+ goto out;
+ /* Source address selection. XXX: need pcblookup? */
+ error = in6_pcbselsrc(&in6a, addr, in6p, in6p->inp_outputopts6);
+ if (error != 0)
+ goto out;
+
+ in6p->inp_laddr6 = *in6a;
+ in6p->inp_faddr6 = addr->sin6_addr;
+ soisconnected(so);
+
+out:
+ return (error);
+}
The error path of the original *_usrreq() is commonn for all commands
and contains mbufs release, because they could be wrongly passed for
all commands, so we silently release them to prevent memory leak:
foo_usrreq()
{
switch (req) {
case PRU_ACCEPT:
error = EOPNOTSUPP;
break;
}
if (req != PRU_RCVD && req != PRU_RCVOOB && req != PRU_SENSE) {
m_freem(control);
m_freem(m);
}
return (error);
}
But in this diff such mbufs can't be passed to the wrong handlers. So
in the cases like rip6_connect() the first idiom looks strange.
Re: Split (*pr_usrreq)() by multiple handlers
On Sun, Aug 07, 2022 at 03:53:27AM +0300, Vitaliy Makkoveev wrote: > Please note, the `so_pcb' can't be NULL. We nullify it only on dead > socket, which should not be passed to (*pr_userreq)(). The newly created > socket has `so_pcb' set to NULL only until we pass it to (*pr_attach)() > and we don't use sockets if attach failed. So I use KASSERT() instead of > pcb != NULL check. For TCP this is not true. A reset packet in tcp_input() calls tcp_drop() -> tcp_close() -> in_pcbdetach() -> so->so_pcb = NULL This cannot happen in any TCP state. But KASSERT(inp != NULL) in tcp_disconnect() wrong. On the other hand if (tp == NULL) is not necessary everywhere. > - if (error) > - break; > + if (error != 0) > + return (error); I prefer the first idiom. If there is an error, do something. We should not change the style in opposite direction. This will prevent consistency. Otherwise I think the diff is correct. bluhm
re(4): watchdog timeout and temporally disable MSI interrupt
Hi,
Sometimes I see "re0: watchdog timeout" message on my amd64 PC.
bios0: ASRock A88M-G/3.1
cpu0: AMD A10-7860K Radeon R7, 12 Compute Cores 4C+8G, 3593.88 MHz
re0 at pci3 dev 0 function 0 "Realtek 8168" rev 0x06: RTL8168E/8111E
(note: re0 is not A88M-G on-board controller, added PCIe board)
I suspect this caused by MSI interrupt so I tweaked if_re_pci.c like this:
--- if_re_pci.c~Sat Mar 12 03:00:48 2022
+++ if_re_pci.c Sat Jun 11 19:35:17 2022
@@ -159,7 +159,7 @@
}
/* Allocate interrupt */
- if (pci_intr_map_msi(pa, &ih) == 0)
+ if (/*pci_intr_map_msi(pa, &ih) ==*/ 0)
sc->rl_flags |= RL_FLAG_MSI;
else if (pci_intr_map(pa, &ih) != 0) {
printf(": couldn't map interrupt\n");
After this modify, I do not see "watchdog timeout".
I want to know how many people see this timeout of re(4) and
that is solved by this MSI tweak.
Or, please tell me if there is any other workarounds.
Regards,
--
SASANO Takayoshi (JG1UAA)
Re: mips64: trigger deferred timer interrupt from splx(9)
On Sun, Jul 31, 2022 at 01:28:18PM -0500, Scott Cheloha wrote: > Apparently mips64, i.e. octeon and loongson, has the same problem as > powerpc/macppc and powerpc64. The timer interrupt is normally only > logically masked, not physically masked in the hardware, when we're > running at or above IPL_CLOCK. If we arrive at cp0_int5() when the > clock interrupt is logically masked we postpone all work until the > next tick. This is a problem for my WIP clock interrupt work. I think the use of logical masking has been a design choice, not something dictated by the hardware. Physical masking should be possible, but some extra care would be needed to implement it, as the mips64 interrupt code is a bit clunky. > So, this patch is basically the same as what I did for macppc and what > I have proposed for powerpc64. > > - Add a new member, ci_timer_deferred, to mips64's cpu_info struct. > > While here, remove ci_pendingticks. We don't need it anymore. > > - If we get to cp0_int5() and our IPL is too high, set > cpu_info.ci_timer_deferred and return. > > - If we get to cp0_int5() and our IPL is low enough, clear > cpu_info.ci_timer_deferred and do clock interrupt work. > > - In splx(9), if the new IPL is low enough and cpu_info.ci_timer_deferred > is set, trigger the clock interrupt. > > The only big difference is that mips64 uses an equality comparison > when deciding whether to arm the timer interrupt, so it's really easy > to "miss" CP0.count when you're setting CP0.compare. > > To address this I've written a function, cp0_raise_int5(), that spins > until it is sure the timer interrupt will go off. The function needed > a bit of new code for reading CP0.cause, which I've added to > cp0access.S. I am using an initial offset of 16 cycles based on > experimentation with the machine I have access to, a 500Mhz CN50xx. > Values lower than 16 require more than one loop to arm the timer. If > that value is insufficient for other machines we can try passing the > initial offset as an argument to the function. It should not be necessary to make the initial offset variable. The offset is primarily a function of the length and content of the instruction sequence. Some unpredictability comes from cache misses and maybe branch prediction failures. > I wasn't sure where to put the prototype for cp0_raise_int5() so I > stuck it in mips64/cpu.h. If there's a better place for it, just say > so. Currently, mips64 clock.c is formulated as a proper driver. I think callers should not invoke its functions directly but use a hook instead. The MI mips64 code starts the clock through the md_startclock function pointer. Maybe there could be md_triggerclock. To reduce risk of confusion, I would rename cp0_raise_int5 to cp0_trigger_int5, as `raise' overlaps with the spl API. Also, ci_clock_deferred instead of ci_timer_deferred would look more consistent with the surrounding code.
slowcgi, httpd and fastcgi abnormal termination
I'm not sure httpd(8) handles correctly when the fastcgi application
(e.g. slowcgi) closes the connection prematurely.
To verify it, I'm playing with three simple CGI scripts running under
slowcgi with a very low timeout (-t2). The scripts test "hangs"
(which slowcgi turns into hard disconnections) in different places:
/* slow.c -- hang before replying */
sleep(10);
puts("Content-Type: text/plain\n");
puts("hello, world");
return 0;
/* slow2.c -- hang during headers */
puts("Content-Type: text/plain");
fflush(stdout);
sleep(10);
puts("");
puts("hello, world");
return 0;
/* slow3.c -- hang during the body */
puts("Content-Type: text/plain\n");
printf("hello, (wait for it...)");
fflush(stdout);
sleep(10);
puts("world!");
return 0;
Those sleep calls are meant to hit slowcgi_timeout which abruptly
closes the connection with httpd(8).
httpd handles the fastcgi termination via server_file_error which
assumes the request' headers and body were already sent and so goes on
to set up for the next request (if clt_persist.) This leaves the HTTP
client hanging and waiting for a response that won't arrive, until
they give up when the fastcgi reply wasn't complete.
Diff below (first three hunks) addresses this issue by introducing a
server_fcgi_error callback that handles the "no header" and
"incomplete data" cases before calling server_file_error. With it,
the first two scripts become an httpd' "500 error" page and the third
is correctly closed after the first line.
I'm also bundling another small change for slowcgi to make it send a
FCGI_END_REQUEST record instead of abruptly closing the request. I
think it's better to let the fastcgi server know that the request is
over, but the slowcgi/httpd combo works even without it.
diff refs/heads/wip refs/heads/fastcgi
commit - 3aac3f8529325cbd58806247542caabd23befb6e
commit + cab6b39fe822d105a2dc852f2435693a6eff834f
blob - 381fade2924c4b5cea77cd9cd6500e75d4d59257
blob + b6541b7c68235ac1dfc5a9d0243db988e5932a7f
--- usr.sbin/httpd/server_fcgi.c
+++ usr.sbin/httpd/server_fcgi.c
@@ -77,6 +77,7 @@ struct server_fcgi_param {
};
intserver_fcgi_header(struct client *, unsigned int);
+void server_fcgi_error(struct bufferevent *, short, void *);
void server_fcgi_read(struct bufferevent *, void *);
intserver_fcgi_writeheader(struct client *, struct kv *, void *);
intserver_fcgi_writechunk(struct client *);
@@ -133,7 +134,7 @@ server_fcgi(struct httpd *env, struct client *clt)
clt->clt_srvbev_throttled = 0;
clt->clt_srvbev = bufferevent_new(fd, server_fcgi_read,
- NULL, server_file_error, clt);
+ NULL, server_fcgi_error, clt);
if (clt->clt_srvbev == NULL) {
errstr = "failed to allocate fcgi buffer event";
goto fail;
@@ -482,6 +483,23 @@ fcgi_add_param(struct server_fcgi_param *p, const char
}
void
+server_fcgi_error(struct bufferevent *bev, short error, void *arg)
+{
+ struct client *clt = arg;
+
+ if ((error & EVBUFFER_EOF) && !clt->clt_fcgi.headersdone) {
+ server_abort_http(clt, 500, "malformed or no headers");
+ return;
+ }
+
+ /* send the end marker if not already */
+ if (clt->clt_fcgi.chunked && !clt->clt_fcgi.end++)
+ server_bufferevent_print(clt, "0\r\n\r\n");
+
+ server_file_error(bev, error, arg);
+}
+
+void
server_fcgi_read(struct bufferevent *bev, void *arg)
{
uint8_t buf[FCGI_RECORD_SIZE];
blob - ddf83f965d0e6a99ada695694bea77b775bae2aa
blob + 1d577ba63efca388ca3644d1a52d9b3d9f246014
--- usr.sbin/slowcgi/slowcgi.c
+++ usr.sbin/slowcgi/slowcgi.c
@@ -515,7 +515,34 @@ slowcgi_accept(int fd, short events, void *arg)
void
slowcgi_timeout(int fd, short events, void *arg)
{
- cleanup_request((struct request*) arg);
+ struct request *c;
+
+ c = arg;
+
+ if (c->script_flags & (STDOUT_DONE | STDERR_DONE | SCRIPT_DONE))
+ return;
+
+ if (!c->stdout_fd_closed) {
+ c->stdout_fd_closed = 1;
+ close(EVENT_FD(&c->script_ev));
+ event_del(&c->script_ev);
+ }
+
+ if (!c->stderr_fd_closed) {
+ c->stderr_fd_closed = 1;
+ close(EVENT_FD(&c->script_err_ev));
+ event_del(&c->script_err_ev);
+ }
+
+ if (!c->stdin_fd_closed) {
+ c->stdin_fd_closed = 1;
+ close(EVENT_FD(&c->script_stdin_ev));
+ event_del(&c->script_stdin_ev);
+ }
+
+ c->script_status = SIGALRM;
+ c->script_flags = STDOUT_DONE | STDERR_DONE | SCRIPT_DONE;
+ create_end_record(c);
}
void
