From owner-freebsd-current@freebsd.org Thu Apr 11 04:56:25 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 42C0A157930D for ; Thu, 11 Apr 2019 04:56:25 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: from mail-qt1-x844.google.com (mail-qt1-x844.google.com [IPv6:2607:f8b0:4864:20::844]) (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 539EF755CF; Thu, 11 Apr 2019 04:56:24 +0000 (UTC) (envelope-from mjguzik@gmail.com) Received: by mail-qt1-x844.google.com with SMTP id x12so5655250qts.7; Wed, 10 Apr 2019 21:56:24 -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=4IlXWQvua4jRPpwdq0PM5BLSerUNgV8opYEEXaTCgJg=; b=lVML4VvkZ1j+MkFw2BnuDhuV0LBbziSZxNSP2HBoxAjNgoGIO4W+gpGak6D/eqy6mD XPY1bO/hcsnRw2eoYjhQ8A5jX+wWhXfR/hUw/PT5DyqeabGlfN4U7gjNxntOf4JteBtp 8Y4pdZZzCBbMIDpufPF9zx0ZGvjusmKaDDo3wd15mwomYJD77hhyAbWXLOAx4wiZGyen eFFj1iDuphiXtn66tzf0nYzaFE5mg937XCk23ppyKnTwbY2PM0TJ0z7G2Pt+xSh+0S8t 8+d/1wvYL9WE5jKpmlCGSlwFi6iI1K4DnIGp0VtM9v8ul0qSCCEUk5AQLloXCutb6O24 0HNQ== 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=4IlXWQvua4jRPpwdq0PM5BLSerUNgV8opYEEXaTCgJg=; b=r2LGZ7QAB+xRg1J7rNh/4cTbx4AxAcA2IN5ClLa3KKrfcShPKxmtesiRjBL0RJqnz8 kpRtJ5T0rOYFe/jJ+1S1ZnYYf+70ALro0tqtXgMY8pnlBOhFCldcOcixNEsbyuEzqcVf OO1t2GujYwBRtOvDANa55Lgm6MyCjRLriVvVqlQixQpmS78SR2Qg5zUph5UCnIKcH2xM Vhqe4EJhZQeAJ4EmLQHRJ9AvjK2EVwjiBEN+hLxtcZlLuHVFzqql2yLKBJRruOUnXI9F yavNgWiq/+XDTAKafYvyiVFKEU/FDf5RW6tbYR0dqa82oa4IlSwbMGNIuiWYLQTC1U/x x3TQ== X-Gm-Message-State: APjAAAVz32bOV0wNB175d9zToQXrYkc3o9VKsGq5VNpUIHNz/jGJzb83 /cZhYANz8RMEShPx+VDU9MV7YE2lzh7Stsq7VPc= X-Google-Smtp-Source: APXvYqxsPG21krDxMubb9+J8EAbADPbIC1HGqb8bO+JBcDLaF13Ri9Dq6aGxA4MXO6sbFAa1D6XlUQ/jGdLMWb1Ec+M= X-Received: by 2002:ac8:27b0:: with SMTP id w45mr39715019qtw.341.1554958583526; Wed, 10 Apr 2019 21:56:23 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:ac8:3353:0:0:0:0:0 with HTTP; Wed, 10 Apr 2019 21:56:22 -0700 (PDT) In-Reply-To: References: From: Mateusz Guzik Date: Thu, 11 Apr 2019 06:56:22 +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: 539EF755CF X-Spamd-Bar: -- Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20161025 header.b=lVML4Vvk; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (mx1.freebsd.org: domain of mjguzik@gmail.com designates 2607:f8b0:4864:20::844 as permitted sender) smtp.mailfrom=mjguzik@gmail.com X-Spamd-Result: default: False [-2.80 / 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:+]; MX_GOOD(-0.01)[cached: alt3.gmail-smtp-in.l.google.com]; DMARC_POLICY_ALLOW(-0.50)[gmail.com,none]; NEURAL_HAM_SHORT(-0.16)[-0.159,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.8.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.63)[ip: (2.07), 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 04:56:25 -0000 On 4/11/19, Rick Macklem wrote: > Mateusz Guzik wrote: >>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. > Long ago when I first did the NFSv4 code for OpenBSD2.6, I had a callback > function > pointer in "struct proc" which the NFS code set non-null to get a callback. > { The code still has remnants of that because it still has > nfscl_cleanup_common(), > which was code shared by that callback and this approach which was used > for > the Mac OS X port, where I couldn't change "struct proc". } > I have never added anything like that for FreeBSD, but I suppose we could > look > at doing it that way. > To be honest, since the current code works fine and can be difficult to test > well, > I hesitate to change to using a callback. > >>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... > Hmm, so you are saying that every element of the proc_zone always has a > valid > p_mtx field in it that can be safely PROC_LOCK()'d no matter if the element > refers to a process at that time? > I would also need help with the code to determine if the structure refers > to > a process that currently exists with the same pid and creation time. You can just: PROC_LOCK(p); if (p->p_state != PRS_NORMAL) { /* it's not the one you want, bail */ } /* the current timestamp check goes here */ PROC_UNLOCK(p); > > I suppose saving "p" with the lock/open owner string and then doing what > you > suggest is possible, but it would take some work. > > For now, I can just grab all the pidhashtbl_locks once/sec and fix head so > it works. > -- Mateusz Guzik