Re: [PATCH v4 17/50] IB/hfi1: add PSM driver control/data path

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3644 bytes --]

On 07/31/2015 04:22 AM, Al Viro wrote:
> On Fri, Jul 31, 2015 at 12:31:58AM -0700, Christoph Hellwig wrote:
>> On Thu, Jul 30, 2015 at 05:42:16PM -0400, Doug Ledford wrote:
>>> I have no problem with this code.  That Al finds the user space ABI for
>>> this driver to be bizarre is neither here nor there to me.  Sure, this
>>> file does not exhibit normal file API behavior.  Who cares?
>>
>> Everyone who cares about filesystem semantics does.
>>
>> A NACK from me for a this features, and b) for trying to sneak it in
>> after a negative comment without cc to -fsdevel and Al.
> 
> FWIW, the lack of comments appears to have tripped Doug, judging by his
> "another option would be to drop the write function altogether and just make
> all commands come through writev/write_iter and if you only have one command,
> you only send one element".
> 
> The thing is, ipath and qib (and AFAICS this one as well) have write(2) and
> writev(2) take different and completely unrelated sets of commands.  On
> the same file.  IOW, the effects of
> 	writev(fd, &(struct iovec){buf, len}, 1)
> and
> 	write(fd, buf, len)
> are not even remotely similar.  _That_ is the gratuitous weirdness I'd been
> unhappy with.  And yes, it is gratuitous - it's trivial to have separate files
> for separate command sets.

Yes, admittedly I did think that the command sets were the same.
However, even with them not being the same, I still don't care.  This is
a private driver interface.  Nothing uses it but libpsm or libpsm2, both
of which are the mated user space libraries that implement the user side
of this interface.  It generally isn't intended for anyone else to use.
 If this were a public API, I would care.  It isn't.  All that *really*
matters here is that their kernel driver and user space library speak
the same language.

As for separate files, that presents its own issues, and for little to
no benefit.  Again, this isn't a public API.  It threw you for a loop
because it isn't what you expected.  But that hardly matters, you aren't
writing a user space application to work with the thing.  It is,
however, exactly what the existing library expects.

> If you drop ->write() in there, you certainly lose the weirdness - along with
> one of those command sets.  Sure, having individual iovecs correspond to
> separate datagrams is fine; tons of drivers are like that.  qib and ipath
> are unique, though, in having *two* command sets overloaded on the same file,
> with write() vs. writev() acting as selector (BTW, single-element AIO
> going like writev(), not like write()).

It sounds weird to someone who isn't used to it, but sounds perfectly
fine to someone used to it.  If this were a public API, I would care,
but this is Intel's private communication channel between their kernel
driver and their user space library.  I find it hard to justify telling
them they need to re-engineer both their kernel driver and their library
because a disinterested third party finds their particular means of
organizing their communications "weird".

> PS: I'm back after several weeks of being sick (recipe for fun: start with
> 40C in shade, get completely soaked in serious rain, then have half an hour
> cab ride, with AC set to ~15C) and while I'd managed to get the mailbox
> down to relatively sane size, I might have easily deleted more than I should
> have.  If there had been anything important sent my way and I don't reply to 
> it by Saturday, please resend.
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]