From nobody Mon May 01 04:01:44 2023 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4Q8qKL2XVSz47k6N; Mon, 1 May 2023 04:01:58 +0000 (UTC) (envelope-from byond.lenox@gmail.com) Received: from mail-qv1-f42.google.com (mail-qv1-f42.google.com [209.85.219.42]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Q8qKK21y8z3RBy; Mon, 1 May 2023 04:01:57 +0000 (UTC) (envelope-from byond.lenox@gmail.com) Authentication-Results: mx1.freebsd.org; dkim=none; spf=pass (mx1.freebsd.org: domain of byond.lenox@gmail.com designates 209.85.219.42 as permitted sender) smtp.mailfrom=byond.lenox@gmail.com; dmarc=none Received: by mail-qv1-f42.google.com with SMTP id 6a1803df08f44-61af33bdf1dso3103426d6.2; Sun, 30 Apr 2023 21:01:57 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1682913715; x=1685505715; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=SEtNwz/dw/MqjA1dPhaef+AK/Kg+mhLEjFM5l9cXumo=; b=A1BDoib4z1WMSaC8Q35aOxBtLHRLjC7AulCv8G7+GQdWlmCzEKCq99HMyD7wLWxUAc Czd+oMiypkKo6gEhGkKym7lxYIUw4fSvj3pQtz2L6Ii+ij/cA2SugLoyeFxUX554e8mn ZK2Ap4Cx1ZyBzALMih19H97PqOYebJeXHly+V65sZPYwG55Ycan9owFYc7XzDUwF5X1S h+nJCLLG6dssscURNobE9ZAHOis09p7hMbhHx31jFchpUKwSD4Fn5QU/CavVJkFTv87A /veoJkyNIYSLiA7rwTaVmIvvGw/P3ySSH4SKMqhhoeU2QAQyUPa+4d6/y0K7FUrs3thH N29A== X-Gm-Message-State: AC+VfDwkMsgIlD4UAq2z9StnXHX/DWCo/dTTvStCIo2JL89XQJ++am9A xvHfPB2SsfdL4QGaQg3fnN77YNYrJmc= X-Google-Smtp-Source: ACHHUZ7tFMGp4achLCstx3m4POK7XNKM5blXw2I3bnbDC9ivFqWeuH0cc9yaLVv/TsO/Mztf4NTxzQ== X-Received: by 2002:a05:6214:d4e:b0:5bf:ba9d:8726 with SMTP id 14-20020a0562140d4e00b005bfba9d8726mr22047094qvr.10.1682913715524; Sun, 30 Apr 2023 21:01:55 -0700 (PDT) Received: from mail-qv1-f49.google.com (mail-qv1-f49.google.com. [209.85.219.49]) by smtp.gmail.com with ESMTPSA id b5-20020a0cf045000000b0061b5d3a61aesm16684qvl.9.2023.04.30.21.01.55 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 30 Apr 2023 21:01:55 -0700 (PDT) Received: by mail-qv1-f49.google.com with SMTP id 6a1803df08f44-61af33bdf1dso3103396d6.2; Sun, 30 Apr 2023 21:01:55 -0700 (PDT) X-Received: by 2002:a05:6214:248f:b0:61a:5664:f8e9 with SMTP id gi15-20020a056214248f00b0061a5664f8e9mr5094664qvb.51.1682913714963; Sun, 30 Apr 2023 21:01:54 -0700 (PDT) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <202101100241.10A2fvtm057663@gitrepo.freebsd.org> In-Reply-To: <202101100241.10A2fvtm057663@gitrepo.freebsd.org> From: Kyle Evans Date: Sun, 30 Apr 2023 23:01:44 -0500 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: git: 5844bd058aed - main - jobc: rework detection of orphaned groups. To: Konstantin Belousov Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spamd-Result: default: False [-2.77 / 15.00]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-0.94)[-0.941]; NEURAL_HAM_MEDIUM(-0.82)[-0.824]; FORGED_SENDER(0.30)[self@kyle-evans.net,byondlenox@gmail.com]; R_SPF_ALLOW(-0.20)[+ip4:209.85.128.0/17]; MIME_GOOD(-0.10)[text/plain]; RCVD_IN_DNSWL_NONE(0.00)[209.85.219.42:from,209.85.219.49:received]; MLMMJ_DEST(0.00)[dev-commits-src-all@freebsd.org,dev-commits-src-main@freebsd.org]; MIME_TRACE(0.00)[0:+]; RWL_MAILSPIKE_POSSIBLE(0.00)[209.85.219.42:from]; FREEMAIL_ENVFROM(0.00)[gmail.com]; R_DKIM_NA(0.00)[]; RCVD_COUNT_THREE(0.00)[4]; FROM_NEQ_ENVFROM(0.00)[self@kyle-evans.net,byondlenox@gmail.com]; DMARC_NA(0.00)[kyle-evans.net]; RCPT_COUNT_THREE(0.00)[4]; ARC_NA(0.00)[]; RCVD_VIA_SMTP_AUTH(0.00)[]; ASN(0.00)[asn:15169, ipnet:209.85.128.0/17, country:US]; TO_DN_SOME(0.00)[]; FROM_HAS_DN(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; RCVD_TLS_LAST(0.00)[]; TAGGED_FROM(0.00)[] X-Rspamd-Queue-Id: 4Q8qKK21y8z3RBy X-Spamd-Bar: -- X-ThisMailContainsUnwantedMimeParts: N On Sat, Jan 9, 2021 at 8:43=E2=80=AFPM Konstantin Belousov wrote: > > The branch main has been updated by kib: > > URL: https://cgit.FreeBSD.org/src/commit/?id=3D5844bd058aed6f3d0c8cbbddd6= aa95993ece0189 > > commit 5844bd058aed6f3d0c8cbbddd6aa95993ece0189 > Author: Konstantin Belousov > AuthorDate: 2020-12-29 00:41:56 +0000 > Commit: Konstantin Belousov > CommitDate: 2021-01-10 02:41:20 +0000 > > jobc: rework detection of orphaned groups. > > Instead of trying to maintain pg_jobc counter on each process group > update (and sometimes before), just calculate the counter when needed= . > Still, for the benefit of the signal delivery code, explicitly mark > orphaned groups as such with the new process group flag. > > This way we prevent bugs in the corner cases where updates to the cou= nter > were missed due to complicated configuration of p_pptr/p_opptr/real_p= arent > (debugger). > > Since we need to iterate over all children of the process on exit, th= is > change mostly affects the process group entry and leave, where we nee= d > to iterate all process group members to detect orpaned status. > Hi, I have a question about the locking here... > [... snip ...] > @@ -678,43 +677,40 @@ jobc_reaper(struct proc *p) > } > > static struct proc * > -jobc_parent(struct proc *p) > +jobc_parent(struct proc *p, struct proc *p_exiting) > { > struct proc *pp; > > - sx_assert(&proctree_lock, SX_LOCKED); > + sx_assert(&proctree_lock, SA_LOCKED); > > pp =3D proc_realparent(p); > - if (pp->p_pptr =3D=3D NULL || > + if (pp->p_pptr =3D=3D NULL || pp =3D=3D p_exiting || > (pp->p_treeflag & P_TREE_GRPEXITED) =3D=3D 0) > return (pp); > return (jobc_reaper(pp)); > } > > -#ifdef INVARIANTS > -static void > -check_pgrp_jobc(struct pgrp *pgrp) > +static int > +pgrp_calc_jobc(struct pgrp *pgrp) > { > struct proc *q; > int cnt; > > - sx_assert(&proctree_lock, SX_LOCKED); > - PGRP_LOCK_ASSERT(pgrp, MA_NOTOWNED); > +#ifdef INVARIANTS > + if (!mtx_owned(&pgrp->pg_mtx)) > + sx_assert(&proctree_lock, SA_LOCKED); > +#endif > > cnt =3D 0; > - PGRP_LOCK(pgrp); > LIST_FOREACH(q, &pgrp->pg_members, p_pglist) { > if ((q->p_treeflag & P_TREE_GRPEXITED) !=3D 0 || > q->p_pptr =3D=3D NULL) > continue; > - if (isjobproc(jobc_parent(q), pgrp)) > + if (isjobproc(jobc_parent(q, NULL), pgrp)) > cnt++; > } > - KASSERT(pgrp->pg_jobc =3D=3D cnt, ("pgrp %d %p pg_jobc %d cnt %d"= , > - pgrp->pg_id, pgrp, pgrp->pg_jobc, cnt)); > - PGRP_UNLOCK(pgrp); > + return (cnt); > } > -#endif > > /* > * Move p to a process group > [... snip ...] So the proctree lock is sufficient for pgrp_calc_jobc() to provide a stable answer needed for everything fixjobc_kill() uses it for... > @@ -934,35 +827,46 @@ fixjobc_kill(struct proc *p) > p->p_treeflag |=3D P_TREE_GRPEXITED; > > /* > - * Check p's parent to see whether p qualifies its own process > - * group; if so, adjust count for p's process group. > + * Check if exiting p orphans its own group. > */ > - if (isjobproc(jobc_parent(p), pgrp)) > - pgadjustjobc(pgrp, false); > + pgrp =3D p->p_pgrp; > + if (isjobproc(jobc_parent(p, NULL), pgrp)) { > + PGRP_LOCK(pgrp); > + if (pgrp_calc_jobc(pgrp) =3D=3D 0) > + orphanpg(pgrp); > + PGRP_UNLOCK(pgrp); > + } > > /* > * Check this process' children to see whether they qualify > - * their process groups after reparenting to reaper. If so, > - * adjust counts for children's process groups. > + * their process groups after reparenting to reaper. > */ > LIST_FOREACH(q, &p->p_children, p_sibling) { > - if ((q->p_treeflag & P_TREE_ORPHANED) !=3D 0) > - continue; > - fixjobc_kill_q(p, q, true); > + pgrp =3D q->p_pgrp; > + PGRP_LOCK(pgrp); > + if (pgrp_calc_jobc(pgrp) =3D=3D 0) { > + /* > + * We want to handle exactly the children that > + * has p as realparent. Then, when calculating > + * jobc_parent for children, we should ignore > + * P_TREE_GRPEXITED flag already set on p. > + */ > + if (jobc_parent(q, p) =3D=3D p && isjobproc(p, pg= rp)) > + orphanpg(pgrp); > + } else > + pgrp->pg_flags &=3D ~PGRP_ORPHANED; > + PGRP_UNLOCK(pgrp); > } > - LIST_FOREACH(q, &p->p_orphans, p_orphan) > - fixjobc_kill_q(p, q, true); > - LIST_FOREACH(q, &p->p_children, p_sibling) { > - if ((q->p_treeflag & P_TREE_ORPHANED) !=3D 0) > - continue; > - fixjobc_kill_q(p, q, false); > + LIST_FOREACH(q, &p->p_orphans, p_orphan) { > + pgrp =3D q->p_pgrp; > + PGRP_LOCK(pgrp); > + if (pgrp_calc_jobc(pgrp) =3D=3D 0) { > + if (isjobproc(p, pgrp)) > + orphanpg(pgrp); > + } else > + pgrp->pg_flags &=3D ~PGRP_ORPHANED; > + PGRP_UNLOCK(pgrp); > } > - LIST_FOREACH(q, &p->p_orphans, p_orphan) > - fixjobc_kill_q(p, q, false); > - > -#ifdef INVARIANTS > - check_pgrp_jobc(pgrp); > -#endif > } > > void ... and I would imagine the proctree lock is sufficient for isjobproc/jobc_parent as well- is there any reason we can't/shouldn't just use atomic(9) for operations with pgrp->pg_flags and push all of the extra lock acquisitions into the orphanpg() cases? It seems like we could avoid taking any pgrp lock in the common case and at least mitigate that additional overhead from the exit() path. Thanks, Kyle Evans