From owner-svn-src-head@FreeBSD.ORG Wed Sep 18 15:06:35 2013 Return-Path: Delivered-To: svn-src-head@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTP id D7A62434; Wed, 18 Sep 2013 15:06:34 +0000 (UTC) (envelope-from davide.italiano@gmail.com) Received: from mail-vc0-x22b.google.com (mail-vc0-x22b.google.com [IPv6:2607:f8b0:400c:c03::22b]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (No client certificate requested) by mx1.freebsd.org (Postfix) with ESMTPS id 5FB332026; Wed, 18 Sep 2013 15:06:34 +0000 (UTC) Received: by mail-vc0-f171.google.com with SMTP id ij15so5393113vcb.16 for ; Wed, 18 Sep 2013 08:06:33 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=jKGMUFwgkpndrc0PkWIPgQoOH5VuaECZVUDA4ETW4mA=; b=oDjRkreeJVkezMHFEYgbg93TgXNPIm+RKl7iTZ+IKDk9hdqscTbIkY5VFOc2NBlb/A 3VJ+nATZIEwLotcwm7w2TPoxTvxPtFlAo/FpnOWKxlWSIKthhUh1sx+VlF0wdeCKCcTv TrcIgzXxfc8snU3tykPboouv1+U119OeO12TFitGX7LdHtPVs8CiqcOlURykkvGsTkkR tTYUu6aJFmu/gksaXqb0E5ldwks3FQorQSHGXvC3Kub/P7tZqXZ+qHUIEWLOl6HM3WQw zwL2aTqxn9vrGveorDEQOszOoRBFrBgQDD6VeFa5VfxB3TJyWddTb6BbAxoiRoZB+d9F 5JNA== MIME-Version: 1.0 X-Received: by 10.58.44.37 with SMTP id b5mr38565817vem.4.1379516793497; Wed, 18 Sep 2013 08:06:33 -0700 (PDT) Sender: davide.italiano@gmail.com Received: by 10.220.65.132 with HTTP; Wed, 18 Sep 2013 08:06:33 -0700 (PDT) In-Reply-To: <201309121008.01115.jhb@freebsd.org> References: <201308231412.r7NECdG7081565@svn.freebsd.org> <201308261502.13277.jhb@freebsd.org> <201309121008.01115.jhb@freebsd.org> Date: Wed, 18 Sep 2013 17:06:33 +0200 X-Google-Sender-Auth: ENEqLqRVawH1oFBiH5N_CmSCgNI Message-ID: Subject: Re: svn commit: r254703 - in head: share/man/man9 sys/sys From: Davide Italiano To: John Baldwin Content-Type: text/plain; charset=ISO-8859-1 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 18 Sep 2013 15:06:35 -0000 On Thu, Sep 12, 2013 at 4:08 PM, John Baldwin wrote: > Hmm, I think I had envisioned something a bit simpler. Namely, I would > change lc_lock/lc_unlock to return a uintptr_t instead of an int, and > I would then change lc_unlock for an rm lock to return a pointer to the > current thread's tracker as the 'how' and 0 for a write lock. Note > that you have to use the existing tracker to make this work correctly > for the sleep case where you unlock/lock. > Well, your solution is indeed a lot simpler :) Here's a patch that implements it: http://people.freebsd.org/~davide/review/rmshared.diff Some (more or less relevant) observations: -> I realized that before this change lc_unlock() for rmlocks, i.e. unlock_rm(), returned 1 for exclusive lock and panic'ed otherwise, while all the other primitives returned 0 for exclusive lock. Not sure if this was intentional or just an oversight, but I changed it to return what other primitive did mostly (0 for excl, (uintptr_t)rm_tracker for shared). -> In order to get the rm_priotracker structure for curthread (which I think is unique as long as we assert curthread is not recursively acquiring this lock) I just traversed the pc->pc_rm_queue. Maybe it's not the most efficient solution here, but I think is correct. -> I think that only lc_unlock() return type need to be changed from 'int' to 'uintptr_t'. lc_lock() type can stay void as it is right now. But I think you were talking about "how" argument of lc_lock() and not return type, probably. > However, if you make my suggested change to make the 'how' a uintptr_t > that passes the tracker you can actually do this in the callout case: > > struct rm_priotracker tracker; > uintptr_t how; > > how = 0; > if (flags & CALLOUT_SHAREDLOCK) > how = 1; > else if (flags & CALLOUT_SHAREDRM) > how = (uintptr_t)&tracker; > ... > > class->lc_lock(lock, how); > > Now, it would be even nicer to make prevent footshooting perhaps by > checking the lock class directly: > > how = 0; > if (flags & CALLOUT_SHAREDLOCK) { > if (class == &lock_class_rm || class == &lock_class_rm_sleepable) > how = (uintptr_t)&tracker; > else > how = 1; > } > > -- > John Baldwin This other patch just puts your code into kern_timeout.c I also removed the check for lock_class_rm_sleepable as callout_init() should catch this earlier. http://people.freebsd.org/~davide/review/callout_sharedrm.diff Thanks for the guidance on this. I probably commit this in the next days if you don't have objections. -- Davide "There are no solved problems; there are only problems that are more or less solved" -- Henri Poincare