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@xxxxxxxxxx> > GPG KeyID: 0E572FDD > >
Attachment:
signature.asc
Description: Digital signature
