From owner-freebsd-current@freebsd.org Thu Apr 11 01:49:33 2019 Return-Path: Delivered-To: freebsd-current@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 0A4301574324 for ; Thu, 11 Apr 2019 01:49:33 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-qk1-x744.google.com (mail-qk1-x744.google.com [IPv6:2607:f8b0:4864:20::744]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "GTS CA 1O1" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 230646EB3B; Thu, 11 Apr 2019 01:49:32 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-qk1-x744.google.com with SMTP id o129so2510924qke.8; Wed, 10 Apr 2019 18:49:32 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=uipVgq/RBBxwfCt++UEEj9xjz8vy7Z1AVMBJ/tB+WAs=; b=JLW46ihneAjeOx9AJdZehfbk1Wu6ILaAuTzGEGOvBDn0Z+xxsttP/7uQBkMkmOkuJM gmmsioDeWdi3XlXouLn2udYgpj9QHSXf+GjIvh3xOGasCFLJchF1Is1t6QoloygBGaKq /Ae05hOhe0q5egTfpduJnXlTS4R2FUCAqdSi+ahPQgEv/bifW8in1R1RgcYLdd8WC/r2 iaYWPWuSlJ8JJ9JBFThuhKl2KzP7AcEPQbQ+jLvzI2i6UCyZDvxH41BOgoRRuz6pVza1 dgq0A0B1QumVuYKyZ5JyouMJ2xCmR6JdruAN7heaCuOvZVUbHyc87GNcmZHPBXE1J14/ 1gUA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=uipVgq/RBBxwfCt++UEEj9xjz8vy7Z1AVMBJ/tB+WAs=; b=BCnA4WOQwBG8nr7w7z62zYrq5wAuVj9//IACvcta9X0dVriRIoXtd17R8tWneJcgqd jjP3QnQJNfIvEjFNc7dfrXoRXcu6ZOPTcdn5LJqil7UcKJDqx8zXmI22LrQhN74BIf39 B0HXzpUapZBvnryWviZFmycvOuoDrTeA2WlZIiHZGFihhqagU+Dgxhtg9CCyf3Fe3OGZ CFO50vBZ+wYIdhHPDrh4zh0TgPUJ4bSXHlgqhjNdLjEUAFJ/ag8x/WNAtcic+Wl2ubu0 6CN7k5lPiscqFqOuNDCEE5wPi5jnfvvGd7GXOo+W5wB15biI1VnqYhzL6e4wqeJLBeit m2cQ== X-Gm-Message-State: APjAAAVJNXSRiAeqfBe83b+jXBeGM0FkZnna9Pf2aEpJAErAc244ed7S jqLPYsSKvKcL2Qa41wDVTBtI+ZrVQTfyFf92/UI= X-Google-Smtp-Source: APXvYqwGjjKsYHalieeSmUX26WJ1QrLs589EF3VP5x/ZOWzysU6jhjTILCG/j28qDbUBTGpejM8I/YD2jnlYKQBOuFQ= X-Received: by 2002:a05:620a:126d:: with SMTP id b13mr36286606qkl.174.1554947371756; Wed, 10 Apr 2019 18:49:31 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac8:3353:0:0:0:0:0 with HTTP; Wed, 10 Apr 2019 18:49:31 -0700 (PDT) In-Reply-To: References: From: Mateusz Guzik Date: Thu, 11 Apr 2019 03:49:31 +0200 Message-ID: Subject: Re: Do the pidhashtbl_locks added by r340742 need to be sx locks? To: Rick Macklem Cc: "kib@freebsd.org" , "freebsd-current@FreeBSD.org" Content-Type: text/plain; charset="UTF-8" X-Rspamd-Queue-Id: 230646EB3B X-Spamd-Bar: --- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20161025 header.b=JLW46ihn; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of mjguzik@gmail.com designates 2607:f8b0:4864:20::744 as permitted sender) smtp.mailfrom=mjguzik@gmail.com X-Spamd-Result: default: False [-3.24 / 15.00]; TO_DN_EQ_ADDR_SOME(0.00)[]; TO_DN_SOME(0.00)[]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36]; FREEMAIL_FROM(0.00)[gmail.com]; RCVD_COUNT_THREE(0.00)[3]; DKIM_TRACE(0.00)[gmail.com:+]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; MX_GOOD(-0.01)[cached: alt3.gmail-smtp-in.l.google.com]; NEURAL_HAM_SHORT(-0.54)[-0.543,0]; FROM_EQ_ENVFROM(0.00)[]; SUBJECT_ENDS_QUESTION(1.00)[]; MIME_TRACE(0.00)[0:+]; FREEMAIL_ENVFROM(0.00)[gmail.com]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; DWL_DNSWL_NONE(0.00)[gmail.com.dwl.dnswl.org : 127.0.5.0]; ARC_NA(0.00)[]; NEURAL_HAM_MEDIUM(-1.00)[-0.999,0]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20161025]; FROM_HAS_DN(0.00)[]; RCPT_COUNT_THREE(0.00)[3]; NEURAL_HAM_LONG(-1.00)[-1.000,0]; MIME_GOOD(-0.10)[text/plain]; RCVD_TLS_LAST(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; RCVD_IN_DNSWL_NONE(0.00)[4.4.7.0.0.0.0.0.0.0.0.0.0.0.0.0.0.2.0.0.4.6.8.4.0.b.8.f.7.0.6.2.list.dnswl.org : 127.0.5.0]; IP_SCORE(-0.69)[ip: (1.77), ipnet: 2607:f8b0::/32(-2.96), asn: 15169(-2.19), country: US(-0.06)] X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Apr 2019 01:49:33 -0000 On 4/11/19, Rick Macklem wrote: > Hi, > > I finally got around to looking at what effect replacing pfind_locked() > with > pfind() has for the NFSv4 client and it is broken. > > The problem is that the NFS code needs to call some variant of "pfind()" > while > holding a mutex lock. The current _pfind() code uses the pidhashtbl_locks, > which are "sx" locks. > > There are a few ways to fix this: > 1 - Create a custom version of _pfind() for the NFS client with the sx_X() > calls > removed, plus replace the locking of allproc_lock with locking of all > the > pidhashtbl_locks, so that the "sx" locks are acquired before the > mutex. > --> Not very efficient, but since it is only done once/sec, I can live > with it. > 2 - Similar to the above, but still lock the allproc_lock and use a loop of > FOREACH_PROC_IN_SYSTEM(p) instead of a hash list for the pid in the > custom pfind(). (I don't know if this would be preferable to locking > all > the pidhashtbl_locks for other users of pfind()?) > 3 - Convert the pidhashtbl_locks to mutexes. Then the NFS client doesn't > need > to acquire any proc related locks and it just works. > I can't see anywhere that "sleeps" while holding the pidhashtbl_locks, > so I > think they can be converted, although I haven't tried it yet? > > From my perspective, #3 seems the better solution. > What do others think? > Changing the lock type to rwlock may be doable and worthwhile on its own, but I don't think it would constitute the right fix. Preferably there would be an easy to use mechanism which allows registering per-process callbacks. Currently it can be somewhat emulated with EVENTHANDLERs, but it would give calls for all exiting processes, not only the ones of interest. Then there would be no need to periodically scan as you would always get notified on process exit. Note the current code does not ref processes it is interested in any manner and just performs a timestamp check to see if it got the one it expected (with pid reuse in mind). So I think a temporary hack which will do the trick will take the current approach further: rely on struct proc being type-stable (i.e. never being freed) and also store the pointer. You can always safely PROC_LOCK it, do checks to see the proc is alive and has the right timestamp, all without needing to pfind or similar. -- Mateusz Guzik