Re: [PATCH 06/10] IB/hfi1: Add ioctl() interface for user commands

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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

On Tue, May 24, 2016 at 03:17:00PM -0400, Doug Ledford wrote:
> On 05/24/2016 01:54 PM, Leon Romanovsky wrote:
> > On Tue, May 24, 2016 at 12:13:56PM -0400, Doug Ledford wrote:
> >> On 05/23/2016 10:10 AM, Dennis Dalessandro wrote:
> >>> On Mon, May 23, 2016 at 04:03:12PM +0300, Leon Romanovsky wrote:
> >>>> On Mon, May 23, 2016 at 08:22:08AM -0400, Dennis Dalessandro wrote:
> >>>>> On Sun, May 22, 2016 at 08:57:15PM +0300, Leon Romanovsky wrote:
> >>>>>> On Sun, May 22, 2016 at 10:03:52AM -0400, Dennis Dalessandro wrote:
> >>>>>>> On Sun, May 22, 2016 at 03:01:29PM +0300, Leon Romanovsky wrote:
> >>>>>>>>>> I think the overall consensus over participants in OFVWG call
> >>>>> was to use
> >>>>>>>>>> one IOCTL to enter into device specific handler which will do all
> >>>>>>>>>> necessary parsing and not spamming common IOCTL interface.
> >>>>>>>>>
> >>>>>>>>> That was for the verbs working group and the verbs 2.0 uAPI. This
> >>>>> is for
> >>>>>>>>> psm.
> >>>>>>>>
> >>>>>>>> I'm glad that you are supporting my point.
> >>>>>>>> It is vendor specific implementation for vendor specific driver
> >>>>> and not
> >>>>>>>> for whole IB core, so there is no need to pollute general IB ioctls.
> >>>>>>>
> >>>>>>> It is making use of and applying a proper classification.  Is there a
> >>>>>>> technical concern with this other than that's not how verbs may end
> >>>>> up doing
> >>>>>>> it?
> >>>>>>>
> >>>>>>> I'm not completely opposed to the single ioctl, I just don't
> >>>>> necessarily see
> >>>>>>> that as better in this case but am willing to listen to a technical
> >>>>>>> justification for why it's incorrect.
> >>>>>>
> >>>>>> it will simplify internal and external development by removing the
> >>>>>> tensions over ioctls numbers. Do you plan to take the block of ioctls
> >>>>>> for future expansion? Do you plan to mix hfi's ioctls with verbs's
> >>>>> ioctls
> >>>>>> based on acceptance of new code?
> >>>>>
> >>>>> I'm still not sure what you are getting at here. Can you explain what
> >>>>> you
> >>>>> mean by tensions over ioctl numbers?  I guess I don't understand why the
> >>>>> hfi1_x device's use of icotl numbers has any bearing at all on the
> >>>>> ibcore/verbs ioctl(s).
> >>>>>
> >>>>> If and when new code is accepted and hfi1 converges its API to go
> >>>>> through a
> >>>>> common character device, then hfi1 would surely change to match
> >>>>> whatever is
> >>>>> there whether that's a single ioctl with a command type embedded or
> >>>>> something that has not even yet been proposed.
> >>>>
> >>>> Denny,
> >>>>
> >>>> It is easy for everyone to converge hfi1 API from day one, so if and
> >>>> when new code is posted, the hfi1 changes will be summarized by one
> >>>> line change.
> >>>
> >>> Let's put the future API issue, and the specifics of this patch aside
> >>> for just a minute.  I'd like to understand the rationale for wanting a
> >>> single ioctl over specific ioctls in the general sense. I know that's
> >>> what folks seem to prefer from the calls, but perhaps we can get that
> >>> down in writing here on the list.
> >>>
> >>> I see an advantage for the specific ioctls because we can classify them
> >>> based on permission. When running things like strace you can decode the
> >>> ioctl number and see what access it is making. It also makes it easy to
> >>> have a gist of what is going on based on the ioctl call itself.
> >>
> >> Personally, if there is no shortage of ioctls (and there shouldn't be in
> >> this case because this is ioctls on the psm cdev, not on the uverbs
> >> device file), then the separate ioctls have their benefits as Dennis
> >> points out.  And seeing as how they (Intel) maintain the psm library
> >> that uses this interface, if they want their library using different
> >> ioctls and their driver using different ioctls versus one mega ioctl
> >> with embedded commands, I'm inclined to let them decide how they want it
> >> to be.
> > 
> > Except one thing that their device should integrate into already
> > available char device and don't create new one in IB space.
> 
> They have always had their own device.  Until the verbs 2.0 API is moved
> forward, I expect them to continue to do so.  That they used the
> InfiniBand ioctl number means we might need to make sure that the verbs
> 2.0 API ioctl numbers and the ones they used don't clash, but given that
> we have an assigned range of 256 ioctls and this patchset uses up only
> 13 (and 13 that could probably be shared with qib), I don't see this as
> a starvation of ioctl space issue.

Let's assume that you are planning to provide block of 20 ioctls per
device. Right now, there are 10 (or 8 if we count driver families) drivers in
drivers/infiniband/hw/ + 1 is coming (HSI) + uverbs (approximately 40)
=> 8 * 20 + 20 + 40 = 220 ioctls => we already in shortage without room
to expansion.

Why do we need to put ourselves in such situation?

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



[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]