Date: Wed, 13 Oct 2010 13:04:54 +0200 From: Attilio Rao <attilio@freebsd.org> To: Robert Watson <rwatson@freebsd.org> Cc: FreeBSD Current <current@freebsd.org>, freebsd-net@freebsd.org, Nima Misaghian <nmisaghian@sandvine.com>, Sergey Kandaurov <pluknet@freebsd.org>, Jack F Vogel <jfv@freebsd.org>, Ryan Stone <rstone@sandvine.com>, Ed Maste <emaste@sandvine.com> Subject: Re: [PATCH] Netdump for review and testing -- preliminary version Message-ID: <AANLkTikaUL5g5ViaG763yq2=NGw9V-%2B6JL_%2BWxum8iAB@mail.gmail.com> In-Reply-To: <alpine.BSF.2.00.1010090121310.1232@fledge.watson.org> References: <AANLkTikA5OUYD1A9pqCqVEZ5qk%2BVECq8x-fnRXnpp0KE@mail.gmail.com> <AANLkTikau6omhWrXVM13zonFEPCxXM%2B8EqJauovDu0OU@mail.gmail.com> <alpine.BSF.2.00.1010090121310.1232@fledge.watson.org>
next in thread | previous in thread | raw e-mail | index | archive | help
2010/10/9 Robert Watson <rwatson@freebsd.org>: > On Fri, 8 Oct 2010, Attilio Rao wrote: > >>> GENERAL FRAMEWORK ARCHITECTURE >>> >>> Netdump is composed, right now, by an userland "server" and a kernel >>> "client". The former is run on the target machine (where the dump will >>> phisically happen) and it is responsible for receiving =C2=A0the packet= s >>> containing coredumps frame and for correctly writing them on-disk. The >>> latter is part of the kernel installed on the source machine (where the= dump >>> is initiated) and is responsible for building correctly UDP packets >>> containing the coredump frames, pushing through the network interface a= nd >>> routing them appropriately. > > Hi Attilio: > > Network dumps would be a great addition to the FreeBSD debugging suite! = =C2=A0A > few casual comments and questions, I need to spend some more time ponderi= ng > the implications of the current netdump design later in the week. > > (1) Did you consider using tftp as the network dump protocol, rather than= a > custom protocol? =C2=A0It's also a simple UDP-based, ACKed file transfer > protocol, with the advantage that it's widely supported by exiting tftp > daemons, packet sniffers, security devices, etc. =C2=A0This wouldn't requ= ire > using a stock tftpd, although that would certainly be a benefit as well. > =C2=A0The post-processing of crashdumps that seems to happen in the netdu= mp > server could, presumably, happen "offline" as easily...? I don't understand the "offline" question but, really, I don't know why tftp wasn't considered in the first place. Let me do some small search and see how much we could benefit from it. > (2) Have you thought about adding a checksum to the dump format, since > packets are going over the wire on UDP, etc? =C2=A0Even an MD5 sum wouldn= 't be > too hard to arrange: all the code is in the kernel already, requires > relatively little state storage, and is designed for streamed data. Someone else already brought this point and I agree, we could use a checksum here. > (3) As the bounds checking/etc behavior in dumping grows more complex, it > seems a shame to replicate it in architecture-specific code. =C2=A0Could = we pull > the mediaoffset/mediasize checking into common code? =C2=A0Also, a reserv= ed > size/offset of "0" meaning "no limit" sounds slightly worrying; it might = be > slightly more conservative to add a flags field to dumperinfo and have a > flag indicating "size is no object" for netdump, rather than overloading = the > existing fields. I don't agree with you here. The real problem is that dumpsys is highly disk-specific (I've commented about it somewhere, maybe the e-mail or maybe the commit logs). What we'd need is to have something like netdumpsys() which tries to use network, but it is not short to make and the mediasize+mediaoffset concept really rappresents an higher bound which can't be 0. I think it is a reasonable compromise so far but it is subjected to further improvements for sure. > Some specific patch comments: > > + * XXX: This should be split into machdep and non-machdep parts > > What MD parts are in the file? This is just a stale comment. > The sysctl parts of the patch have a number of issues: Sysctl are still not overhauled just because I'm not sure we want to keep them. That is one of the points I raised in the main e-mail and I'd really would like to hear opinions about if we should keep them rather than having a setup userland process, etc. I'm sorry about this, but please keep in mind that the file still needs a lot of cleanup so some comments are a bit out of date and other parts may not be still perfectly overhauled. > +sysctl_force_crash(SYSCTL_HANDLER_ARGS) > > Does this belong in the netdump code? =C2=A0We already have some of these= options > in debug.kdb.*, but perhaps others should be added there as well. For this specific case, I'd really axe it out rather. > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 /* > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* get and fill a= header mbuf, then chain data as an > extended > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* mbuf. > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/ > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 MGETHDR(m, M_DONTWAIT,= MT_DATA); > > The idea of calling into the mbuf allocator in this context is just freak= y, > and may have some truly awful side effects. =C2=A0I suppose this is the c= ost of > trying to combine code paths in the network device driver rather than hav= e > an independent path in the netdump case, but it's quite unfortunate and w= ill > significantly reduce the robustness of netdumps in the face of, for examp= le, > mbuf starvation. I'm not sure in which other way we could fix that actually. Maybe a pre-allocated pool of mbufs? > + =C2=A0 =C2=A0 =C2=A0 if (ntohs(ah->ar_hrd) !=3D ARPHRD_ETHER && > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ntohs(ah->ar_hrd) !=3D ARPHRD_IEEE80= 2 && > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ntohs(ah->ar_hrd) !=3D ARPHRD_ARCNET= && > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ntohs(ah->ar_hrd) !=3D ARPHRD_IEEE13= 94) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 NETDDEBUG("nd_handle_a= rp: unknown hardware address fmt " > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "0x%2D)\= n", (unsigned char *)&ah->ar_hrd, ""); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return; > + =C2=A0 =C2=A0 =C2=A0 } > > Are you sure you don't want to just check for ETHER here? I'd really like to hear Ryan's or Ed's idea here. > + =C2=A0 =C2=A0 =C2=A0 /* XXX: Probably should check if we're the recipie= nt MAC address */ > + =C2=A0 =C2=A0 =C2=A0 /* Done ethernet processing. Strip off the etherne= t header */ > > Yes, quite probably. =C2=A0What if you panic in promiscuous mode? > > + =C2=A0 =C2=A0 =C2=A0 /* > + =C2=A0 =C2=A0 =C2=A0 =C2=A0* The first write (at offset 0) is the kerne= l dump header. =C2=A0Flag it > + =C2=A0 =C2=A0 =C2=A0 =C2=A0* for the server to treat specially. =C2=A0X= XX: This doesn't strip out > the > + =C2=A0 =C2=A0 =C2=A0 =C2=A0* footer KDH, although it shouldn't hurt any= thing. > + =C2=A0 =C2=A0 =C2=A0 =C2=A0*/ > > The footer allows us to confirm that the tail end of a dump matches the > beginning; while probably not strictly necessary in this scenario, it's n= ot > a bad idea given the lack of a checksum. So I assume you are in favor of it, right? > + =C2=A0 =C2=A0 =C2=A0 /* Default the nic to the first available interfac= e */ > + =C2=A0 =C2=A0 =C2=A0 if ((ifn =3D TAILQ_FIRST(&ifnet)) !=3D NULL) do { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if ((ifn->if_flags & I= FF_UP) =3D=3D 0) > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 continue; > > More locking needed. Please refer to the second patch I posted. It should have proper locking and actually this code is just trimmed (more locking in V_ifnet accessings, but not in this codepath). > > - =C2=A0 =C2=A0 =C2=A0 void =C2=A0 =C2=A0*if_pspare[7]; > + =C2=A0 =C2=A0 =C2=A0 struct =C2=A0netdump_methods *if_ndumpfuncs; /* ne= tdump virtual methods > */ > + =C2=A0 =C2=A0 =C2=A0 void =C2=A0 =C2=A0*if_pspare[6]; > =C2=A0 =C2=A0 =C2=A0 =C2=A0int =C2=A0 =C2=A0 if_ispare[4]; > > In HEAD, at least, you should add your field and not use the spare. =C2= =A0The > spare should only be used on a merge to a KBI-stable branch (such as 8.x)= . Thanks, for picking this. > I need to ponder your changes to ifnet and to the drivers some more; I > recognize the benefits of maximal code alignment, but I worry a lot about > these code paths having side effects (not least, calls into memory > allocators, which in turn can enter VM, etc). =C2=A0I wonder if, given th= at, we > need to teach the mbuf allocator to be much more conservative if netdumpi= ng > is active... Sorry, which calls from drivers should get in the memory allocator? Are you just referring to the mbuf headers allocation? Changes to the drivers are mostly stright-forward -- they just try to do polling in locked or unlocked mode, following DDB entering or not (basically how other DDB-agnostic routines already do in FreeBSD for the known locking problems we discussed several times in the past). Thanks for your valuable feedback, looking forward to hear more. Attilio --=20 Peace can only be achieved by understanding - A. Einstein
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?AANLkTikaUL5g5ViaG763yq2=NGw9V-%2B6JL_%2BWxum8iAB>