Re: [PATCH v2] swait: export symbols __prepare_to_swait and __finish_swait

From: Mike Snitzer <snitzer@redhat.com>
To: Mikulas Patocka <mpatocka@redhat.com>
Cc: Peter Zijlstra <peterz@infradead.org>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	wagi@monom.org, Christoph Hellwig <hch@infradead.org>,
	dm-devel@redhat.com, Dan Williams <dan.j.williams@intel.com>,
	tglx@linutronix.de
Subject: Re: [PATCH v2] swait: export symbols __prepare_to_swait and __finish_swait
Date: Wed, 23 May 2018 17:51:40 -0400	[thread overview]
Message-ID: <20180523215140.GA32285@redhat.com> (raw)
In-Reply-To: <alpine.LRH.2.02.1805231633060.17160@file01.intranet.prod.int.rdu2.redhat.com>

On Wed, May 23 2018 at  4:38pm -0400,
Mikulas Patocka <mpatocka@redhat.com> wrote:

> 
> 
> On Wed, 23 May 2018, Mike Snitzer wrote:
> 
> > [Peter, in this v2 I switched to using _GPL for the exports and updated
> > the patch header.  As covered in previous mail, please let me know if
> > you're OK with me staging this change for 4.18 via linux-dm.git with
> > your Ack, thanks]
> > 
> > From: Mikulas Patocka <mpatocka@redhat.com>
> > Subject: [PATCH] swait: export symbols __prepare_to_swait and __finish_swait
> > 
> > __prepare_to_swait and __finish_swait are declared in
> > include/linux/swait.h but they are not exported; so they are not useable
> > from kernel modules.
> > 
> > A new consumer of swait (in dm-writecache) reduces its locking overhead
> > by using the spinlock in swait_queue_head to protect not only the wait
> > queue, but also the list of events.  Consequently, this swait consuming
> > kernel module needs to use these unlocked functions.
> > 
> > Peter Zijlstra explained:
> >   "The reason swait exists is to be deterministic (for RT) -- something
> >   that regular wait code cannot be.
> >   And by (ab)using / exporting the wait internal lock you risk losing
> >   that. So I don't think the proposed [dm-writecache] usage is bad, it
> >   is possible to create badness.
> >   So if we're going to export them; someone needs to keep an eye on things
> >   and ensure the lock isn't abused."
> > 
> > So while this new use of the wait internal lock doesn't jeopardize the
> > realtime requirements of swait, these exports do open swait's internal
> > locking up to being abused.  As such, EXPORT_SYMBOL_GPL is used because
> > any future consumers of __prepare_to_swait and __finish_swait must
> > always be thoroughly scrutinized.
> > 
> > Cc: Peter Zijlstra <peterz@infradead.org>
> > Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
> > Signed-off-by: Mike Snitzer <snitzer@redhat.com>
> > ---
> >  kernel/sched/swait.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/kernel/sched/swait.c b/kernel/sched/swait.c
> > index b6fb2c3b3ff7..5d891b65ada5 100644
> > --- a/kernel/sched/swait.c
> > +++ b/kernel/sched/swait.c
> > @@ -75,6 +75,7 @@ void __prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait)
> >  	if (list_empty(&wait->task_list))
> >  		list_add(&wait->task_list, &q->task_list);
> >  }
> > +EXPORT_SYMBOL_GPL(__prepare_to_swait);
> >  
> >  void prepare_to_swait(struct swait_queue_head *q, struct swait_queue *wait, int state)
> >  {
> > @@ -104,6 +105,7 @@ void __finish_swait(struct swait_queue_head *q, struct swait_queue *wait)
> >  	if (!list_empty(&wait->task_list))
> >  		list_del_init(&wait->task_list);
> >  }
> > +EXPORT_SYMBOL_GPL(__finish_swait);
> >  
> >  void finish_swait(struct swait_queue_head *q, struct swait_queue *wait)
> >  {
> > -- 
> > 2.15.0
> 
> Then - you should export swake_up_locked with EXPORT_SYMBOL_GPL too.
> 
> Because swake_up_locked is unusable without __prepare_to_swait and 
> __finish_swait.

Point taken.  But if swake_up_locked is unusable without them it is
implicitly _GPL once __prepare_to_swait and __finish_swait are exported
via _GPL.  Which is to say, I don't care to get into the games of
switching symbols from EXPORT_SYMBOL to EXPORT_SYMBOL_GPL unless
completely necessary.  Happy to leave well enough alone on this.

So I consider this v2 perfectly adequate for our needs.  And appreciate
any additional review/acks.

Thanks,
Mike