From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751921AbdGRTPE (ORCPT ); Tue, 18 Jul 2017 15:15:04 -0400 Received: from mx1.redhat.com ([209.132.183.28]:35584 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751817AbdGRTPC (ORCPT ); Tue, 18 Jul 2017 15:15:02 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 97C397EBD3 Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx03.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=joe.lawrence@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 97C397EBD3 Date: Tue, 18 Jul 2017 15:15:00 -0400 From: Joe Lawrence To: Petr Mladek Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh Poimboeuf , Jessica Yu , Jiri Kosina , Miroslav Benes Subject: Re: [PATCH v2 2/2] livepatch: add shadow variable sample programs Message-ID: <20170718191500.vjdjze6fpfz4us3s@redhat.com> References: <1498664247-12296-1-git-send-email-joe.lawrence@redhat.com> <1498664247-12296-3-git-send-email-joe.lawrence@redhat.com> <20170718144745.GG32632@pathway.suse.cz> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170718144745.GG32632@pathway.suse.cz> User-Agent: Mutt/1.6.2-neo (2016-08-08) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Tue, 18 Jul 2017 19:15:02 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Jul 18, 2017 at 04:47:45PM +0200, Petr Mladek wrote: > Date: Tue, 18 Jul 2017 16:47:45 +0200 > From: Petr Mladek > To: Joe Lawrence > Cc: live-patching@vger.kernel.org, linux-kernel@vger.kernel.org, Josh > Poimboeuf , Jessica Yu , Jiri Kosina > , Miroslav Benes > Subject: Re: [PATCH v2 2/2] livepatch: add shadow variable sample programs > User-Agent: Mutt/1.5.21 (2010-09-15) > > On Wed 2017-06-28 11:37:27, Joe Lawrence wrote: > > diff --git a/samples/livepatch/livepatch-shadow-mod.c b/samples/livepatch/livepatch-shadow-mod.c > > new file mode 100644 > > index 000000000000..423f4b7b0adb > > --- /dev/null > > +++ b/samples/livepatch/livepatch-shadow-mod.c > > + * Usage > > + * ----- > > + * > > + * Fix the memory leak > > + * ------------------- > > + * > > + * Extend functionality > > + * -------------------- > > When I made first quick look, I had troubles to understand that > these two sections were still part of the Usage section. I could double underline "Usage" and keep the single underlines for "Fix the memory leak' and "Extend functionality" sections if that helps. > Also > the commands how to enable the modules were hidden deep in the > expected output analyze. Yeah, v2's examples were hard to summarize and describe through comments. The code is pretty simple, but the devil is in the timing details and log output seemed the best way to illustrate the fixes. Do you think the log entries are worth copying into these comments? I could simply describe the effect and let the reader generate their own if they so desired. > I wonder if it would make sense to move most of the information > into the respective module sources. Are you suggesting that it would be clearer to distribute the log + comments to each individual livepatch-shadow-*.c module file? > I actually missed some > more info in that modules. The few lines were hard to spot > after the long copyright/license blob. Would a heading like this help separate the legalese from the patch description? /* [ ... GPL, copyrights, etc ... ] * * Module Description * ================== * [...] */ > > + > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_AUTHOR("Joe Lawrence "); > > +MODULE_DESCRIPTION("Buggy module for shadow variable demo"); > > + > > +#define T1_PERIOD 1 /* allocator thread */ > > +#define T2_PERIOD (3 * T1_PERIOD) /* cleanup thread */ > > + > > +LIST_HEAD(dummy_list); > > +DEFINE_MUTEX(dummy_list_mutex); > > + > > +struct dummy { > > + struct list_head list; > > + unsigned long jiffies_expire; > > +}; > > + > > +noinline struct dummy *dummy_alloc(void) > > +{ > > + struct dummy *d; > > + void *leak; > > + > > + d = kzalloc(sizeof(*d), GFP_KERNEL); > > + if (!d) > > + return NULL; > > + > > + /* Dummies live long enough to see a few t2 instances */ > > + d->jiffies_expire = jiffies + 1000 * 4 * T2_PERIOD; > > + > > + /* Oops, forgot to save leak! */ > > + leak = kzalloc(sizeof(int), GFP_KERNEL); > > + > > + pr_info("%s: dummy @ %p, expires @ %lx\n", > > + __func__, d, d->jiffies_expire); > > + > > + return d; > > +} > > + > > +noinline void dummy_free(struct dummy *d) > > +{ > > + pr_info("%s: dummy @ %p, expired = %lx\n", > > + __func__, d, d->jiffies_expire); > > + > > + kfree(d); > > +} > > + > > +noinline bool dummy_check(struct dummy *d, unsigned long jiffies) > > +{ > > + return time_after(jiffies, d->jiffies_expire); > > +} > > + > > +/* > > + * T1: alloc_thread allocates new dummy structures, allocates additional > > + * memory, aptly named "leak", but doesn't keep permanent record of it. > > + */ > > +struct workqueue_struct *alloc_wq; > > A custom workqueue is needed only for special purposes. > IMHO, the standard system workqueue is perfectly fine > in our case. AFAIK, it is able to spawn/run about > 256 worker threads and process this number of different > works in parallel. No problem. This example executes infrequently and obviously isn't high priority or anything. > > +struct delayed_work alloc_dwork; > > +static void alloc_thread(struct work_struct *work) > > The suffix "_thread" is misleading. I think that alloc_work_func() > is the most descriptive name. > Noted for v3. > > +{ > > + struct dummy *d; > > + > > + d = dummy_alloc(); > > + if (!d) > > + return; > > + > > + mutex_lock(&dummy_list_mutex); > > + list_add(&d->list, &dummy_list); > > + mutex_unlock(&dummy_list_mutex); > > + > > + queue_delayed_work(alloc_wq, &alloc_dwork, > > + msecs_to_jiffies(1000 * T1_PERIOD)); > > +} > > + > > +static int livepatch_shadow_mod_init(void) > > +{ > > + alloc_wq = create_singlethread_workqueue("klp_demo_alloc_wq"); > > + if (!alloc_wq) > > + return -1; > > + > > + cleanup_wq = create_singlethread_workqueue("klp_demo_cleanup_wq"); > > + if (!cleanup_wq) > > + goto exit_free_alloc; > > + > > + INIT_DELAYED_WORK(&alloc_dwork, alloc_thread); > > + queue_delayed_work(alloc_wq, &alloc_dwork, 1000 * T1_PERIOD); > > + > > + INIT_DELAYED_WORK(&cleanup_dwork, cleanup_thread); > > + queue_delayed_work(cleanup_wq, &cleanup_dwork, > > + msecs_to_jiffies(1000 * T2_PERIOD)); > > If you use the system workqueue and DECLARE_DELAYED_WORK, you might > reduce this to: > > schedule_delayed_work(&alloc_dwork, 1000 * T1_PERIOD); > schedule_delayed_work(&cleanup_dwork, msecs_to_jiffies(1000 * T2_PERIOD)); Good cleanup for v3. > > + return 0; > > + > > +exit_free_alloc: > > + destroy_workqueue(alloc_wq); > > + > > + return -1; > > +} > > + > > +static void livepatch_shadow_mod_exit(void) > > +{ > > + struct dummy *d, *tmp; > > + > > + /* Cleanup T1 */ > > + if (!cancel_delayed_work(&alloc_dwork)) > > + flush_workqueue(alloc_wq); > > You will get the same with > > cancel_delayed_work_sync(&alloc_dwork); > > I am sorry, I spent too much time working on kthread worker API > and was not able to help myself. Also Tejun would put a shame > on me if I did not suggest this ;-) No worries, thanks for the kthread worker tips, especially this last one. I'll incorporate them all in the next version. > Otherwise I like the examples. Funny that I had anticipated head-scratching over the convoluted example instead of my workqueue usage :) -- Joe