Re: [PATCH] Update default QoS markers for ssh

[Stuart Henderson]

On 2018/04/01 08:59, Theo de Raadt wrote:
> > I think this is the right thing to do, but needs handling in conjunction
> > with changes to PF, which has dual queue-setting (IPTOS_LOWDELAY packets
> 
> pf has to change first??
> 
> I don't understand the requirement that pf must be capable of handling
> this naunce of packets, before any of our applications are changed.
> 
> other applications outside openbsd have already been adapted to use
> DSCP.
> 
> there is traffic on the internet doing this, and yet noone has died.

Why not? PF has a feature which is *exactly* designed to work with this,
specifically so ssh still works when the line is flooded, and the minimum
fix is a couple of lines of diff..

Re: interface queue transmit mitigation (again)

[Hrvoje Popovski]

On 28.3.2018. 11:42, Hrvoje Popovski wrote:
> Hi all,
> 
> with this diff i'm getting 1.43Mpps on
> 12 x Intel(R) Xeon(R) CPU E5-2620 v2 @ 2.10GHz, 2400.01 MHz
> 
> with plain kernel i'm getting 1.12Mpps and with older diff's i was
> getting 1.31Mpps ...

On box with 6 x Intel(R) Xeon(R) CPU E5-2643 v2 @ 3.50GHz, 3600.46 MHz
with or without this diff i'm getting 1.75 Mpps and i can't trigger
freeze although i left ifconfig down/up over weekend and it's still running.

Stability of forwarding performance seems more stable with this diff
than without it, meaning that there aren't oscillation in forwarding as
before.

Re: dd(1): snprintf+writev -> dprintf

[Scott Cheloha]

On Sat, Mar 31, 2018 at 01:41:00PM -0600, Theo de Raadt wrote:
> > On Sat, Mar 31, 2018 at 01:04:43PM -0600, Theo de Raadt wrote:
> > > The only worry about converting from iov to dprintf could be some
> > > atomicity requirement.  I don't really see that in play here, I think
> > > the iov uses was simply for convenience.  No other 'signalling interrupts'
> > > seem to be in play.
> > 
> > My take was convenience, too, but in the commit you said it
> > was for atomicity:
> > 
> > http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/bin/dd/misc.c?rev=1.9=text/x-cvsweb-markup
> > 
> > Is there anything else we're worried about here?
> > 
> > Rogue signals we can't anticipate leaving us with only partial
> > output on stderr?
> 
> Let's see.  SIGINFO starts.  Imagine if the buffering is small (I
> don't think it is).  If it was small, there could be multiple writes
> to the sequence.  SIGINT arrives.  This will result in stderr output
> being a bit garbled.  This isn't a huge problem in this circumstance,
> but it demonstrates why atomicity may matter.  I think it can't occur
> because the default buffer size is large enough?  ktrace can be used
> to see what the dprintf is doing.

Indeed, the buffer is plenty large, so you won't have garbled lines:

 17629 dd   CALL  clock_gettime(CLOCK_MONOTONIC,0x7f7efa60)
 17629 dd   STRU  struct timespec { 3565013891<"Dec 20 11:38:11 
2082">.434591994 }
 17629 dd   RET   clock_gettime 0
 17629 dd   CALL  write(2,0x7f7ef420,0x25)
 17629 dd   GIO   fd 2 wrote 37 bytes
   "1024+0 records in
1024+0 records out
   "
 17629 dd   RET   write 37/0x25
 17629 dd   CALL  write(2,0x7f7ef420,0x3c)
 17629 dd   GIO   fd 2 wrote 60 bytes
   "524288 bytes transferred in 0.029 secs (17699693 bytes/sec)
   "
 17629 dd   RET   write 60/0x3c

But there is a race between each dprintf write.  So it's possible
(though I can't simulate it myself without using a debugger) to get
output like this, if SIGINT arrives mid-summary() for SIGINFO:

44232+0 records in
44231+0 records out
44232+0 records in
44231+0 records out
22646272 bytes transferred in 1.078 secs (20990805 bytes/sec)

This is probably harmless, but dd(1) is meant to be parsable
on stderr, and I think the above might break a poorly written
parser, which makes me lean towards leaving summary() as-is.

... however, We could also explicitly mask SIGINFO for SIGINT
and vice versa with sigaction(2):

sigemptyset(_mask);
sigaddset(_mask, SIGINFO);
sigaddset(_mask, SIGINT);
sa.sa_flags = SA_RESTART;
sa.sa_handler = summaryx;
sigaction(SIGINFO, , NULL);
sa.sa_handler = terminate;
sigaction(SIGINT, , NULL);

Though uglier than the current signal handling installation
in main():

signal(SIGINFO, summaryx);
signal(SIGINT, terminate);

it seems more correct to explicitly disallow preemption of a handler
by itself via a distinct signal than to implicitly prevent any of the
consequences of said preemption by using writev(2) to force atomicity.

Both the sigaction change and the dprintf change from the first mail
are attached.

ok?

--
Scott Cheloha

Index: bin/dd/dd.c
===
RCS file: /cvs/src/bin/dd/dd.c,v
retrieving revision 1.24
diff -u -p -r1.24 dd.c
--- bin/dd/dd.c 13 Aug 2017 02:06:42 -  1.24
+++ bin/dd/dd.c 1 Apr 2018 17:49:18 -
@@ -71,11 +71,23 @@ const   u_char  *ctab;  /* conversion table
 int
 main(int argc, char *argv[])
 {
+   struct sigaction sa;
+
jcl(argv);
setup();
 
-   (void)signal(SIGINFO, summaryx);
-   (void)signal(SIGINT, terminate);
+   /*
+* Keep summary output intact: don't let SIGINFO preempt SIGINT,
+* and vice versa.
+*/
+   sigemptyset(_mask);
+   sigaddset(_mask, SIGINFO);
+   sigaddset(_mask, SIGINT);
+   sa.sa_flags = SA_RESTART;
+   sa.sa_handler = summaryx;
+   sigaction(SIGINFO, , NULL);
+   sa.sa_handler = terminate;
+   sigaction(SIGINT, , NULL);
 
atexit(summary);
 
Index: bin/dd/misc.c
===
RCS file: /cvs/src/bin/dd/misc.c,v
retrieving revision 1.22
diff -u -p -r1.22 misc.c
--- bin/dd/misc.c   24 Oct 2017 14:21:10 -  1.22
+++ bin/dd/misc.c   1 Apr 2018 17:49:18 -
@@ -36,13 +36,9 @@
 
 #include 
 #include 
-#include 
 
-#include 
-#include 
-#include 
-#include 
 #include 
+#include 
 #include 
 #include 
 
@@ -53,10 +49,7 @@ void
 summary(void)
 {
struct timespec elapsed, now;
-   char buf[4][100];
-   struct iovec iov[4];
double nanosecs;
-   int i = 0;
 
if (ddflags & C_NOINFO)
return;
@@ -67,38 +60,25 @@ summary(void)
if (nanosecs == 0)
nanosecs = 1;
 
-   /* Use snprintf(3) so that we don't reenter stdio(3). */
-  

Add support for register shift/io-width to com(4)

[Mark Kettenis]

A popular choice for adding serial ports (UARTs) to SoCs is to copy
and paste some Syopsys Designware "IP" into the design.  These ports
are almost, but not quite, compatible with the NS16550.  The main
difference is that this "IP" can be synthesized with different
register widths and layouts to cater for limitations of the bus used
to connect these devices to the other components of the SoC.

On arm64 and armv7 we currently have an ugly bus_space(9) hack to make
com(4) work with these devices.  Buy the Synopsys Desigware UART is
also used in many Intel SoCs these days.  So I think it is time to
bite the bullet and change com(4) to handle these differences
natively.  The diff below does this by introducing com_read_reg() and
com_write_reg() functions that look at sc_reg_width and sc_reg_shift
in the softc to do the right thing.  For now only a sc_reg_width of 4
is handled in a special way such that we still do byte access when
sc_reg_width is 0.  So none of the existing drivers need to be changed
as we can rely on softc members being initialized to 0.

This doesn't yet deal with com(4) being the console; that will come
later.  I'd appreciate it of somebody could test this diff on an i386
or amd64 machine with serial console.

ok?


Index: dev/ic/com.c
===
RCS file: /cvs/src/sys/dev/ic/com.c,v
retrieving revision 1.165
diff -u -p -r1.165 com.c
--- dev/ic/com.c19 Feb 2018 08:59:52 -  1.165
+++ dev/ic/com.c1 Apr 2018 15:27:19 -
@@ -237,8 +237,6 @@ comopen(dev_t dev, int flag, int mode, s
 {
int unit = DEVUNIT(dev);
struct com_softc *sc;
-   bus_space_tag_t iot;
-   bus_space_handle_t ioh;
struct tty *tp;
int s;
int error = 0;
@@ -292,9 +290,6 @@ comopen(dev_t dev, int flag, int mode, s
sc->sc_ibufhigh = sc->sc_ibuf + COM_IHIGHWATER;
sc->sc_ibufend = sc->sc_ibuf + COM_IBUFSIZE;
 
-   iot = sc->sc_iot;
-   ioh = sc->sc_ioh;
-
/*
 * Wake up the sleepy heads.
 */
@@ -302,14 +297,14 @@ comopen(dev_t dev, int flag, int mode, s
switch (sc->sc_uarttype) {
case COM_UART_ST16650:
case COM_UART_ST16650V2:
-   bus_space_write_1(iot, ioh, com_lcr, LCR_EFR);
-   bus_space_write_1(iot, ioh, com_efr, EFR_ECB);
-   bus_space_write_1(iot, ioh, com_ier, 0);
-   bus_space_write_1(iot, ioh, com_efr, 0);
-   bus_space_write_1(iot, ioh, com_lcr, 0);
+   com_write_reg(sc, com_lcr, LCR_EFR);
+   com_write_reg(sc, com_efr, EFR_ECB);
+   com_write_reg(sc, com_ier, 0);
+   com_write_reg(sc, com_efr, 0);
+   com_write_reg(sc, com_lcr, 0);
break;
case COM_UART_TI16750:
-   bus_space_write_1(iot, ioh, com_ier, 0);
+   com_write_reg(sc, com_ier, 0);
break;
}
}
@@ -326,8 +321,8 @@ comopen(dev_t dev, int flag, int mode, s
fifo |= FIFO_TRIGGER_8;
if (sc->sc_uarttype == COM_UART_TI16750) {
fifo |= FIFO_ENABLE_64BYTE;
-   lcr = bus_space_read_1(iot, ioh, com_lcr);
-   bus_space_write_1(iot, ioh, com_lcr,
+   lcr = com_read_reg(sc, com_lcr);
+   com_write_reg(sc, com_lcr,
lcr | LCR_DLAB);
}
 
@@ -343,33 +338,33 @@ comopen(dev_t dev, int flag, int mode, s
 * Set the FIFO threshold based on the receive speed.
 */
for (;;) {
-   bus_space_write_1(iot, ioh, com_fifo, 0);
+   com_write_reg(sc, com_fifo, 0);
delay(100);
-   (void) bus_space_read_1(iot, ioh, com_data);
-   bus_space_write_1(iot, ioh, com_fifo, fifo |
+   (void) com_read_reg(sc, com_data);
+   com_write_reg(sc, com_fifo, fifo |
FIFO_RCV_RST | FIFO_XMT_RST);
delay(100);
-   if(!ISSET(bus_space_read_1(iot, ioh,
+   if(!ISSET(com_read_reg(sc,
com_lsr), LSR_RXRDY))
break;
}
   

Re: [PATCH] Update default QoS markers for ssh

[Theo de Raadt]

> I think this is the right thing to do, but needs handling in conjunction
> with changes to PF, which has dual queue-setting (IPTOS_LOWDELAY packets

pf has to change first??

I don't understand the requirement that pf must be capable of handling
this naunce of packets, before any of our applications are changed.

other applications outside openbsd have already been adapted to use
DSCP.

there is traffic on the internet doing this, and yet noone has died.

Re: [PATCH] Update default QoS markers for ssh

[Stuart Henderson]

On 2018/04/01 13:29, Job Snijders wrote:
> On Sun, Apr 01, 2018 at 11:29:55AM +0100, Stuart Henderson wrote:
> > On 2018/03/31 16:10, Job Snijders wrote:
> > > TL;DR: I propose to update the defaults to use DSCP "AF21" (Low
> > > Latency Data) for interactive session traffic, and CS1 ("Lower
> > > Effort") for non-interactive traffic. This applies to both IPv4 and
> > > IPv6.
> > 
> > I think this is the right thing to do, but needs handling in
> > conjunction with changes to PF, which has dual queue-setting
> > (IPTOS_LOWDELAY packets go in one queue along with empty TCP ACKs,
> > other packets can go in another lower priority queue).
> > 
> > Since firewalls are often updated less often than hosts I think it
> > would be better if the PF side was done first with some time to give
> > people chance to update, though it doesn't need to be too long (they
> > can set qos in sshd_config if they want to retain the old setting).
> > 
> > A few other pieces use IPTOS_LOWDELAY (pfsync, carp, vxlan, etherip)
> > which I think would want switching too.
> 
> What we're looking at is some kind of "migrate TOS to DSCP" project.
> There is a few tens of files where TOS values need to be examined and an
> appropiate DSCP value choosen.

Yes, that's clearly a much bigger scope. But if we're starting with ssh
those should be on the radar too IMO.

>I'm not sure I'd go as far as blindly
> replacing IPTOS_LOWDELAY with IPTOS_DSCP_AF21, but perhaps as a
> transition that is what is needed in some places.

Indeed. For some of these uses (like carp) something like CS7 seems
more appropriate. ssh seems a great place to start gaining more
experience with this - in particular any breakage can be handled
more easily than things like carp (since ssh already has config
options for this).

> We can start by treating IPTOS_LOWDELAY and IPTOS_DSCP_AF21 similarly in
> pf (see untested patch below), and then accept patches that migrate from
> TOS to DSCP?

That approach generally makes sense to me. It seems that some other
codepoints should probably also be treated as higher priority but
I'm not quite sure which yet...

> As far as I understand, out-of-the-box pf doesn't do anything with TOS
> values, so users wlil have to have explicitly configured something on
> the firewall or clients anyhow?

Yes, the firewall needs to have queues enabled and traffic assigned with
"set queue (foo, bar)" syntax.

> Kind regards,
> 
> Job
> 
>  share/man/man5/pf.conf.5 | 12 +---
>  sys/net/pf.c |  8 +---
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git share/man/man5/pf.conf.5 share/man/man5/pf.conf.5
> index 13e23423daa..f5a65e1b0d4 100644
> --- share/man/man5/pf.conf.5
> +++ share/man/man5/pf.conf.5
> @@ -679,7 +679,9 @@ interface, the queueing priority will also be written as 
> the priority
>  code point in the 802.1Q VLAN header.
>  If two priorities are given, TCP ACKs with no data payload and packets
>  which have a TOS of
> -.Cm lowdelay
> +.Cm lowdelay ,
> +or DSCP value
> +.Cm af21 ,
>  will be assigned to the second one.
>  Packets with a higher priority number are processed first,
>  and packets with the same priority are processed
> @@ -702,7 +704,9 @@ section.
>  Packets matching this rule will be assigned to the specified
>  .Ar queue .
>  If two queues are given, packets which have a TOS of
> -.Cm lowdelay
> +.Cm lowdelay ,
> +or DSCP value
> +.Cm af21 ,
>  and TCP ACKs with no data payload will be assigned to the second one.
>  See
>  .Sx QUEUEING
> @@ -1608,7 +1612,9 @@ Normally only one
>  .Ar queue
>  is specified; when a second one is specified it will instead be used for
>  packets which have a TOS of
> -.Cm lowdelay
> +.Cm lowdelay ,
> +or DSCP value
> +.Cm af21 ,
>  and for TCP ACKs with no data payload.
>  .Pp
>  To continue the previous example, the examples below would specify the
> diff --git sys/net/pf.c sys/net/pf.c
> index d841f834af1..aac20603a01 100644
> --- sys/net/pf.c
> +++ sys/net/pf.c
> @@ -2804,7 +2804,7 @@ pf_build_tcp(const struct pf_rule *r, sa_family_t af,
>   h->ip_len = htons(tlen);
>   h->ip_v = 4;
>   h->ip_hl = sizeof(*h) >> 2;
> - h->ip_tos = IPTOS_LOWDELAY;
> + h->ip_tos = IPTOS_DSCP_AF21;
>   h->ip_len = htons(len);
>   h->ip_off = htons(ip_mtudisc ? IP_DF : 0);
>   h->ip_ttl = ttl ? ttl : ip_defttl;
> @@ -7020,7 +7020,8 @@ done:
>   pf_scrub(pd.m, s->state_flags, pd.af, s->min_ttl,
>   s->set_tos);
>   pf_tag_packet(pd.m, s->tag, s->rtableid[pd.didx]);
> - if (pqid || (pd.tos & IPTOS_LOWDELAY)) {
> + if (pqid || (pd.tos & IPTOS_LOWDELAY)
> + || (pd.tos & IPTOS_DSCP_AF21)) {
>   qid = s->pqid;
>   if (s->state_flags & PFSTATE_SETPRIO)
> 

Re: pf generic packet delay

[Henning Brauer]

* Martin Pieuchot  [2018-02-23 10:04]:
> On 23/02/18(Fri) 04:08, Henning Brauer wrote:
> > * Martin Pieuchot  [2018-02-21 09:37]:
> > > On 21/02/18(Wed) 02:37, Henning Brauer wrote:
> > > I'd suggest moving the pool allocation and the function in net/pf*.c
> > > and only have a function call under #if NPF > 0.
> > worth discussing, but imo that part doesn't really have all that much
> > to do with pf.
> It is only used by pf(4).  All the code is under #if NPF > 0, so it *is*
> related to PF.  We have been reducing the #ifdef maze through the years
> for maintainability reasons.  I don't see the point for reintroducing
> them for taste.

you're jumping the gun from seeing #ifdef NPF - that is really used
here to keep stuff out of the ramdisks. Unfortunately NPF has become
quite a kitchensink ifdef for that purpose. The delay functionality is
network stack and not pf in my book, with pf being one of the possible
triggers. But anyway, this is a corner case and I don't care too much
and code can easily be moved later.
It actually has the welcome sideeffect that the pool limit can be
adjusted easily.

> If you can easily avoid using ph_cookie, then please do it.  Otherwise
> you're putting maintenance burden by writing fragile code that will
> subtly break when somebody will start using ph_cookie for something else.

I don't see that in this case but anyway, not worth splitting
hair over. Not that I had much anyway.

Here's the HND-MUC version.

Index: sys/sys/mbuf.h
===
RCS file: /cvs/src/sys/sys/mbuf.h,v
retrieving revision 1.235
diff -u -p -r1.235 mbuf.h
--- sys/sys/mbuf.h  11 Feb 2018 00:24:13 -  1.235
+++ sys/sys/mbuf.h  11 Feb 2018 04:47:44 -
@@ -100,10 +100,11 @@ struct pkthdr_pf {
struct inpcb*inp;   /* connected pcb for outgoing packet */
u_int32_tqid;   /* queue id */
u_int16_ttag;   /* tag id */
+   u_int16_tdelay; /* delay packet by X ms */
u_int8_t flags;
u_int8_t routed;
u_int8_t prio;
-   u_int8_t pad[3];
+   u_int8_t pad;
 };
 
 /* pkthdr_pf.flags */
Index: sys/net/if.c
===
RCS file: /cvs/src/sys/net/if.c,v
retrieving revision 1.549
diff -u -p -r1.549 if.c
--- sys/net/if.c20 Mar 2018 08:58:19 -  1.549
+++ sys/net/if.c1 Apr 2018 13:26:42 -
@@ -681,6 +681,11 @@ if_enqueue(struct ifnet *ifp, struct mbu
struct ifqueue *ifq;
int error;
 
+#if NPF > 0
+   if (m->m_pkthdr.pf.delay > 0)
+   return (pf_delay_pkt(m, ifp->if_index));
+#endif 
+
 #if NBRIDGE > 0
if (ifp->if_bridgeport && (m->m_flags & M_PROTO1) == 0) {
KERNEL_LOCK();
Index: sys/net/pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.1063
diff -u -p -r1.1063 pf.c
--- sys/net/pf.c6 Mar 2018 17:35:53 -   1.1063
+++ sys/net/pf.c17 Mar 2018 21:55:18 -
@@ -161,7 +161,7 @@ struct pf_test_ctx {
 
 struct pool pf_src_tree_pl, pf_rule_pl, pf_queue_pl;
 struct pool pf_state_pl, pf_state_key_pl, pf_state_item_pl;
-struct pool pf_rule_item_pl, pf_sn_item_pl;
+struct pool pf_rule_item_pl, pf_sn_item_pl, pf_pktdelay_pl;
 
 voidpf_add_threshold(struct pf_threshold *);
 int pf_check_threshold(struct pf_threshold *);
@@ -258,6 +258,7 @@ void pf_state_key_link_inpcb(struct 
p
struct inpcb *);
 voidpf_state_key_unlink_inpcb(struct pf_state_key *);
 voidpf_inpcb_unlink_state_key(struct inpcb *);
+voidpf_pktenqueue_delayed(void *);
 
 #if NPFLOG > 0
 voidpf_log_matches(struct pf_pdesc *, struct pf_rule *,
@@ -273,7 +274,8 @@ struct pf_pool_limit pf_pool_limits[PF_L
{ _src_tree_pl, PFSNODE_HIWAT, PFSNODE_HIWAT },
{ _frent_pl, PFFRAG_FRENT_HIWAT, PFFRAG_FRENT_HIWAT },
{ _ktable_pl, PFR_KTABLE_HIWAT, PFR_KTABLE_HIWAT },
-   { _kentry_pl, PFR_KENTRY_HIWAT, PFR_KENTRY_HIWAT }
+   { _kentry_pl, PFR_KENTRY_HIWAT, PFR_KENTRY_HIWAT },
+   { _pktdelay_pl, PF_PKTDELAY_MAXPKTS, PF_PKTDELAY_MAXPKTS }
 };
 
 #define STATE_LOOKUP(i, k, d, s, m)\
@@ -3488,6 +3490,8 @@ pf_rule_to_actions(struct pf_rule *r, st
a->set_prio[0] = r->set_prio[0];
a->set_prio[1] = r->set_prio[1];
}
+   if (r->rule_flag & PFRULE_SETDELAY)
+   a->delay = r->delay;
 }
 
 #define PF_TEST_ATTRIB(t, a)   \
@@ -3983,6 +3987,7 @@ pf_create_state(struct pf_pdesc *pd, str
 #endif /* NPFSYNC > 0 */
s->set_prio[0] = 

Re: [PATCH] Update default QoS markers for ssh

[Job Snijders]

On Sun, Apr 01, 2018 at 11:29:55AM +0100, Stuart Henderson wrote:
> On 2018/03/31 16:10, Job Snijders wrote:
> > TL;DR: I propose to update the defaults to use DSCP "AF21" (Low
> > Latency Data) for interactive session traffic, and CS1 ("Lower
> > Effort") for non-interactive traffic. This applies to both IPv4 and
> > IPv6.
> 
> I think this is the right thing to do, but needs handling in
> conjunction with changes to PF, which has dual queue-setting
> (IPTOS_LOWDELAY packets go in one queue along with empty TCP ACKs,
> other packets can go in another lower priority queue).
> 
> Since firewalls are often updated less often than hosts I think it
> would be better if the PF side was done first with some time to give
> people chance to update, though it doesn't need to be too long (they
> can set qos in sshd_config if they want to retain the old setting).
> 
> A few other pieces use IPTOS_LOWDELAY (pfsync, carp, vxlan, etherip)
> which I think would want switching too.

What we're looking at is some kind of "migrate TOS to DSCP" project.
There is a few tens of files where TOS values need to be examined and an
appropiate DSCP value choosen. I'm not sure I'd go as far as blindly
replacing IPTOS_LOWDELAY with IPTOS_DSCP_AF21, but perhaps as a
transition that is what is needed in some places.

We can start by treating IPTOS_LOWDELAY and IPTOS_DSCP_AF21 similarly in
pf (see untested patch below), and then accept patches that migrate from
TOS to DSCP?

As far as I understand, out-of-the-box pf doesn't do anything with TOS
values, so users wlil have to have explicitly configured something on
the firewall or clients anyhow?

Kind regards,

Job

 share/man/man5/pf.conf.5 | 12 +---
 sys/net/pf.c |  8 +---
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git share/man/man5/pf.conf.5 share/man/man5/pf.conf.5
index 13e23423daa..f5a65e1b0d4 100644
--- share/man/man5/pf.conf.5
+++ share/man/man5/pf.conf.5
@@ -679,7 +679,9 @@ interface, the queueing priority will also be written as 
the priority
 code point in the 802.1Q VLAN header.
 If two priorities are given, TCP ACKs with no data payload and packets
 which have a TOS of
-.Cm lowdelay
+.Cm lowdelay ,
+or DSCP value
+.Cm af21 ,
 will be assigned to the second one.
 Packets with a higher priority number are processed first,
 and packets with the same priority are processed
@@ -702,7 +704,9 @@ section.
 Packets matching this rule will be assigned to the specified
 .Ar queue .
 If two queues are given, packets which have a TOS of
-.Cm lowdelay
+.Cm lowdelay ,
+or DSCP value
+.Cm af21 ,
 and TCP ACKs with no data payload will be assigned to the second one.
 See
 .Sx QUEUEING
@@ -1608,7 +1612,9 @@ Normally only one
 .Ar queue
 is specified; when a second one is specified it will instead be used for
 packets which have a TOS of
-.Cm lowdelay
+.Cm lowdelay ,
+or DSCP value
+.Cm af21 ,
 and for TCP ACKs with no data payload.
 .Pp
 To continue the previous example, the examples below would specify the
diff --git sys/net/pf.c sys/net/pf.c
index d841f834af1..aac20603a01 100644
--- sys/net/pf.c
+++ sys/net/pf.c
@@ -2804,7 +2804,7 @@ pf_build_tcp(const struct pf_rule *r, sa_family_t af,
h->ip_len = htons(tlen);
h->ip_v = 4;
h->ip_hl = sizeof(*h) >> 2;
-   h->ip_tos = IPTOS_LOWDELAY;
+   h->ip_tos = IPTOS_DSCP_AF21;
h->ip_len = htons(len);
h->ip_off = htons(ip_mtudisc ? IP_DF : 0);
h->ip_ttl = ttl ? ttl : ip_defttl;
@@ -7020,7 +7020,8 @@ done:
pf_scrub(pd.m, s->state_flags, pd.af, s->min_ttl,
s->set_tos);
pf_tag_packet(pd.m, s->tag, s->rtableid[pd.didx]);
-   if (pqid || (pd.tos & IPTOS_LOWDELAY)) {
+   if (pqid || (pd.tos & IPTOS_LOWDELAY)
+   || (pd.tos & IPTOS_DSCP_AF21)) {
qid = s->pqid;
if (s->state_flags & PFSTATE_SETPRIO)
pd.m->m_pkthdr.pf.prio = s->set_prio[1];
@@ -7032,7 +7033,8 @@ done:
} else {
pf_scrub(pd.m, r->scrub_flags, pd.af, r->min_ttl,
r->set_tos);
-   if (pqid || (pd.tos & IPTOS_LOWDELAY)) {
+   if (pqid || (pd.tos & IPTOS_LOWDELAY)
+   || (pd.tos & IPTOS_DSCP_AF21)) {
qid = r->pqid;
if (r->scrub_flags & PFSTATE_SETPRIO)
pd.m->m_pkthdr.pf.prio = r->set_prio[1];

Re: [PATCH] Update default QoS markers for ssh

[Stuart Henderson]

On 2018/03/31 16:10, Job Snijders wrote:
> TL;DR: I propose to update the defaults to use DSCP "AF21" (Low Latency
> Data) for interactive session traffic, and CS1 ("Lower Effort") for
> non-interactive traffic. This applies to both IPv4 and IPv6.

I think this is the right thing to do, but needs handling in conjunction
with changes to PF, which has dual queue-setting (IPTOS_LOWDELAY packets
go in one queue along with empty TCP ACKs, other packets can go in another
lower priority queue).

Since firewalls are often updated less often than hosts I think it would
be better if the PF side was done first with some time to give people
chance to update, though it doesn't need to be too long (they can set
qos in sshd_config if they want to retain the old setting).

A few other pieces use IPTOS_LOWDELAY (pfsync, carp, vxlan, etherip)
which I think would want switching too.

>  interactive sessions and the second for non-interactive sessions.
>  The default is
> -.Cm lowdelay
> +.Cm af21
> +.Ar (Low-Latency Data)
>  for interactive sessions and
> -.Cm throughput
> +.Cm CS1
> +.Ar (Lower Effort)

Mismatching caps/lower case (between these two and with the "accepted
values" list).

Re: route(8): improve show output

[Otto Moerbeek]

On Sun, Apr 01, 2018 at 11:09:18AM +0200, Florian Obser wrote:

> On Sun, Apr 01, 2018 at 10:42:09AM +0200, Otto Moerbeek wrote:
> > On Sun, Apr 01, 2018 at 10:39:39AM +0200, Florian Obser wrote:
> > 
> > > On a backbone router with many interfaces route show scrolls for quite
> > > some time and it is difficult to see at a glance what is going on.
> > > 
> > > Colour coding is an obvious improvement.
> > > 
> > > The following diff sets the background colour to green for up routes
> > > and black for down routes.
> > > 
> > > The default route has yellow text colour (because 10BASE5 yellow
> > > cable), reject routes red and blackhole routes black text colour.
> > > 
> > > Host routes are underlined, multicast routes blink and multipath
> > > routes blink fast.
> > > 
> > > I believe the colours are intuitively obvious so no need to document
> > > the meaning.
> > > 
> > > OK?
> > 
> > Nope, this won't work on my teletype.
> > 
> 
> Oh, good point. What capabilities does it have?
> 
> I guess colours are right out. But underline should work, no? How
> about blink?
> 
> For reject routes, can we get it to generate a rattling sound or a high
> pitched screaching noise?

Blink is a bit hard, you can do ( BS BS BS BS) repeatedly until the
paper is gone to mark a blackhole route though

You also do a bell bang bell bang ... by repeating CR, a few HT and
then CR again

-Otto

> > 
> > > 
> > > diff --git show.c show.c
> > > index 913baf6cdb6..0e5d1e780e2 100644
> > > --- show.c
> > > +++ show.c
> > > @@ -278,6 +278,20 @@ p_rtentry(struct rt_msghdr *rtm)
> > >   if ((sa = rti_info[RTAX_DST]) == NULL)
> > >   return;
> > >  
> > > + if (rtm->rtm_flags & RTF_UP)
> > > + printf("\e[42m");
> > > + if (!(rtm->rtm_flags & RTF_UP))
> > > + printf("\e[40m");
> > > + if (rtm->rtm_flags & RTF_REJECT)
> > > + printf("\e[31m");
> > > + if (rtm->rtm_flags & RTF_BLACKHOLE)
> > > + printf("\e[30m");
> > > + if (rtm->rtm_flags & RTF_MULTICAST)
> > > + printf("\e[5m");
> > > + if (rtm->rtm_flags & RTF_MPATH)
> > > + printf("\e[6m");
> > > + if (rtm->rtm_flags & RTF_HOST)
> > > + printf("\e[4m");
> > >   p_sockaddr(sa, mask, rtm->rtm_flags, WID_DST(sa->sa_family));
> > >   p_sockaddr_mpls(sa, rti_info[RTAX_SRC], rtm->rtm_mpls,
> > >   WID_DST(sa->sa_family));
> > > @@ -297,6 +311,7 @@ p_rtentry(struct rt_msghdr *rtm)
> > >   if_indextoname(rtm->rtm_index, ifbuf));
> > >   if (verbose && rti_info[RTAX_LABEL])
> > >   printf(" %s", routename(rti_info[RTAX_LABEL]));
> > > + printf("\e[0m");
> > >   putchar('\n');
> > >  }
> > >  
> > > @@ -369,6 +384,10 @@ p_sockaddr(struct sockaddr *sa, struct sockaddr 
> > > *mask, int flags, int width)
> > >   cp = netname(sa, mask);
> > >   break;
> > >   }
> > > +
> > > + if (strcmp(cp, "default") == 0)
> > > + printf("\e[33m");
> > > +
> > >   if (width < 0)
> > >   printf("%s", cp);
> > >   else {
> > > 
> > > -- 
> > > I'm not entirely sure you are real.
> > 
> 
> -- 
> I'm not entirely sure you are real.

Re: route(8): improve show output

[Florian Obser]

On Sun, Apr 01, 2018 at 10:42:09AM +0200, Otto Moerbeek wrote:
> On Sun, Apr 01, 2018 at 10:39:39AM +0200, Florian Obser wrote:
> 
> > On a backbone router with many interfaces route show scrolls for quite
> > some time and it is difficult to see at a glance what is going on.
> > 
> > Colour coding is an obvious improvement.
> > 
> > The following diff sets the background colour to green for up routes
> > and black for down routes.
> > 
> > The default route has yellow text colour (because 10BASE5 yellow
> > cable), reject routes red and blackhole routes black text colour.
> > 
> > Host routes are underlined, multicast routes blink and multipath
> > routes blink fast.
> > 
> > I believe the colours are intuitively obvious so no need to document
> > the meaning.
> > 
> > OK?
> 
> Nope, this won't work on my teletype.
> 

Oh, good point. What capabilities does it have?

I guess colours are right out. But underline should work, no? How
about blink?

For reject routes, can we get it to generate a rattling sound or a high
pitched screaching noise?

>   -Otto
> 
> > 
> > diff --git show.c show.c
> > index 913baf6cdb6..0e5d1e780e2 100644
> > --- show.c
> > +++ show.c
> > @@ -278,6 +278,20 @@ p_rtentry(struct rt_msghdr *rtm)
> > if ((sa = rti_info[RTAX_DST]) == NULL)
> > return;
> >  
> > +   if (rtm->rtm_flags & RTF_UP)
> > +   printf("\e[42m");
> > +   if (!(rtm->rtm_flags & RTF_UP))
> > +   printf("\e[40m");
> > +   if (rtm->rtm_flags & RTF_REJECT)
> > +   printf("\e[31m");
> > +   if (rtm->rtm_flags & RTF_BLACKHOLE)
> > +   printf("\e[30m");
> > +   if (rtm->rtm_flags & RTF_MULTICAST)
> > +   printf("\e[5m");
> > +   if (rtm->rtm_flags & RTF_MPATH)
> > +   printf("\e[6m");
> > +   if (rtm->rtm_flags & RTF_HOST)
> > +   printf("\e[4m");
> > p_sockaddr(sa, mask, rtm->rtm_flags, WID_DST(sa->sa_family));
> > p_sockaddr_mpls(sa, rti_info[RTAX_SRC], rtm->rtm_mpls,
> > WID_DST(sa->sa_family));
> > @@ -297,6 +311,7 @@ p_rtentry(struct rt_msghdr *rtm)
> > if_indextoname(rtm->rtm_index, ifbuf));
> > if (verbose && rti_info[RTAX_LABEL])
> > printf(" %s", routename(rti_info[RTAX_LABEL]));
> > +   printf("\e[0m");
> > putchar('\n');
> >  }
> >  
> > @@ -369,6 +384,10 @@ p_sockaddr(struct sockaddr *sa, struct sockaddr *mask, 
> > int flags, int width)
> > cp = netname(sa, mask);
> > break;
> > }
> > +
> > +   if (strcmp(cp, "default") == 0)
> > +   printf("\e[33m");
> > +
> > if (width < 0)
> > printf("%s", cp);
> > else {
> > 
> > -- 
> > I'm not entirely sure you are real.
> 

-- 
I'm not entirely sure you are real.

Re: route(8): improve show output

[Otto Moerbeek]

On Sun, Apr 01, 2018 at 10:39:39AM +0200, Florian Obser wrote:

> On a backbone router with many interfaces route show scrolls for quite
> some time and it is difficult to see at a glance what is going on.
> 
> Colour coding is an obvious improvement.
> 
> The following diff sets the background colour to green for up routes
> and black for down routes.
> 
> The default route has yellow text colour (because 10BASE5 yellow
> cable), reject routes red and blackhole routes black text colour.
> 
> Host routes are underlined, multicast routes blink and multipath
> routes blink fast.
> 
> I believe the colours are intuitively obvious so no need to document
> the meaning.
> 
> OK?

Nope, this won't work on my teletype.

-Otto

> 
> diff --git show.c show.c
> index 913baf6cdb6..0e5d1e780e2 100644
> --- show.c
> +++ show.c
> @@ -278,6 +278,20 @@ p_rtentry(struct rt_msghdr *rtm)
>   if ((sa = rti_info[RTAX_DST]) == NULL)
>   return;
>  
> + if (rtm->rtm_flags & RTF_UP)
> + printf("\e[42m");
> + if (!(rtm->rtm_flags & RTF_UP))
> + printf("\e[40m");
> + if (rtm->rtm_flags & RTF_REJECT)
> + printf("\e[31m");
> + if (rtm->rtm_flags & RTF_BLACKHOLE)
> + printf("\e[30m");
> + if (rtm->rtm_flags & RTF_MULTICAST)
> + printf("\e[5m");
> + if (rtm->rtm_flags & RTF_MPATH)
> + printf("\e[6m");
> + if (rtm->rtm_flags & RTF_HOST)
> + printf("\e[4m");
>   p_sockaddr(sa, mask, rtm->rtm_flags, WID_DST(sa->sa_family));
>   p_sockaddr_mpls(sa, rti_info[RTAX_SRC], rtm->rtm_mpls,
>   WID_DST(sa->sa_family));
> @@ -297,6 +311,7 @@ p_rtentry(struct rt_msghdr *rtm)
>   if_indextoname(rtm->rtm_index, ifbuf));
>   if (verbose && rti_info[RTAX_LABEL])
>   printf(" %s", routename(rti_info[RTAX_LABEL]));
> + printf("\e[0m");
>   putchar('\n');
>  }
>  
> @@ -369,6 +384,10 @@ p_sockaddr(struct sockaddr *sa, struct sockaddr *mask, 
> int flags, int width)
>   cp = netname(sa, mask);
>   break;
>   }
> +
> + if (strcmp(cp, "default") == 0)
> + printf("\e[33m");
> +
>   if (width < 0)
>   printf("%s", cp);
>   else {
> 
> -- 
> I'm not entirely sure you are real.

route(8): improve show output

[Florian Obser]

On a backbone router with many interfaces route show scrolls for quite
some time and it is difficult to see at a glance what is going on.

Colour coding is an obvious improvement.

The following diff sets the background colour to green for up routes
and black for down routes.

The default route has yellow text colour (because 10BASE5 yellow
cable), reject routes red and blackhole routes black text colour.

Host routes are underlined, multicast routes blink and multipath
routes blink fast.

I believe the colours are intuitively obvious so no need to document
the meaning.

OK?

diff --git show.c show.c
index 913baf6cdb6..0e5d1e780e2 100644
--- show.c
+++ show.c
@@ -278,6 +278,20 @@ p_rtentry(struct rt_msghdr *rtm)
if ((sa = rti_info[RTAX_DST]) == NULL)
return;
 
+   if (rtm->rtm_flags & RTF_UP)
+   printf("\e[42m");
+   if (!(rtm->rtm_flags & RTF_UP))
+   printf("\e[40m");
+   if (rtm->rtm_flags & RTF_REJECT)
+   printf("\e[31m");
+   if (rtm->rtm_flags & RTF_BLACKHOLE)
+   printf("\e[30m");
+   if (rtm->rtm_flags & RTF_MULTICAST)
+   printf("\e[5m");
+   if (rtm->rtm_flags & RTF_MPATH)
+   printf("\e[6m");
+   if (rtm->rtm_flags & RTF_HOST)
+   printf("\e[4m");
p_sockaddr(sa, mask, rtm->rtm_flags, WID_DST(sa->sa_family));
p_sockaddr_mpls(sa, rti_info[RTAX_SRC], rtm->rtm_mpls,
WID_DST(sa->sa_family));
@@ -297,6 +311,7 @@ p_rtentry(struct rt_msghdr *rtm)
if_indextoname(rtm->rtm_index, ifbuf));
if (verbose && rti_info[RTAX_LABEL])
printf(" %s", routename(rti_info[RTAX_LABEL]));
+   printf("\e[0m");
putchar('\n');
 }
 
@@ -369,6 +384,10 @@ p_sockaddr(struct sockaddr *sa, struct sockaddr *mask, int 
flags, int width)
cp = netname(sa, mask);
break;
}
+
+   if (strcmp(cp, "default") == 0)
+   printf("\e[33m");
+
if (width < 0)
printf("%s", cp);
else {

-- 
I'm not entirely sure you are real.