From owner-svn-src-head@freebsd.org Sat Oct 14 09:47:38 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 8A26BE42F4A; Sat, 14 Oct 2017 09:47:38 +0000 (UTC) (envelope-from onwahe@gmail.com) Received: from mail-wm0-f65.google.com (mail-wm0-f65.google.com [74.125.82.65]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 251016AA9C; Sat, 14 Oct 2017 09:47:37 +0000 (UTC) (envelope-from onwahe@gmail.com) Received: by mail-wm0-f65.google.com with SMTP id q124so25312095wmb.0; Sat, 14 Oct 2017 02:47:37 -0700 (PDT) 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=mpK+llHje6RNi7gY1C/bEDuPGt/xW0V8xLkxBh1tBlE=; b=fnQWSwj5e0V2h2a77CEilM+iJXkCA7Je0B60sTWitled1XCPfghDPd3CB8h8OlVhb5 LUbCqxUP4flKEMZkdgexcgWRCg32DrGIuBF1jko3lNEgdoA9kax2k8LouIjkkbgN3bSq dz7bGzkayMATwOsuLN4S+fQzynSlY+n9ytNR+41Nqe9TBMIlSqLGVD7h2TOZz4B9Y1cd LNBL3QFpLYrpXrXk8ptXvjkZ6sdaqhXhjFJfUX9u4lKEHXjfPva7X6vrthIwRlTIo/lV LgF9nc3qCvlkDGMOIKt2aVpiGVbayjVwU704qFrRjpUxCyLET6KyiAq+8pdJ7BCaQjZj 0G9A== X-Gm-Message-State: AMCzsaUBYXaRXKFUl9aFsRh0OQzWayyj9mR3oMzrJ4ttCYy380tzO/lJ N2LNKmJfQTVoQ5SlIos+5DUzESOV X-Received: by 10.80.203.66 with SMTP id h2mr5406195edi.54.1507973015906; Sat, 14 Oct 2017 02:23:35 -0700 (PDT) Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com. [74.125.82.42]) by smtp.gmail.com with ESMTPSA id j6sm1922250edj.58.2017.10.14.02.23.35 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 14 Oct 2017 02:23:35 -0700 (PDT) Received: by mail-wm0-f42.google.com with SMTP id q124so25248301wmb.0; Sat, 14 Oct 2017 02:23:35 -0700 (PDT) X-Google-Smtp-Source: ABhQp+QbD7o2Q2XMN5kAqA9L7FxLsl6efFJ833w8gUKOcKtmCv3usQS5K6F/5H339isEH4mH3Wg/vG7cAIfmlYvpQVg= X-Received: by 10.28.132.69 with SMTP id g66mr3726166wmd.13.1507973014970; Sat, 14 Oct 2017 02:23:34 -0700 (PDT) MIME-Version: 1.0 Received: by 10.223.143.20 with HTTP; Sat, 14 Oct 2017 02:23:34 -0700 (PDT) In-Reply-To: References: <201710132031.v9DKVueS089009@repo.freebsd.org> <1507941050.8386.88.camel@freebsd.org> From: Svatopluk Kraus Date: Sat, 14 Oct 2017 11:23:34 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: svn commit: r324609 - head/sys/sys To: Mateusz Guzik Cc: Ian Lepore , Mateusz Guzik , "src-committers@freebsd.org" , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" Content-Type: text/plain; charset="UTF-8" X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 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: Sat, 14 Oct 2017 09:47:38 -0000 On Sat, Oct 14, 2017 at 2:48 AM, Mateusz Guzik wrote: > On Sat, Oct 14, 2017 at 2:30 AM, Ian Lepore wrote: >> >> On Fri, 2017-10-13 at 23:38 +0200, Svatopluk Kraus wrote: >> > MTX_UNOWNED is a flag. You did not change its value from 4 to 0, you >> > removed it actually. I have very bad feeling about it. But maybe, it's >> > really possible and in that case, a very good explanation should be >> > provided. >> > >> > Svata >> > >> >> The part that scares me is that DESTROYED may have been defined as >> CONTESTED|UNOWNED for subtle and clever reasons lost in the mists of >> time. Any of the places that are testing the MTX_CONTESTED bit may >> have been relying somehow on the fact that a destroyed mutex has that >> bit set. >> >> Then again, maybe Mateusz has carefully analyzed all this stuff, and we >> should just relax. :) >> > > I highly doubt there was anything clever going on. Rather, someone wanted > to somehow denote a destroyed mutex, but did not want to introduce an > additional flag - they are not free since the field shares the value with > the > address of the owner. That is, adding flags requires increasing alignment > of struct thread and that adds to memory usage. Thus they combined the > flags in a way which can never happen under normal circumstances. > > mtx_destroy explicitly sets the value to MTX_DESTROYED, so there is no > change here. > > MTX_UNOWNED is *not* used as a flag. If you grep the tree you will see > it is only used in direct comparisons. > > That said, I reviewed all users again and a minor bug was introduced in > owner_mtx (only used by dtrace). For some reason it grabs the owner, > but the instead of checking if it grabbed anything it checks the flag > (fixed in r324613). So that's my bad. > > Justification for the change was provided in the commit message: it makes > .text smaller on amd64 and probably all architectures. > > -- > Mateusz Guzik I did not lack an explanation why you did it, but why it's possible and safe. Something like this: ----------------- (1) MTX_UNOWNED even if defined like a flag was never used like a flag. It was used like a value set for unowned mutex and test accordingly. There is no logical operation (AND, OR, ...) done with MTX_UNOWNED in code. There are only assignments and checks for equality there. Thus, we can change its value and do not pretend that it's a flag anymore. As mutex owner thread pointer and mutex flags are kept in one variable together, setting MTX_UNOWNED value to 0 is more appropriate and brings some benefits. However, MTX_DESTROYED flag must be distinguish from MTX_CONTESTED one now, so there is no savings considering mutex flags. (2) MTX_UNONWED value is used only internally within mutex implemention, thus this change should be quite safe. ----------------- However, I suggest to do something with MTX_UNOWNED leak in sys/dev/syscons/syscons.c. i.e. to make mtx_unowned() public and use it instead. Further, MTX_DESTROYED is not used like a flag too. So, was its redefinition really needed? In other words, isn't MTX_CONTESTED flag without owner thread pointer set enough? Also, I would prefer to have some explanation to be in mutex.h about all of this. At least to not mess flags with values in definitions without explanation. Svata