Re: [WTF] utterly tasteless ABI in hfi1 (around ->write()/->write_iter())

[Doug Ledford]

On 03/16/2016 12:17 AM, Al Viro wrote:
> 	Folks, we'd discussed that kind of crap already; why, in name of
> everything unholy, is that kind of garbage brought back in a new driver?

No doubt because in large part the hfi1 driver copy and pastes
significant portions of the qib driver.  Likewise, libpsm2 (which works
with the hfi1 devices) is largely a copy and paste job from open-psm
(which works with qib/ipath devices).

> 	Having both ->write() and ->write_iter() *AND* having entirely
> unrelated interpretation of user input on those two on the same device
> is bogus, with the capital Stop That Shit Right Now.
> 
> 	As it is, write(fd, p, len) and writev(fd, &(struct iovec){p,len}, 1)
> are interpreted in absolutely unrelated ways.  As in, entirely different
> set of commands.  Moreover, aio IOCB_CMD_PWRITE matches writev(), not write().
> 
> 	What's going on?  Sure, ipathfs is a precious snowflake with the same
> kind of braindamage.  Back when it had been brought to your attention
> (along with the fact that this piece of idiocy happens to be a file on
> filesystem of your own, under your full control and no need whatsofuckingever
> to multiplex completely unrelated sets of commands on the same file with
> "was it write(2) or writev(2)?" used as a selector) the answer had been
> basically "it doesn't have to make sense, it's Special".

I can't speak for Mike, but I never said "It's Special".  I said it's a
driver internal thing with only one consumer and the kernel driver and
the user space consumer are a matched pair.  If this were a general API
for use by any old program I would agree with you, but since it isn't, I
wasn't that concerned about whether it got fixed.  If it broke, Intel
had both pieces and could fix it.  And with that in mind I said "ince
this is an internal driver interface that only Intel uses, I'm not
inclined to force them to rewrite their driver and their library just
because their particular usage took you off guard."

I should point out that the fragility is not so rampant as you make it
sound.  The qib driver was added in 2010 and had this interface then
(modulo your changes in 4961772560d2).  It was a copy from the ipath
driver at the time, so in truth had been around much longer even than 2010.

>  Now it turns out
> that it has grown an equally special sibling.  With the idiocy directly
> exposed as userland ABI.  Could we please fix that thing before it's cast in
> stone?

If we want to maintain back compatibility, then the qib driver has to
maintain this interface. We could possibly do a new one as well, but we
can't remove this one.

For the hfi1 driver (and OPA in general), we do have the ability to do a
new API.  But, going back to what I said before, I just don't care that
much.  It's Intel internal stuff as far as I'm concerned.  If they do
something fragile and it breaks, then that's all on their hands.
They've gotten away with it for over 10 years so I can see why they
probably aren't that concerned about it themselves.

> 	Take a look at drivers/staging/rdma/hfi1/file_ops.c in -next and
> compare hfi1_write_iter() with hfi1_file_write().  Folks, this ABI is too
> ugly to live, let alone to be allowed breeding.
> 
> 	It's also brittle as hell - trivial massage around fs/read_write.c
> and fs/aio.c is quite capable of breaking that shit.  Arguably, IOCB_CMD_PWRITE
> and IOCB_CMD_PWRITEV both triggering your writev() semantics is an example of
> just such breakage.  Sigh...

I'm sure you could break it intentionally if you wanted to.  Not that I
think you *should*, but I'm sure you *could*.

Intel: For your own sake's you might want to consider doing something
simple such as using two files, one for regular commands and one for
SDMA commands, and modifying both the hfi1 and libpsm2 code bases.  I
don't care, and I wouldn't force you to do it, but I've made my argument
to Al and he appears to be running it up the tree, so it might be
easiest to capitulate.

-- 
Doug Ledford <dledford@xxxxxxxxxx>
              GPG KeyID: 0E572FDD

Attachment: signature.asc
Description: OpenPGP digital signature