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: 3380 bytes --]

On 07/30/2015 06:00 PM, Marciniszyn, Mike wrote:
>> On 07/30/2015 04:01 PM, Marciniszyn, Mike wrote:
>>>>
>>>> I thought you were getting rid of this?
>>>>
>>>> Jason
>>>
>>> Doug wanted the v4 submitted as we currently have it.
>>
>> To be accurate, I said "If you want a chance at making 4.3, I need a v4".  I
>> didn't comment on whether or not any specific review comments were
>> addressed.
>>
>>> Doug?
>>
>> 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?  It's not a normal file in *any*
>> sense of the word.  For example, the normal write routine will never, ever
>> accept just plain data.  It's always in the form of a command.  If you don't
>> have the right magic decoder ring, you will get nothing but errors when
>> trying to do something with this file.
>>  Much like /dev/infiniband/uverbs? files, it is a command interface, not a
>> raw data interface.  I actually think the fact that you guys use write for a
>> single command and writev/write_iter for a command queue is an elegant
>> solution to your particular needs.  The only reason Al threw a hissy over it is
>> because it tripped him up when he went to do the conversion from writev to
>> write_iter.  That's understandable.  So, some clear documentation so
>> someone like Al doesn't have to go reading through sources and try to figure
>> out what you are doing would be the generally nice thing to do for other
>> kernel generalists that might come poking around this way.  Or, 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.  Regardless, those things can be
>> cleaned up in follow on patches, please do not resubmit this set for that.
>>
> 
> Jason,
> 
> I did ask you in http://marc.info/?l=linux-rdma&m=143707462806767&w=2 if you thought ioctl was ok.
> 
> Hearing nothing, we left the interface as it was.

I think the interface is fine as is, with the only thing I would do, if
*really* forced to by Al, would be to do as I suggested above and
convert all of the write cases to writev with a single element.

> I suspect (I lack the early history) that the ioctl BKL might have forced both uverbs and PSM to go this route.

An ioctl interface is not really designed for a queue of commands to be
sent in a single operation any more than any other interface is.  I
don't personally see a great benefit to it.

> 
> Doug,
> 
> Where would be the appropriate location to document?  In the source itself?  Somewhere else?

At the function for both write and writev (and a patch to update to
write_iter would be a good next step to keep us in line with qib),
document how each is used and point to the other one and point out that
they differ in their basic usage.  If Al had a clear comment saying "our
write function is used to pass a single command to our driver, and our
writev function is used to pass a queue of formatted commands, one per
element", he might have not written what he did in his commit message.


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



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