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

From: Logan Gunthorpe <logang@deltatee.com>
To: Thomas Gleixner <tglx@linutronix.de>,
	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>,
	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: Sat, 14 Mar 2020 00:01:07 -0600	[thread overview]
Message-ID: <9213d617-207e-da4e-094a-45ae587fdc98@deltatee.com> (raw)
In-Reply-To: <87sgibeqcs.fsf@nanos.tec.linutronix.de>

On 2020-03-13 6:23 p.m., Thomas Gleixner wrote:
> Logan Gunthorpe <logang@deltatee.com> writes:
>> 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.
> 
> Feel free to do s.
> 
>> 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.
> 
> There is no standard primitive which allows to poll on a completion.
> 
> You decided that this is smart to do and just because C does not
> allow to hide implementation details this is not a justification for
> this at all.
> 
> Due to the limitations of C, the kernel has to rely on the assumption
> that developers know and respect the difference between API and
> implementation.
> 
> Relying on implementation details of an interface and then arguing that
> this is a standard primitive for the chosen purpose is just backwards.
> 
> What's even more hillarious is that you now request that we give you a
> replacement interface for something which was not an interface to use in
> the first place.

I'm in awe at the lack of professionalism in your emails. If you
bothered to edit out the ad hominems, you might have noticed that nobody
has yet described how the poll interface fails here (with
EPOLLEXCLUSIVE) or how replacing one wait queue for another fixes the
purported problem.

We clearly disagree on what's considered appropriate usage of the
completion helper (calling it a "locking primitive" is a bit
disingenuous) and it doesn't sound like that's going to change. I hold
no power here, but you aren't going to bully me into giving this patch
an Ack or into silencing my opinion on the matter.

I'd prefer it if you submit a patch that's honest about what it's trying
to accomplish and why (ie. it doesn't masquerade as being necessary to
fix a bug). I also ask that you accept that I'm within my right to voice
my dissent. If, after that, Bjorn chooses to take your patch, then I
will respect his decision. I trust that he's able to read behind the
personal attacks and look only at the technical issues.

Logan