From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932466AbdGXPEc (ORCPT ); Mon, 24 Jul 2017 11:04:32 -0400 Received: from mx1.redhat.com ([209.132.183.28]:40146 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932406AbdGXPEK (ORCPT ); Mon, 24 Jul 2017 11:04:10 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A6B41285B5 Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx06.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=jpoimboe@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com A6B41285B5 Date: Mon, 24 Jul 2017 10:04:07 -0500 From: Josh Poimboeuf To: Joe Lawrence Cc: Petr Mladek , live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Jessica Yu , Jiri Kosina , Miroslav Benes Subject: Re: [PATCH v2 1/2] livepatch: introduce shadow variable API Message-ID: <20170724150407.jyb33ux2f5iyetvq@treble> References: <1498664247-12296-1-git-send-email-joe.lawrence@redhat.com> <1498664247-12296-2-git-send-email-joe.lawrence@redhat.com> <20170718124500.GF3393@pathway.suse.cz> <79756d67-0f6e-4d7d-1827-f1e275e65f27@redhat.com> <20170721091350.GB26370@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.6.0.1 (2016-04-01) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Mon, 24 Jul 2017 15:04:10 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Jul 21, 2017 at 09:55:59AM -0400, Joe Lawrence wrote: > >>> I would do WARN() in klp_shadow_attach() when the variable > >>> already existed are return NULL. Of course it might be inoncent > >>> duplication. But it might mean that someone else is using another > >>> variable of the same name but with different content. klp_shadow_get() > >>> would then return the same variable for two different purposes. > >>> Then the whole system might end like a glass on a stony floor. > >> > >> What do you think of expanding the API to include each the cases > >> outlined above? Something like: > >> > >> 1 - klp_attach = allocate and add a unique to the hash, > >> duplicates return NULL and a WARN > > > > Sounds good. > > > >> 2 - klp_get_or_attach = return if it already exists, > >> otherwise allocate a new one > > > > Sounds good. > > > >> 3 - klp_get_or_update = update and return if it already > >> exists, otherwise allocate a new one > > > > I am not sure where this behavior would make sense. See below. > > > > > >> IMHO, I think cases 1 and 3 are most intuitive, so maybe case 2 should > >> be dropped. Since you suggested adding klp_get_or_attach(), what do you > >> think? > > > > I do not agree. Let's look at the example with the missing lock. > > The patch adds the lock if it did not exist. Then the lock can > > be used to synchronize all further operations. > > > > klp_get_or_update() would always replace the existing lock > > with a freshly initialized one. We would loss the information > > if it was locked or not. > > Ah good point, perhaps we have two situations here: > > A - A shadow variable that's pointing to some object, like a lock, > where the original object is required. (Your example above.) > > B - A shadow variable that's storing the data itself. In other words, > instead of attaching a pointer, the whole object was attached: > > void patched_function() > { > ... > klp_get_or_attach(obj, id, &jiffies, sizeof(jiffies), ...) > ... > > in which case the caller is only interested in pushing in the > latest version of jiffies. > > For these I suggest klp_get_or_attach() for case A and > klp_get_or_update() for case B. klp_get_or_update() doesn't actually 'get', because even if it does, it gets updated first. I think a more precise name would be klp_update_or_attach(). -- Josh