Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1380056 > unrolled thread

Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Christoph Hellwig <hch@infradead.org> - 2016-04-15 19:40 +0200
    RE: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access "Hefty, Sean" <sean.hefty@intel.com> - 2016-04-15 19:50 +0200
    RE: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access "Woodruff, Robert J" <robert.j.woodruff@intel.com> - 2016-04-15 19:50 +0200
      Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-15 23:30 +0200
    Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-15 23:30 +0200
      Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Ira Weiny <ira.weiny@intel.com> - 2016-04-16 01:30 +0200
        Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-16 08:20 +0200
          Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-16 17:30 +0200
      Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-16 01:40 +0200
        Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-16 08:10 +0200
          Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Al Viro <viro@ZenIV.linux.org.uk> - 2016-04-16 21:20 +0200
            Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-18 14:10 +0200

#1380056 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Fri, Apr 15, 2016 at 08:30:35PM +0300, Leon Romanovsky wrote:
> Great, did you show it to other RDMA stakeholders except Intel?
> I saw nothing posted on ML or proposed for initial discussion, which
> will be held in the next week or two.

I fear it's kfabrics, which is an entirely crackpot idea and a total
non-starter, but for some reason Intel and their buddies keep wasting
time on it.

[toc] | [next] | [standalone]

#1380062

> > Great, did you show it to other RDMA stakeholders except Intel?
> > I saw nothing posted on ML or proposed for initial discussion, which
> > will be held in the next week or two.
> 
> I fear it's kfabrics, which is an entirely crackpot idea and a total
> non-starter, but for some reason Intel and their buddies keep wasting
> time on it.

There were discussions between several developers from multiple companies around moving away from using writev.  That's it.  Making random accusations and throwing crap over the wall does nothing to improve the community or instill trust.

[toc] | [prev] | [next] | [standalone]

#1380068

> I fear it's kfabrics, which is an entirely crackpot idea and a total non-starter, but for some reason Intel and their buddies keep wasting time on it.

What is being discussed her is not kfabrics. That is a totally different out of kernel pathfinding project at this point.
What is being discussed here is how to best solve the write/writev issue with the PSM interface. The code submitted was to move
to IOCTL instead, but people like Jason have suggested routing the IOCTLs through the verbs layer instead.

[toc] | [prev] | [next] | [standalone]

#1380281

[Multipart message — attachments visible in raw view] — view raw

On Fri, Apr 15, 2016 at 05:44:48PM +0000, Woodruff, Robert J wrote:
> > I fear it's kfabrics, which is an entirely crackpot idea and a total non-starter, but for some reason Intel and their buddies keep wasting time on it.
> 
> What is being discussed her is not kfabrics. That is a totally different out of kernel pathfinding project at this point.
> What is being discussed here is how to best solve the write/writev issue with the PSM interface. The code submitted was to move
> to IOCTL instead, but people like Jason have suggested routing the IOCTLs through the verbs layer instead.

The discussion here is much broader than conversion of PSM interface.

[toc] | [prev] | [next] | [standalone]

#1380284

[Multipart message — attachments visible in raw view] — view raw

On Fri, Apr 15, 2016 at 10:34:01AM -0700, Christoph Hellwig wrote:
> On Fri, Apr 15, 2016 at 08:30:35PM +0300, Leon Romanovsky wrote:
> > Great, did you show it to other RDMA stakeholders except Intel?
> > I saw nothing posted on ML or proposed for initial discussion, which
> > will be held in the next week or two.
> 
> I fear it's kfabrics, which is an entirely crackpot idea and a total
> non-starter, but for some reason Intel and their buddies keep wasting
> time on it.

It is a different thing, during OFA16 conference we were **strongly
advised** to move from old read/write interface in RDMA stack to
something else.

The agreement was that a couple of weeks after the conference,
Liran will organize open web meeting to discuss what we want from
this interface.

It is important to make it open, so all participants will be able to
express their willingness.

Intel as usual decided to do it in their way and the result is presented
on this mailing list.

Thanks.

[toc] | [prev] | [next] | [standalone]

#1380375

On Sat, Apr 16, 2016 at 12:23:28AM +0300, Leon Romanovsky wrote:

> 
> Intel as usual decided to do it in their way and the result is presented
> on this mailing list.

Excuse me, but this statement is completely unfair.  We were specifically asked
by Al and Linus to fix our char device with regards to the write/writev
inconsistency.

https://www.spinics.net/lists/linux-rdma/msg34451.html

Which is _exactly_ what this patch series does.

Do you have a technical reason that this patch series does not fix the
write/writev issue brought up by Al?

Ira

[toc] | [prev] | [next] | [standalone]

#1380488

[Multipart message — attachments visible in raw view] — view raw

On Fri, Apr 15, 2016 at 07:28:01PM -0400, Ira Weiny wrote:
> On Sat, Apr 16, 2016 at 12:23:28AM +0300, Leon Romanovsky wrote:
> Do you have a technical reason that this patch series does not fix the
> write/writev issue brought up by Al?

Sure, I truly believe that we can do common API in a months time-frame
and I want to be focused on one transition path only (write/read -> new
API) and not on two parallel paths (ioctl -> new API and write/read ->
new API) plus support of all these intermediate steps.

The original request came after this driver was moved from staging to
RDMA stack, since the driver is still in staging, there is no need to
hurry up now.

> 
> Ira
> 

[toc] | [prev] | [next] | [standalone]

#1380561 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Sat, Apr 16, 2016 at 09:09:40AM +0300, Leon Romanovsky wrote:
>On Fri, Apr 15, 2016 at 07:28:01PM -0400, Ira Weiny wrote:
>> On Sat, Apr 16, 2016 at 12:23:28AM +0300, Leon Romanovsky wrote:
>> Do you have a technical reason that this patch series does not fix the
>> write/writev issue brought up by Al?
>
>Sure, I truly believe that we can do common API in a months time-frame
>and I want to be focused on one transition path only (write/read -> new
>API) and not on two parallel paths (ioctl -> new API and write/read ->
>new API) plus support of all these intermediate steps.

That doesn't say anything about how this patch doesn't address Al and 
Linus's complaint, or raise a technical issue with the patch set.

These are two separate issues. I do not see a reason to try and make them 
one, and use this to drive the "one-device to rule them all" idea.  This 
series converts the write() to ioctl() and fixes the problem we set to, as 
promised. You don't like the API, that's fine.  We'll discuss that on 
linux-rdma, but no reason to hold this patch set while that happens. 

>The original request came after this driver was moved from staging to
>RDMA stack, since the driver is still in staging, there is no need to
>hurry up now.

There is no need to keep the driver in staging. This is not a driver that 
has style problems or is not well tested.  It is a driver that has been 
heavily tested, performs well and has completed its staging TODO list. We 
went ahead and added this write()/writev() fix before making the move 
because Al and Linus wanted that issue addressed. For the record:

 $ cat drivers/staging/rdma/hfi1/TODO
 July, 2015

 - Remove unneeded file entries in sysfs
 - Remove software processing of IB protocol and place in library for use
   by qib, ipath (if still present), hfi1, and eventually soft-roce

Both of those items are complete. The API issue was raised back when the 
driver was submitted (almost a year ago), as you can see it did not make the 
cut as a staging requirement. Whether you agree with the maintainer's 
decision or not. I don't see how it's fair to try and add it again now.

As I mentioned let's discuss the uAPI stuff on linux-rdma. Have the web 
meetings that you were mentioning and do whatever we need to in order to 
improve the sub-system, but stop trying to tie our driver and moving out of 
staging to this much larger issue.

Thanks

-Denny

[toc] | [prev] | [next] | [standalone]

#1380380

On Sat, Apr 16, 2016 at 12:23:28AM +0300, Leon Romanovsky wrote:

> Intel as usual decided to do it in their way and the result is presented
> on this mailing list.

Dennis was pretty clear he was going to send the patches to address
Al's concern, which he has done.

I was also pretty clear I was looking to get rid of the char dev :)

Not seeing a problem here.

Jason

[toc] | [prev] | [next] | [standalone]

#1380487

[Multipart message — attachments visible in raw view] — view raw

On Fri, Apr 15, 2016 at 05:37:32PM -0600, Jason Gunthorpe wrote:
> On Sat, Apr 16, 2016 at 12:23:28AM +0300, Leon Romanovsky wrote:
> 
> > Intel as usual decided to do it in their way and the result is presented
> > on this mailing list.
> 
> Dennis was pretty clear he was going to send the patches to address
> Al's concern, which he has done.
> 
> I was also pretty clear I was looking to get rid of the char dev :)

Yes, and I was pretty clear that we need to converge on one common API
prior to converting old code (including drivers in staging) in order to
do it once only.

[toc] | [prev] | [next] | [standalone]

#1380590

On Sat, Apr 16, 2016 at 09:00:42AM +0300, Leon Romanovsky wrote:
> On Fri, Apr 15, 2016 at 05:37:32PM -0600, Jason Gunthorpe wrote:
> > On Sat, Apr 16, 2016 at 12:23:28AM +0300, Leon Romanovsky wrote:
> > 
> > > Intel as usual decided to do it in their way and the result is presented
> > > on this mailing list.
> > 
> > Dennis was pretty clear he was going to send the patches to address
> > Al's concern, which he has done.
> > 
> > I was also pretty clear I was looking to get rid of the char dev :)
> 
> Yes, and I was pretty clear that we need to converge on one common API
> prior to converting old code (including drivers in staging) in order to
> do it once only.

While we are at it, could the person who'd come up with ui_lseek() be located
and made to stand up and explain the rationale behind the SEEK_END semantics
therein?  To quote the manpage (and paraphrase just about any introductory
textbook):
       SEEK_END
              The file offset is set to the size of the file plus offset bytes.

I'm really curious - which part of "plus" might have lead to
        case SEEK_END:
                offset = ((dd->kregend - dd->kregbase) + DC8051_DATA_MEM_SIZE) -
                        offset;
and, if its author has decided that of course it _must_ have meant "minus",
why had he or she failed to post a correction to the manpage?  Or, on the
off-chance that this "plus" might have something to do with reality,
experimented with some file, for that matter.

Folks, this is a well-earned "F".  And not just for Unix Programming 101 -
the same semantics applies to fseek(3), which is a part of C standard.
Incidentally, lseek(fd, 0, SEEK_END) is "seek to end", not "fail with EINVAL".

As for the use of ioctl...  Frankly, considering the above, it does sound like
"that'll make them STFU about the weirdness - ioctl *is* weird, so there!"

Single-consumer APIs stink, film at 11...

[toc] | [prev] | [next] | [standalone]

#1381645 — Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access

On Sat, Apr 16, 2016 at 08:19:17PM +0100, Al Viro wrote:

>While we are at it, could the person who'd come up with ui_lseek() be located
>and made to stand up and explain the rationale behind the SEEK_END semantics
>therein?  To quote the manpage (and paraphrase just about any introductory
>textbook):
>       SEEK_END
>              The file offset is set to the size of the file plus offset bytes.
>
>I'm really curious - which part of "plus" might have lead to
>        case SEEK_END:
>                offset = ((dd->kregend - dd->kregbase) + DC8051_DATA_MEM_SIZE) -
>                        offset;
>and, if its author has decided that of course it _must_ have meant "minus",
>why had he or she failed to post a correction to the manpage?  Or, on the

Original author of that code confirmed it is just a coding mistake and we 
will fix it.

-Denny

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web