Date: Sat, 8 Sep 2012 21:00:28 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Attilio Rao <attilio@FreeBSD.org> Cc: Davide Italiano <davide@FreeBSD.org>, svn-src-projects@FreeBSD.org, src-committers@FreeBSD.org, Andriy Gapon <avg@FreeBSD.org> Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern Message-ID: <20120908180028.GJ33100@deviant.kiev.zoral.com.ua> In-Reply-To: <CAJ-FndD3xxV8gPv3czBnS-9VGXt5QV0JZDfRyvT=j8WHRA8W-w@mail.gmail.com> References: <20120730143943.GY2676@deviant.kiev.zoral.com.ua> <CAJ-FndByYcZ%2BUhnkFT_n2=W=UheqUCi0%2BUAX%2BF07EqbVU=6iDQ@mail.gmail.com> <CAJ-FndCQ6HGAfFdjofNfJ%2BHeNaE7uqoNhJB9GH4pGFxyZ_1yLg@mail.gmail.com> <5016A21B.6090409@FreeBSD.org> <CAJ-FndCFjZP=0ThpMxy6WSDQAZOm0TRkyu0bWfxVBwtT-h%2B1cA@mail.gmail.com> <5016A8E4.7070405@FreeBSD.org> <CAJ-FndB0zc-TC0E=H4p1qcOB4ngEWtwXoyhScf68G8i0p5UErw@mail.gmail.com> <CAJ-FndDAFmrEBW3d29cj-CZoPT%2BD5UPFadzL6i9BNH9ztzsJ%2BQ@mail.gmail.com> <20120908153839.GF33100@deviant.kiev.zoral.com.ua> <CAJ-FndD3xxV8gPv3czBnS-9VGXt5QV0JZDfRyvT=j8WHRA8W-w@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
--R8jsNM7kDXYEcGgQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Sep 08, 2012 at 06:14:41PM +0100, Attilio Rao wrote: > On Sat, Sep 8, 2012 at 4:38 PM, Konstantin Belousov <kostikbel@gmail.com>= wrote: > > On Fri, Sep 07, 2012 at 11:35:15PM +0100, Attilio Rao wrote: > >> On Mon, Jul 30, 2012 at 9:39 PM, Attilio Rao <attilio@freebsd.org> wro= te: > >> > On Mon, Jul 30, 2012 at 4:31 PM, Andriy Gapon <avg@freebsd.org> wrot= e: > >> >> on 30/07/2012 18:04 Attilio Rao said the following: > >> >>> On 7/30/12, Andriy Gapon <avg@freebsd.org> wrote: > >> >>>> on 30/07/2012 17:56 Attilio Rao said the following: > >> >>>>> More explicitly, I think such combination TDP_NOSLEEPING + > >> >>>>> TDP_NOBLOCKING (name invented) should be set on entering the int= errupt > >> >>>>> context, not only related to this part of callouts. This would b= e a > >> >>>>> very good help for catching buggy situations. > >> >>>> > >> >>>> Something very tangential. I think it would also be nice to chec= k if a > >> >>>> thread has > >> >>>> any(?) locks held when returning to userland. > >> >>> > >> >>> This happens already for INVARIANTS case, with td_locks counters. > >> >>> In the !INVARIANTS case, this doesn't happen because you don't wan= t to > >> >>> add the burden to bump td_locks for the fast case and I think it i= s a > >> >>> good approach. > >> >> > >> >> Ah, I missed that, thank you. > >> >> BTW, it seems that td_locks is checked twice in normal syscallret()= path: once in > >> >> syscallret() itself and then in userret(). On this note, would it = make sense to > >> >> move the whole nine yards of asserts from syscallret() to userret()? > >> >> I mean it might make sense to have those checks (td_critnest, td_pf= lags) in other > >> >> paths to userland. > >> > > >> > Nice catch. > >> > The checks were added to syscallret() in r208453. While this is fine, > >> > I think that putting them in userret() may give them more exposure a= nd > >> > cover also cases like traps which are not covered right now. > >> > If you want to make a patch that moves these conditions in userret() > >> > I'd be in favor of it. > >> > >> More specifically, what do you think about this patch?: > >> http://www.freebsd.org/~attilio/userret_diag.patch > >> > >> Of course I moved the XEN par too before the checks. > >> The patch survived to few consecutive and parallel buildworlds, FWIW. > > > > At least in fork_return(), the last assert which checks that Giant is n= ot > > held, is no longer needed. >=20 > Actually, this is unnecessary also with -CURRENT stock code today, > because userret() already checks for td_locks. And it seems > fork_return() is not the only function where this happens, as trap() > did this too on x86 and possibly all the other architectures grew it > with cut&paste. Possibly we need this further, separate, patch before > userred_diag: > http://www.freebsd.org/~attilio/userret_nogiant.patch Yes, this looks good. I suggest to move #ifdef XEN part of the userret() to befire KASSERT(td->td_locks =3D=3D 0). >=20 > > I had similar thought from the time when I added TDP_NOFAULTING check t= o the > > syscallret(), but the loss of the syscall name in the panic output alwa= ys > > stopped me. >=20 > I think in case of a lock/td_pinned/td_critnest/etc. leak, having the > syscall number in the panic message won't change anything as you will > likely need a coredump and possibly instrument your kernel with > further debugging, etc. to see what's going on. For me, when I debugged TDP_NOFAULTING (which indeed leaked in the intermediate versions of the patch) the syscall name appeared to be enough. But, I do not object strongly. --R8jsNM7kDXYEcGgQ Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (FreeBSD) iEYEARECAAYFAlBLh7wACgkQC3+MBN1Mb4i/KQCg8BVm0w9GU1THNDTnBS+PEKn/ DWkAoMfMe9D9ujRlffU9U6GUIPx6WT7f =1f6K -----END PGP SIGNATURE----- --R8jsNM7kDXYEcGgQ--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120908180028.GJ33100>