From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751371AbdGQP3q (ORCPT ); Mon, 17 Jul 2017 11:29:46 -0400 Received: from mx2.suse.de ([195.135.220.15]:46202 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751280AbdGQP3o (ORCPT ); Mon, 17 Jul 2017 11:29:44 -0400 Date: Mon, 17 Jul 2017 17:29:41 +0200 (CEST) From: Miroslav Benes To: Joe Lawrence cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh Poimboeuf , Jessica Yu , Jiri Kosina , Petr Mladek Subject: Re: [PATCH v2 1/2] livepatch: introduce shadow variable API In-Reply-To: <1498664247-12296-2-git-send-email-joe.lawrence@redhat.com> Message-ID: References: <1498664247-12296-1-git-send-email-joe.lawrence@redhat.com> <1498664247-12296-2-git-send-email-joe.lawrence@redhat.com> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 28 Jun 2017, Joe Lawrence wrote: > +Brief API summary > +----------------- > + > +See the full API usage docbook notes in the livepatch/shadow.c > +implementation. > + > +An in-kernel hashtable references all of the shadow variables. These > +references are stored/retrieved through a key pair. > + > +* The klp_shadow variable data structure encapsulates both tracking > +meta-data and shadow-data: > + - meta-data > + - obj - pointer to original data > + - num - numerical description of new data > + - new_data[] - storage for shadow data > + > +* klp_shadow_attach() - allocate and add a new shadow variable: > + - allocate a new shadow variable > + - push a key pair into hashtable > + > +* klp_shadow_get() - retrieve a shadow variable new_data pointer > + - search hashtable for key pair > + > +* klp_shadow_get_or_attach() - get existing or attach a new shadow variable > + - search hashtable for key pair > + - if not found, call klp_shadow_attach() > + > +* klp_shadow_detach() - detach and free a shadow variable > + - find and remove any references from hashtable > + - if found, release shadow variable > + > +* klp_shadow_detach() - detach and free all <*, num> shadow variables > + - find and remove any <*, num> references from hashtable > + - if found, release shadow variable I think that the second one should be klp_shadow_detach_all(), shouldn't it? [...] > +static DEFINE_HASHTABLE(klp_shadow_hash, 12); Is there a reason, why you pick 12? I'm just curious. > +static DEFINE_SPINLOCK(klp_shadow_lock); > + > +/** > + * struct klp_shadow - shadow variable structure > + * @node: klp_shadow_hash hash table node > + * @rcu_head: RCU is used to safely free this structure > + * @obj: pointer to original data > + * @num: numerical description of new data Josh proposed better description. Could we also have a note somewhere in the documentation what this member is practically for? I mean versioning and ability to attach new members to a data structure if live patches are stacked. > + * @new_data: new data area > + */ > +struct klp_shadow { > + struct hlist_node node; > + struct rcu_head rcu_head; > + void *obj; > + unsigned long num; > + char new_data[]; > +}; What is the reason to change 'void *new_data' to 'char new_data[]'? I assume it is related to API changes below... [...] > +/** > + * _klp_shadow_attach() - allocate and add a new shadow variable > + * @obj: pointer to original data > + * @num: numerical description of new data > + * @new_data: pointer to new data > + * @new_size: size of new data > + * @gfp_flags: GFP mask for allocation > + * @lock: take klp_shadow_lock during klp_shadow_hash operations I am not sure about lock argument. Do we need it? Common practice is to have function foo() which takes a lock, and function __foo() which does not. In klp_shadow_get_or_attach(), you use it as I'd expect. You take the spinlock, call this function and release the spinlock. Is it possible to do the same in klp_shadow_attach() and have __klp_shadow_attach() without lock argument? > + * > + * Note: allocates @new_size space for shadow variable data and copies > + * @new_size bytes from @new_data into the shadow varaible's own @new_data > + * space. If @new_data is NULL, @new_size is still allocated, but no > + * copy is performed. I must say I'm not entirely happy with this. I don't know if this is what Petr had in mind (I'm sure he'll get to the patch set soon). Calling memcpy instead of a simple assignment in v1 seems worse. I'll take another look tomorrow and will think about it. Miroslav > + * Return: the shadow variable new_data element, NULL on failure. > + */ > +static void *_klp_shadow_attach(void *obj, unsigned long num, void *new_data, > + size_t new_size, gfp_t gfp_flags, > + bool lock) > +{ > + struct klp_shadow *shadow; > + unsigned long flags; > + > + shadow = kzalloc(new_size + sizeof(*shadow), gfp_flags); > + if (!shadow) > + return NULL; > + > + shadow->obj = obj; > + shadow->num = num; > + if (new_data) > + memcpy(shadow->new_data, new_data, new_size); > + > + if (lock) > + spin_lock_irqsave(&klp_shadow_lock, flags); > + hash_add_rcu(klp_shadow_hash, &shadow->node, (unsigned long)obj); > + if (lock) > + spin_unlock_irqrestore(&klp_shadow_lock, flags); > + > + return shadow->new_data; > +} > + > +/** > + * klp_shadow_attach() - allocate and add a new shadow variable > + * @obj: pointer to original data > + * @num: numerical description of new num > + * @new_data: pointer to new data > + * @new_size: size of new data > + * @gfp_flags: GFP mask for allocation > + * > + * Return: the shadow variable new_data element, NULL on failure. > + */ > +void *klp_shadow_attach(void *obj, unsigned long num, void *new_data, > + size_t new_size, gfp_t gfp_flags) > +{ > + return _klp_shadow_attach(obj, num, new_data, new_size, > + gfp_flags, true); > +} > +EXPORT_SYMBOL_GPL(klp_shadow_attach); > + > +/** > + * klp_shadow_get_or_attach() - get existing or attach a new shadow variable > + * @obj: pointer to original data > + * @num: numerical description of new data > + * @new_data: pointer to new data > + * @new_size: size of new data > + * @gfp_flags: GFP mask used to allocate shadow variable metadata > + * > + * Note: if memory allocation is necessary, it will do so under a spinlock, > + * so @gfp_flags should include GFP_NOWAIT, or GFP_ATOMIC, etc. > + * > + * Return: the shadow variable new_data element, NULL on failure. > + */ > +void *klp_shadow_get_or_attach(void *obj, unsigned long num, void *new_data, > + size_t new_size, gfp_t gfp_flags) > +{ > + void *nd; > + unsigned long flags; > + > + nd = klp_shadow_get(obj, num); > + > + if (!nd) { > + spin_lock_irqsave(&klp_shadow_lock, flags); > + nd = klp_shadow_get(obj, num); > + if (!nd) > + nd = _klp_shadow_attach(obj, num, new_data, new_size, > + gfp_flags, false); > + spin_unlock_irqrestore(&klp_shadow_lock, flags); > + } > + > + return nd; > + > +} > +EXPORT_SYMBOL_GPL(klp_shadow_get_or_attach);