Re: [PATCH 3/9] pci/switchtec: Don't abuse completion wait queue for poll

From: Logan Gunthorpe <logang@deltatee.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	linux-kernel@vger.kernel.org
Cc: Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@kernel.org>, Will Deacon <will@kernel.org>,
	"Paul E . McKenney" <paulmck@kernel.org>,
	Joel Fernandes <joel@joelfernandes.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Kurt Schwemmer <kurt.schwemmer@microsemi.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	linux-pci@vger.kernel.org
Subject: Re: [PATCH 3/9] pci/switchtec: Don't abuse completion wait queue for poll
Date: Fri, 13 Mar 2020 12:11:38 -0600	[thread overview]
Message-ID: <4d3a997d-ced4-3dbe-d766-0b1e9fc35b29@deltatee.com> (raw)
In-Reply-To: <20200313174701.148376-4-bigeasy@linutronix.de>

On 2020-03-13 11:46 a.m., Sebastian Andrzej Siewior wrote:
> The poll callback is abusing the completion wait queue and sticks it into
> poll_wait() to wake up pollers after a command has completed.
> 
> First of all it's a layering violation as it imposes restrictions on the
> inner workings of completions. Just because C allows to do so does not
> justify that in any way. The proper way to do such things is to post
> patches which extend the core infrastructure and not by silently abusing
> it.

As I've said previously, I disagree with this approach. Open coding
standard primitives sweeps issues under the rug and is a step backwards
for code quality. Calling it a layering violation is just one opinion
and if it is, the better solution would be to create an interface you
find appropriate so that it isn't one.

> Aside of that the implementation is seriously broken:
> 
>  1) It cannot work with EPOLLEXCLUSIVE

Why? You don't explain this. And I don't see how this patch would change
anything to do with the call to poll_wait(). All you've done is
open-code the completion.

Not that it matters that much, having multiple waiters poll on this
interface can pretty much never happen. It only makes sense for the
process who submitted the write to poll on the interface.

>  2) It's racy:
> 
>   poll()	      	  	 write()
>    switchtec_dev_poll()		   switchtec_dev_write()
>     poll_wait(&s->comp.wait);        mrpc_queue_cmd()
>     				       init_completion(&s->comp)
> 					 init_waitqueue_head(&s->comp.wait)

That's a nice catch! But wouldn't an easier solution be to change the
code to use reinit_completion() instead of using the bug to justify a
different change?

Thanks,

Logan