Re: svn commit: r333911 - head/sys/netinet

[Jonathan Looney]

On Sat, May 19, 2018 at 10:27 PM, Matt Macy  wrote:

> +   il = malloc(sizeof(struct in_pcblist) + n * sizeof(struct inpcb
> *), M_TEMP, M_WAITOK|M_ZERO);
> +   inp_list = il->il_inp_list;
>

Why does this need M_ZERO?

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r333494 - head/share/man/man7

[Jonathan Looney]

On Sun, May 13, 2018 at 8:14 PM, Rodney W. Grimes <
free...@pdx.rh.cn85.dnsmgr.net> wrote:
>
> It did take me some time to track down this "crazy concept you all
> think I just invented", but it is infact in the GNU groff info
> documentaton (found on my 5.4 systems in /usr/share/info/groff.info.gz):

Just to be clear, I don't think these rules apply to FreeBSD. We use
mandoc. See mandoc(1) for the rules that apply to us.

And, again, just to be clear, I am also pretty sure the following rule
doesn't apply:

>* In keeping with this, it is helpful to begin a new line after every
>  comma or phrase, since common corrections are to add or delete
>  sentences or phrases.

OTOH, I believe we do have a rule about beginning each sentence on a new
line. (Again, see mandoc(1).)

And, it is easy to figure out whether your page complies with the style
using mandoc's checkers.

For example:

$ mandoc -W all,stop /usr/share/man/man9/tcp_functions.9.gz
mandoc: /usr/share/man/man9/tcp_functions.9.gz:284:16: WARNING: new
sentence, new line

Jonathan

PS: I'm happy to be corrected by one of the man page experts, which I most
certainly am not.
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r332770 - in head/sys: conf netinet netinet/tcp_stacks sys

[Jonathan Looney]

On Mon, Apr 30, 2018 at 3:16 AM, hiren panchasara <
hi...@strugglingcoder.info> wrote:
>
> In my understanding, default stack currently cannot use this mechanism.
When do
> you think that'll be possible?


I think I can speak to Randall's plans for this.

Randall chose not to include in this commit the hooks for the default stack
to use the high-precision timers. I believe his immediate priorities are
upstreaming RACK and BBR. After that, if there is demand, he may upstream
the (relatively untested) code that allows the default stack to use the
high-precision timers (protected by a non-default kernel option) so others
can choose to experiment with it.

(By the way, we're hoping to change the terminology away from describing
the traditional FreeBSD stack as the "default" stack. In theory, someone
can make any stack be their local default. We'll need to figure out what to
actually call it at some point. My suggestion was the "FreeBSD" stack,
although that is lacking in some imagination. In any case, we should have
that discussion at some point in the future.)

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r332860 - head/sys/kern

[Jonathan Looney]

On Sat, Apr 21, 2018 at 1:53 PM, Conrad Meyer  wrote:
>
> I don't think this should be enabled by default.  Can we leave it
> disabled by default and let consumers opt-in?

I'm willing to change the default if there's a good reason or consensus for
that. However, it is not obvious to me that the default is actually wrong.

I think its important that we remember where we are when we hit this code.

By the time we hit this code, we've already panic'd (whether due to a
"true" panic or a violated assertion). At this point, the system is already
in an unknown/unexpected state and we want to preserve our ability to
troubleshoot and/or recover. (And, since we're running a system with
INVARIANTS, presumably the ability to troubleshoot is important.)

We may well violate a second assertion simply because the system is in an
unknown/unexpected state. Once we do that, the question is whether we
should try to preserve the ability to troubleshoot, or should give up and
reset the system.

All too often, my ability to debug assertion violations is hindered because
the system trips over yet another assertion while dumping the core. If we
skip the assertion, nothing bad happens. (The post-panic debugging code
already needs to deal with systems that are inconsistent, and it does a
pretty good job at it.)

On the other hand, I really am not sure what you are worried might happen
if we skip checking assertions after we've already panic'd. As far as I can
tell, the likely worst case is that we hit a true panic of some kind. In
that case, we're no worse off than before.

I think the one obvious exception is when we're purposely trying to
validate the post-panic debugging code. In that case, you can change the
sysctl/tunable to enable troubleshooting.

I would honestly appreciate someone explaining the dangers in disabling a
response to assertion violations after we've already panic'd and are simply
trying to troubleshoot, because they are not obvious to me. But, I could
simply be missing them.

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r331347 - in head: etc/mtree include sys/conf sys/dev/tcp_log sys/kern sys/netinet usr.bin/netstat

[Jonathan Looney]

Compilation should be fixed as of r331483. I also made this an
optional feature in r331485. If you find more problems, please let me
know.

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r331347 - in head: etc/mtree include sys/conf sys/dev/tcp_log sys/kern sys/netinet usr.bin/netstat

[Jonathan Looney]

On Thu, Mar 22, 2018 at 9:05 PM, Justin Hibbits <jr...@alumni.cwru.edu>
wrote:

> On Thu, Mar 22, 2018 at 3:44 PM, Andriy Gapon <a...@freebsd.org> wrote:
> > On 22/03/2018 17:39, Jonathan Looney wrote:
> >> A tinderbox build didn't complain about atomic_fetchadd_64, so I assume
> it is OK.
> >>
> > FWWI, TARGET=powerpc TARGET_ARCH=powerpc build failed for me.
> >
> > cc1: warnings being treated as errors
> > /usr/devel/svn/head/sys/netinet/tcp_log_buf.c: In function
> 'tcp_log_selectauto':
> > /usr/devel/svn/head/sys/netinet/tcp_log_buf.c:286: warning: implicit
> declaration
> > of function 'atomic_fetchadd_64'
> > /usr/devel/svn/head/sys/netinet/tcp_log_buf.c:286: warning: nested
> extern
> > declaration of 'atomic_fetchadd_64' [-Wnested-externs]
>
> mips complains, too.  Check out https://ci.freebsd.org/tinderbox .
>

Thanks for all the reports! I'm working on a fix now.

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r331347 - in head: etc/mtree include sys/conf sys/dev/tcp_log sys/kern sys/netinet usr.bin/netstat

[Jonathan Looney]

A tinderbox build didn't complain about atomic_fetchadd_64, so I assume it
is OK.

Yes, this can be made optional, if there is a need for that.

Jonathan

On Thu, Mar 22, 2018 at 2:22 PM, Ruslan Bukin 
wrote:

> Also can this be pluggable ?
> It looks like it is optional device which means it can free up some space
> in embedded environment when unused
>
> Ruslan
>
> On Thu, Mar 22, 2018 at 02:16:06PM +, Ruslan Bukin wrote:
> > We don't have atomic_fetchadd_64 for mips32 I think
> >
> > Ruslan
> >
> > On Thu, Mar 22, 2018 at 09:40:08AM +, Jonathan T. Looney wrote:
> > > Author: jtl
> > > Date: Thu Mar 22 09:40:08 2018
> > > New Revision: 331347
> > > URL: https://svnweb.freebsd.org/changeset/base/331347
> > >
> > > Log:
> > >   Add the "TCP Blackbox Recorder" which we discussed at the developer
> > >   summits at BSDCan and BSDCam in 2017.
> > >
> > >   The TCP Blackbox Recorder allows you to capture events on a TCP
> connection
> > >   in a ring buffer. It stores metadata with the event. It optionally
> stores
> > >   the TCP header associated with an event (if the event is associated
> with a
> > >   packet) and also optionally stores information on the sockets.
> > >
> > >   It supports setting a log ID on a TCP connection and using this to
> correlate
> > >   multiple connections that share a common log ID.
> > >
> > >   You can log connections in different modes. If you are doing a
> coordinated
> > >   test with a particular connection, you may tell the system to put it
> in
> > >   mode 4 (continuous dump). Or, if you just want to monitor for
> errors, you
> > >   can put it in mode 1 (ring buffer) and dump all the ring buffers
> associated
> > >   with the connection ID when we receive an error signal for that
> connection
> > >   ID. You can set a default mode that will be applied to a particular
> ratio
> > >   of incoming connections. You can also manually set a mode using a
> socket
> > >   option.
> > >
> > >   This commit includes only basic probes. rrs@ has added quite an
> abundance
> > >   of probes in his TCP development work. He plans to commit those soon.
> > >
> > >   There are user-space programs which we plan to commit as ports.
> These read
> > >   the data from the log device and output pcapng files, and then let
> you
> > >   analyze the data (and metadata) in the pcapng files.
> > >
> > >   Reviewed by:  gnn (previous version)
> > >   Obtained from:Netflix, Inc.
> > >   Relnotes: yes
> > >   Differential Revision:https://reviews.freebsd.org/D11085
> > >
> > > Added:
> > >   head/sys/dev/tcp_log/
> > >   head/sys/dev/tcp_log/tcp_log_dev.c   (contents, props changed)
> > >   head/sys/dev/tcp_log/tcp_log_dev.h   (contents, props changed)
> > >   head/sys/netinet/tcp_log_buf.c   (contents, props changed)
> > >   head/sys/netinet/tcp_log_buf.h   (contents, props changed)
> > > Modified:
> > >   head/etc/mtree/BSD.include.dist
> > >   head/include/Makefile
> > >   head/sys/conf/files
> > >   head/sys/kern/subr_witness.c
> > >   head/sys/netinet/tcp.h
> > >   head/sys/netinet/tcp_input.c
> > >   head/sys/netinet/tcp_output.c
> > >   head/sys/netinet/tcp_subr.c
> > >   head/sys/netinet/tcp_timer.c
> > >   head/sys/netinet/tcp_usrreq.c
> > >   head/sys/netinet/tcp_var.h
> > >   head/usr.bin/netstat/inet.c
> > >   head/usr.bin/netstat/main.c
> > >   head/usr.bin/netstat/netstat.1
> > >   head/usr.bin/netstat/netstat.h
> > >
> > > Modified: head/etc/mtree/BSD.include.dist
> > > 
> ==
> > > --- head/etc/mtree/BSD.include.dist Thu Mar 22 08:32:39 2018
> (r331346)
> > > +++ head/etc/mtree/BSD.include.dist Thu Mar 22 09:40:08 2018
> (r331347)
> > > @@ -158,6 +158,8 @@
> > >  ..
> > >  speaker
> > >  ..
> > > +tcp_log
> > > +..
> > >  usb
> > >  ..
> > >  vkbd
> > >
> > > Modified: head/include/Makefile
> > > 
> ==
> > > --- head/include/Makefile   Thu Mar 22 08:32:39 2018(r331346)
> > > +++ head/include/Makefile   Thu Mar 22 09:40:08 2018(r331347)
> > > @@ -47,7 +47,7 @@ LSUBDIRS= cam/ata cam/mmc cam/nvme cam/scsi \
> > > dev/hwpmc dev/hyperv \
> > > dev/ic dev/iicbus dev/io dev/lmc dev/mfi dev/mmc dev/nvme \
> > > dev/ofw dev/pbio dev/pci ${_dev_powermac_nvram} dev/ppbus
> dev/smbus \
> > > -   dev/speaker dev/vkbd dev/wi \
> > > +   dev/speaker dev/tcp_log dev/vkbd dev/wi \
> > > fs/devfs fs/fdescfs fs/msdosfs fs/nandfs fs/nfs fs/nullfs \
> > > fs/procfs fs/smbfs fs/udf fs/unionfs \
> > > geom/cache geom/concat geom/eli geom/gate geom/journal geom/label \
> > >
> > > Modified: head/sys/conf/files
> > > 
> ==
> > > --- head/sys/conf/files Thu Mar 22 08:32:39 2018(r331346)
> > > +++ head/sys/conf/files Thu 

Re: svn commit: r330539 - in head/sys: amd64/amd64 amd64/include arm/include conf gdb i386/include mips/include powerpc/include sparc64/include

[Jonathan Looney]

On Tue, Mar 6, 2018 at 6:31 PM, Oliver Pinter  wrote:

> X-MFC-with:
>
> commit 27ac811b7acd31b1bdbf959fe49a957cdeabf780
> Author: bde 
> Date:   Fri Mar 24 17:34:55 2017 +
>

ACK. Thanks!

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r330407 - head/sys/dev/iicbus

[Jonathan Looney]

It looks like this is causing compilation failures:

/home/jtl/head/sys/dev/iicbus/ds1672.c:147:20: error: use of undeclared
identifier 'sc'
clock_dbgprint_ts(sc->sc_dev, CLOCK_DBG_READ, ts);
  ^
/home/jtl/head/sys/dev/iicbus/ds1672.c:162:20: error: use of undeclared
identifier 'sc'
clock_dbgprint_ts(sc->sc_dev, CLOCK_DBG_WRITE, ts);
  ^
2 errors generated.


Jonathan

On Sun, Mar 4, 2018 at 2:32 PM, Ian Lepore  wrote:

> Author: ian
> Date: Sun Mar  4 19:32:52 2018
> New Revision: 330407
> URL: https://svnweb.freebsd.org/changeset/base/330407
>
> Log:
>   Add calls to the new clock_dbgprint_xxx() functions.  Also, stop applying
>   a local .5 second adjustment to the time, since that is now done by the
>   code in subr_rtc.c.
>
> Modified:
>   head/sys/dev/iicbus/ds1672.c
>
> Modified: head/sys/dev/iicbus/ds1672.c
> 
> ==
> --- head/sys/dev/iicbus/ds1672.cSun Mar  4 19:26:47 2018
> (r330406)
> +++ head/sys/dev/iicbus/ds1672.cSun Mar  4 19:32:52 2018
> (r330407)
> @@ -54,8 +54,6 @@ __FBSDID("$FreeBSD$");
>
>  #defineDS1672_CTRL_EOSC(1 << 7)/* Stop/start
> flag. */
>
> -#define NANOSEC10
> -
>  #defineMAX_IIC_DATA_SIZE   4
>
>  struct ds1672_softc {
> @@ -144,8 +142,9 @@ ds1672_gettime(device_t dev, struct timespec *ts)
> /* counter has seconds since epoch */
> ts->tv_sec = (secs[3] << 24) | (secs[2] << 16)
>| (secs[1] <<  8) | (secs[0] <<  0);
> -   ts->tv_nsec = NANOSEC / 2;
> +   ts->tv_nsec = 0;
> }
> +   clock_dbgprint_ts(sc->sc_dev, CLOCK_DBG_READ, ts);
> return (error);
>  }
>
> @@ -159,6 +158,8 @@ ds1672_settime(device_t dev, struct timespec *ts)
> data[2] = (ts->tv_sec >> 16) & 0xff;
> data[3] = (ts->tv_sec >> 24) & 0xff;
>
> +   ts->tv_nsec = 0;
> +   clock_dbgprint_ts(sc->sc_dev, CLOCK_DBG_WRITE, ts);
> return (ds1672_write(dev, DS1672_COUNTER, data, 4));
>  }
>
>
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r329171 - head/sys/amd64/amd64

[Jonathan Looney]

On Mon, Feb 12, 2018 at 12:27 PM, Jonathan T. Looney 
wrote:

> Author: jtl
> Date: Mon Feb 12 17:27:50 2018
> New Revision: 329171
> URL: https://svnweb.freebsd.org/changeset/base/329171
>
> Log:
>   Mark the pages used for the initial page-table entries as wired. This
>   makes them consistent with the way other page-table pages are allocated.
>   It also provides the rest of the VM system a good clue that these pages
>   are used.
>
>   Reviewed by:  alc, kib, markj
>   Sponsored by: Netflix
>   Differential Revision:https://reviews.freebsd.org/D14269


Should have also said...
MFC after: 2 weeks
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r324179 - head/sys/netinet

[Jonathan Looney]

Hi Julien,

I apologize for just getting to this, but your code just made it into my
local tree.

The non-INVARIANTS case looks incorrect. Because tw stays on the list, it
can end up stuck at the front of the queue and block everything behind it.

Personally, I would be more comfortable just panic'ing here. This indicates
a problem. And, depending on the way things got corrupted, you could end up
with other hard-to-debug problems if you continue with a corrupted state.

Jonathan

On Sun, Oct 1, 2017 at 5:20 PM, Julien Charbon  wrote:

> Author: jch
> Date: Sun Oct  1 21:20:28 2017
> New Revision: 324179
> URL: https://svnweb.freebsd.org/changeset/base/324179
>
> Log:
>   Fix an infinite loop in tcp_tw_2msl_scan() when an INP_TIMEWAIT inp has
>   been destroyed before its tcptw with INVARIANTS undefined.
>
>   This is a symmetric change of r307551:
>
>   A INP_TIMEWAIT inp should not be destroyed before its tcptw, and
> INVARIANTS
>   will catch this case.  If INVARIANTS is undefined it will emit a
> log(LOG_ERR)
>   and avoid a hard to debug infinite loop in tcp_tw_2msl_scan().
>
>   Reported by:  Ben Rubson, hselasky
>   Submitted by: hselasky
>   Tested by:Ben Rubson, jch
>   MFC after:1 week
>   Sponsored by: Verisign, inc
>   Differential Revision:https://reviews.freebsd.org/D12267
>
> Modified:
>   head/sys/netinet/tcp_timewait.c
>
> Modified: head/sys/netinet/tcp_timewait.c
> 
> ==
> --- head/sys/netinet/tcp_timewait.c Sun Oct  1 20:12:30 2017
> (r324178)
> +++ head/sys/netinet/tcp_timewait.c Sun Oct  1 21:20:28 2017
> (r324179)
> @@ -709,10 +709,29 @@ tcp_tw_2msl_scan(int reuse)
> INP_WLOCK(inp);
> tw = intotw(inp);
> if (in_pcbrele_wlocked(inp)) {
> -   KASSERT(tw == NULL, ("%s: held last inp "
> -   "reference but tw not NULL",
> __func__));
> -   INP_INFO_RUNLOCK(_tcbinfo);
> -   continue;
> +   if (__predict_true(tw == NULL)) {
> +   INP_INFO_RUNLOCK(_tcbinfo);
> +   continue;
> +   } else {
> +   /* This should not happen as in
> TIMEWAIT
> +* state the inp should not be
> destroyed
> +* before its tcptw. If INVARIANTS
> is
> +* defined panic.
> +*/
> +#ifdef INVARIANTS
> +   panic("%s: Panic before an
> infinite "
> +   "loop: INP_TIMEWAIT &&
> (INP_FREED "
> +   "|| inp last reference) && tw
> != "
> +   "NULL", __func__);
> +#else
> +   log(LOG_ERR, "%s: Avoid an
> infinite "
> +   "loop: INP_TIMEWAIT &&
> (INP_FREED "
> +   "|| inp last reference) && tw
> != "
> +   "NULL", __func__);
> +#endif
> +   INP_INFO_RUNLOCK(_tcbinfo);
> +   break;
> +   }
> }
>
> if (tw == NULL) {
>
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r323942 - head/sys/net

[Jonathan Looney]

On Sat, Sep 23, 2017 at 4:37 AM, Bjoern A. Zeeb <
bzeeb-li...@lists.zabbadoz.net> wrote:

>
> Then this makes no sense as we don’t do LRO if forwarding is enabled on
> the machine;
> https://svnweb.freebsd.org/base/head/sys/netinet/tcp_lro.c?
> annotate=317390#l645


Yes, that is true. However, this change still makes a difference.
Previously, if LRO was not enabled or the packet was not eligible for LRO,
the iflib code would call ifp->if_input() once for each packet. Now, the
iflib code will build a chain of packets for which it couldn't do LRO and
call ifp->if_input() once for the entire chain.

(I agree that was not obvious from the rather short commit message and the
diff in the email. The lack of comments or meaningful variable names did
not help to alleviate the confusion. Nonetheless, when I looked at the diff
with enough surrounding context, it became clear.)

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r319720 - head/sys/dev/vt

[Jonathan Looney]

Hi Konstantin,

On Sat, Jun 10, 2017 at 2:12 AM, Konstantin Belousov 
wrote:
> No, acquire is only specified for loads, and release for stores.  In other
> words, on some hypothetical ll/sc architecture, the atomic_add_acq()
> could be implemented as follows, in asm-pseudocode
> atomic_add_acq(int x):
> ll  x, r1
> acq x
> add 1, r
> sc  r1, x
> Your use of the atomic does not prevent stores reordering.

If this is true, it sounds like the atomic(9) man page needs some revision.
It says:

 When an atomic operation has acquire semantics, the effects of the
 operation must have completed before any subsequent load or store (by
 program order) is performed.  Conversely, acquire semantics do not
 require that prior loads or stores have completed before the atomic
 operation is performed.

Up until now, I have relied on this description of the way acquire barriers
work. If this description is incorrect, I think it is important to fix it
quickly.


> I'm not claiming that this fixes all bugs in this area. (In fact, I
> > specifically disclaim this.) But, it does stop the crash from occurring.
> >
> > If you still feel there are better mechanisms to achieve the desired
> > ordering, please let me know and I'll be happy to fix and/or improve
> this.
> See the pseudocode I posted in my original reply, which uses acq/rel pair.



The code you posted for vt_resume_flush_timer() would switch vd_timer_armed
from 0 to 1 even if VDF_ASYNC is not set; however, that is not what we
want. vd_timer_armed should be left untouched if VDF_ASYNC is not set.

It sounds like what I want is some sort of fence in vt_upgrade() like jhb@
specified in his email. For example:

vd->vd_timer_armed = 1;
atomic_thread_fence_rel();
vd->vd_flags |= VDF_ASYNC;

I recognize a fence would be cleaner since I really only needed the barrier
and not the atomic operation. Do you agree the above would be sufficient to
ensure store ordering?

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r319720 - head/sys/dev/vt

[Jonathan Looney]

Hi John, Konstantin,

This crash occurs during system startup when we are trying to switch from
having each write to the vt device do an immediate flush to using a
callout-based asynchronous flushing mechanism.

It appears the crash was caused by having the VDF_ASYNC flag set while the
vd_timer_armed flag was 0. The fix is to make sure that vd_timer_armed is 1
before the VDF_ASYNC flag is set. It is my understanding that the acquire
semantics in the atomic_add_acq_int() call should ensure that the write to
vd_timer_armed occurs before the load, bitwise-or, and store associated
with `vd->vd_flags |= VDF_ASYNC`. Ensuring that ordering (or, at least the
store ordering) is all that is really necessary to stop the crash from
occurring.

(A more thorough analysis is available in the PR [217408], which I forgot
to include in the commit metadata.)

To answer Konstantin's question, the VDF_ASYNC and vd_timer_armed flags are
different. The VDF_ASYNC flag indicates that we want to use async flushing.
The vd_timer_armed flag indicates that the callout is actually armed to
flush at some point soon, so a thread that writes to the vt device doesn't
need to worry about scheduling the callout.

I'm not claiming that this fixes all bugs in this area. (In fact, I
specifically disclaim this.) But, it does stop the crash from occurring.

If you still feel there are better mechanisms to achieve the desired
ordering, please let me know and I'll be happy to fix and/or improve this.

Jonathan

On Thu, Jun 8, 2017 at 2:49 PM, John Baldwin  wrote:

> On Thursday, June 08, 2017 08:47:18 PM Jonathan T. Looney wrote:
> > Author: jtl
> > Date: Thu Jun  8 20:47:18 2017
> > New Revision: 319720
> > URL: https://svnweb.freebsd.org/changeset/base/319720
> >
> > Log:
> >   With EARLY_AP_STARTUP enabled, we are seeing crashes in
> softclock_call_cc()
> >   during bootup. Debugging information shows that softclock_call_cc() is
> >   trying to execute the vt_consdev.vd_timer callout, and the callout
> >   structure contains a NULL c_func.
> >
> >   This appears to be due to a race between vt_upgrade() running
> >   callout_reset() and vt_resume_flush_timer() calling callout_schedule().
> >
> >   Fix the race by ensuring that vd_timer_armed is always set before
> >   attempting to (re)schedule the callout.
> >
> >   Discussed with: emaste
> >   MFC after:  2 weeks
> >   Sponsored by:   Netflix
> >   Differential Revision:  https://reviews.freebsd.org/D9828
>
> This should probably be using atomic_thread_fence_foo() in conjunction with
> a simple 'vd->vd_timer_armed = 1' assignment instead of abusing
> atomic_add_acq_int().  Unfortunately atomic_thread_fence_*() aren't yet
> documented in atomic(9). :(  The commit message that added them is below
> though:
>
> 
> r285283 | kib | 2015-07-08 11:12:24 -0700 (Wed, 08 Jul 2015) | 22 lines
>
> Add the atomic_thread_fence() family of functions with intent to
> provide a semantic defined by the C11 fences with corresponding
> memory_order.
>
> atomic_thread_fence_acq() gives r | r, w, where r and w are read and
> write accesses, and | denotes the fence itself.
>
> atomic_thread_fence_rel() is r, w | w.
>
> atomic_thread_fence_acq_rel() is the combination of the acquire and
> release in single operation.  Note that reads after the acq+rel fence
> could be made visible before writes preceeding the fence.
>
> atomic_thread_fence_seq_cst() orders all accesses before/after the
> fence, and the fence itself is globally ordered against other
> sequentially consistent atomic operations.
>
> Reviewed by:alc
> Discussed with: bde
> Sponsored by:   The FreeBSD Foundation
> MFC after:  3 weeks
>
> 
>
> That said, it is hard to see how a bare acquire barrier is really
> sufficient for anything.  Acquire barriers generally must be paired with
> a release barrier in order to provide sychronization.
>
> --
> John Baldwin
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r314216 - head/sys/x86/x86

[Jonathan Looney]

On Fri, Feb 24, 2017 at 3:19 PM, hiren panchasara <
hi...@strugglingcoder.info> wrote:

> On 02/24/17 at 06:56P, Jonathan T. Looney wrote:
> > Author: jtl
> > Date: Fri Feb 24 18:56:00 2017
> > New Revision: 314216
> > URL: https://svnweb.freebsd.org/changeset/base/314216
> >
> > Log:
> >   We have seen several cases recently where we appear to get a
> double-fault:
> >   We have an original panic. Then, instead of writing the core to the
> dump
> >   device, the kernel has a second panic: "smp_targeted_tlb_shootdown:
> >   interrupts disabled". This change is an attempt to fix that second
> panic.
> >
> >   When the other CPUs are stopped, we can't notify them of the TLB
> shootdown,
> >   so we skip that operation. However, when the CPUs come back up, we
> >   invalidate the TLB to ensure they correctly observe any changes to the
> >   page mappings.
> >
> >   Reviewed by:kib
> >   Sponsored by:   Netflix
> >   Differential Revision:  https://reviews.freebsd.org/D9786
>
> Can this be MFCd to 11?
>

It can be. I didn't propose it because I have only seen the problem on
-CURRENT. But, I see no obstacle to MFCing to stable/11, if you want to see
it there.

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r314116 - head/sys/kern

[Jonathan Looney]

On Wed, Feb 22, 2017 at 8:18 PM, Jonathan T. Looney  wrote:

> Author: jtl
> Date: Thu Feb 23 01:18:47 2017
> New Revision: 314116
> URL: https://svnweb.freebsd.org/changeset/base/314116
>
> Log:
>   Fix a panic during boot caused by inadequate locking of some vt(4) driver
>   data structures.
>
>   vt_change_font() calls vtbuf_grow() to change some vt driver data
>   structures. It uses TF_MUTE to prevent the console from trying to use
> those
>   data structures while it changes them.
>
>   During the early stage of the boot process, the vt driver's tc_done
> routine
>   uses those data structures; however, it is currently called outside the
>   TF_MUTE check.
>
>   Move the tc_done routine inside the locked TF_MUTE check.
>
>   PR:   217282
>   Reviewed by:  ed, ray
>   Sponsored by: Netflix
>   Differential Revision:https://reviews.freebsd.org/D9709


Sorry, this should also say:

MFC after: 2 weeks

The change should go back to stable/11, since EARLY_AP_STARTUP is an option
there. (It appears EARLY_AP_STARTUP is a prerequisite for hitting this bug.)

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r307082 - in head: . sys/amd64/conf sys/arm/conf sys/arm64/conf sys/conf sys/i386/conf sys/mips/conf sys/modules/cc sys/modules/khelp sys/netinet sys/netinet/tcp_stacks sys/pc98/conf s

[Jonathan Looney]

Thanks!

Yes, that was clearly an error on my part, and your patch is an obvious
solution. I committed it at r307336.

Thanks again!

Jonathan

On Fri, Oct 14, 2016 at 8:01 PM, Shawn Webb 
wrote:

> On Wed, Oct 12, 2016 at 02:16:42AM +, Jonathan T. Looney wrote:
> > Author: jtl
> > Date: Wed Oct 12 02:16:42 2016
> > New Revision: 307082
> > URL: https://svnweb.freebsd.org/changeset/base/307082
> >
> > Log:
> >   In the TCP stack, the hhook(9) framework provides hooks for kernel
> modules
> >   to add actions that run when a TCP frame is sent or received on a TCP
> >   session in the ESTABLISHED state. In the base tree, this functionality
> is
> >   only used for the h_ertt module, which is used by the cc_cdg, cc_chd,
> cc_hd,
> >   and cc_vegas congestion control modules.
> >
> >   Presently, we incur overhead to check for hooks each time a TCP frame
> is
> >   sent or received on an ESTABLISHED TCP session.
> >
> >   This change adds a new compile-time option (TCP_HHOOK) to determine
> whether
> >   to include the hhook(9) framework for TCP. To retain backwards
> >   compatibility, I added the TCP_HHOOK option to every configuration
> file that
> >   already defined "options INET". (Therefore, this patch introduces no
> >   functional change. In order to see a functional difference, you need to
> >   compile a custom kernel without the TCP_HHOOK option.) This change will
> >   allow users to easily exclude this functionality from their kernel,
> should
> >   they wish to do so.
> >
> >   Note that any users who use a custom kernel configuration and use one
> of the
> >   congestion control modules listed above will need to add the TCP_HHOOK
> >   option to their kernel configuration.
> >
> >   Reviewed by:rrs, lstewart, hiren (previous version), sjg
> (makefiles only)
> >   Sponsored by:   Netflix
> >   Differential Revision:  https://reviews.freebsd.org/D8185
>
> This commit breaks the build when VNET is enabled. Attached is a
> candidate patch to fix.
>
> If the patch doesn't make it to the list, I've pasted it here:
> http://ix.io/1wbE
>
> Thanks,
>
> --
> Shawn Webb
> Cofounder and Security Engineer
> HardenedBSD
>
> GPG Key ID:  0x6A84658F52456EEE
> GPG Key Fingerprint: 2ABA B6BD EF6A F486 BE89  3D9E 6A84 658F 5245 6EEE
>
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r307082 - in head: . sys/amd64/conf sys/arm/conf sys/arm64/conf sys/conf sys/i386/conf sys/mips/conf sys/modules/cc sys/modules/khelp sys/netinet sys/netinet/tcp_stacks sys/pc98/conf s

[Jonathan Looney]

On Wed, Oct 12, 2016 at 5:44 AM, Slawa Olhovchenkov  wrote:

> Do you perform estimate of performane impact of this overhead?
>
>
Somewhat, but not precisely. It will depend on (at least) a few factors:

a. The hardware
   * How fast is your CPU? Your L1/L2/L3 cache?
   * How many CPUs?
   * How smart is the hardware prefetching?
   * How fast is the memory bus?
   * etc.
b. Whether VNET is enabled
   * If VNET is enabled, the hook lookup will require more instructions to
 find the correct list
c. Which TCP stack you are using
   * The hook lookup is implemented as an external function in the
 non-default stacks, so that requires the overhead of a function call
d. The probability that the number of hooks is in your CPU cache
   * This is a combination of the hardware and the exact workload on the
 device

On a test system running a fairly heavy load of TCP traffic using the
default TCP stack, without VNET, and without any TCP hooks installed, the
input/output hhook functions accounted for approximately .075% of the
"busy" CPU cycles during one of my measurements. That certainly is not a
large number, but it is large enough to be measurable. And, I think the
hardware I used for the measurement is tuned for high-performance, so
this may be close to the "best case" measurement.

I suspect that systems with a large number of short-lived sessions will
see a larger improvement from the deletion of khelp_init_osd() from the
session setup path. (I didn't measure this.)

I also suspect that systems with VNET will see a larger improvement.

I also suspect that systems with slower memory busses, smaller caches,
etc. will see a larger improvement.

But, this is very hard to measure precisely in a generic sense. The one
concrete thing we *can* measure is that the functions add some number
of instructions to the session setup, session teardown, input, and
output code paths. If you need those instructions to achieve the desired
functionality (in this case, probably a congestion-control algorithm),
then you probably want those instructions. If not, then you may want to
delete them. This was the impetus to add a kernel option to give you
the ability to decide whether you want to include the functionality.

I hope this answer helps explain some more about the change.

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r307083 - head/sys/netinet

[Jonathan Looney]

On Tue, Oct 11, 2016 at 10:30 PM, Jonathan T. Looney 
wrote:

> Author: jtl
> Date: Wed Oct 12 02:30:33 2016
> New Revision: 307083
> URL: https://svnweb.freebsd.org/changeset/base/307083
>
> Log:
>   Currently, when tcp_input() receives a packet on a session that matches a
>   TCPCB, it checks (so->so_options & SO_ACCEPTCONN) to determine whether or
>   not the socket is a listening socket. However, this causes the code to
>   access a different cacheline. If we first check if the socket is in the
>   LISTEN state, we can avoid accessing so->so_options when processing
> packets
>   received for ESTABLISHED sessions.
>
>   If INVARIANTS is defined, the code still needs to access both variables
> to
>   check that so->so_options is consistent with the state.
>
>   Reviewed by:  gallatin
>   MFC after:1 week
>   Sponsored by: Netflix
>

This should have also noted:
Differential Revision: https://reviews.freebsd.org/D8221

Sorry for the omission!

Jonathan
___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r296387 - head

[Jonathan Looney]

On Fri, Mar 4, 2016 at 12:49 AM, Warner Losh  wrote:
> It's trivial so worth having.  We should discuss what our "oldest
> supported upgrade" release should be as currently it is 8.1.
> 
> We put it to 8.1 based on Juniper wanted it for their operations.
> Normally we'd set this closer to 9.0 or something. If Juniper
> no longer needs it, we should move up to 9.0 since that's typically
> what we've done in the past at this point in the release cycle.

I don't think Juniper cares about this anymore. Even if we do, I'm not
sure why 8.1 would be the release we would choose (at least, at this
point).

Jonathan

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"

Re: svn commit: r289276 - in head/sys: conf kern netinet sys

[Jonathan Looney]

On 10/13/15, 10:33 PM, "Hiren Panchasara"  wrote:

>> 
>> I also replied to the review with style findings just now.
>>
>I'll ask Jonathan to look at it.

Thanks for the comments. I'll prepare a cleanup patch which addresses
this, and other comments which arrived after the commit.

Jonathan

___
svn-src-all@freebsd.org mailing list
https://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"