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

Groups > linux.kernel > #1379051 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-14 17:50 +0200
    Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-14 18:50 +0200
      Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Ira Weiny <ira.weiny@intel.com> - 2016-04-14 19:50 +0200
        Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-14 20:10 +0200
          Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-14 20:50 +0200
            Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-14 21:00 +0200
        Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Leon Romanovsky <leon@leon.nu> - 2016-04-15 06:10 +0200
          Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Ira Weiny <ira.weiny@intel.com> - 2016-04-15 18:20 +0200
      Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user access Dennis Dalessandro <dennis.dalessandro@intel.com> - 2016-04-14 20:00 +0200
        Re: [PATCH 0/7] IB/hfi1: Remove write() and use ioctl() for user  access Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-04-14 20:50 +0200

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

This patch series removes the write() interface for user access in favor of an
ioctl() based approach. This is in response to the complaint that we had
different handlers for write() and writev() doing different things and expecting
different types of data. See:

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

In a response to that thread we mentioned some other possible approaches such
as using multiple files, or converting everything to writev(). However after
looking at things more it seemed a cleaner approach to use ioctl(). 

So each command that was being done through write() has been converted to
ioctl() and the write() interface is removed. We still use writev() for the data
path but control is done totally through ioctl().

The plan is to make the same sort of change in qib as well but we want to get
the opinion of the community on the approach first.

As part of this work I also decided to move the eprom functionality to its own
device (last patch) since it really is its own thing. It uses ioctl() as well,
again because it seems to be a more natural interface for this operation.

There is also a driver software version being exported via a sysfs file. This is
needed so that user space applications (psm) can  determine if it needs to do
ioctl() or write().

This patch applies on the latest hfi1 patch set "Fix link and other issues".

Patches can also be viewed on GitHub at:
https://github.com/ddalessa/kernel/tree/for-4.7

---

Dennis Dalessandro (7):
      IB/hfi1: Export drivers user sw version via sysfs
      IB/hfi1: Remove unused user command
      IB/hfi1: Add ioctl() interface for user commands
      IB/hfi1: Remove write(), use ioctl() for user cmds
      IB/hfi1: Add trace message in user IOCTL handling
      IB/hfi1: Consolidate IOCTL defines
      IB/hfi1: Move eprom to its own device


 drivers/staging/rdma/hfi1/common.h   |    3 
 drivers/staging/rdma/hfi1/diag.c     |  170 ++++++++++++++++++---------
 drivers/staging/rdma/hfi1/eprom.c    |  121 +------------------
 drivers/staging/rdma/hfi1/eprom.h    |   16 ++-
 drivers/staging/rdma/hfi1/file_ops.c |  213 ++++++++++++++--------------------
 drivers/staging/rdma/hfi1/hfi.h      |   22 +++-
 drivers/staging/rdma/hfi1/sysfs.c    |    8 +
 drivers/staging/rdma/hfi1/trace.c    |    1 
 drivers/staging/rdma/hfi1/trace.h    |    1 
 include/uapi/rdma/hfi/hfi1_user.h    |  106 ++++++++++++++++-
 10 files changed, 350 insertions(+), 311 deletions(-)

-- 
-Denny

[toc] | [next] | [standalone]

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

On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
> This patch series removes the write() interface for user access in favor of an
> ioctl() based approach. This is in response to the complaint that we had
> different handlers for write() and writev() doing different things and expecting
> different types of data. See:

I think we should wait on applying these patches until we globally sort out
what to do with the rdma uapi.

It just doesn't make alot of sense for drivers to have their own personal
char devices. :(

A second char dev for the eeprom? How is that OK? Why aren't you using
the I2C layer for this?

Why is there a snoop interface in here? How is that not something that
belongs in a the core code?

Jason

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

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

On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
> On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
> > This patch series removes the write() interface for user access in favor of an
> > ioctl() based approach. This is in response to the complaint that we had
> > different handlers for write() and writev() doing different things and expecting
> > different types of data. See:
> 
> I think we should wait on applying these patches until we globally sort out
> what to do with the rdma uapi.
> 
> It just doesn't make alot of sense for drivers to have their own personal
> char devices. :(

I'm afraid I have to disagree at this time.  Someday we may have "1 char device
to rule them all" but right now we don't have any line of sight to that
solution.  It may be _years_ before we can agree to the semantics which will
work for all high speed, kernel bypass, rdma, low latency, network devices.

We need to fix the write/writev problem now.[1]

Ira

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

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

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

On Thu, Apr 14, 2016 at 01:48:31PM -0400, Ira Weiny wrote:
> On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
> > On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
> > > This patch series removes the write() interface for user access in favor of an
> > > ioctl() based approach. This is in response to the complaint that we had
> > > different handlers for write() and writev() doing different things and expecting
> > > different types of data. See:
> > 
> > I think we should wait on applying these patches until we globally sort out
> > what to do with the rdma uapi.
> > 
> > It just doesn't make alot of sense for drivers to have their own personal
> > char devices. :(
> 
> I'm afraid I have to disagree at this time.  Someday we may have "1 char device
> to rule them all" but right now we don't have any line of sight to that
> solution.  It may be _years_ before we can agree to the semantics which will
> work for all high speed, kernel bypass, rdma, low latency, network devices.

There are some pretty obvious paths to make this saner that could only
be a few weeks away, we haven't even had the first conversations
yet. I think you are completely wrong there is no 'line of sight'

It certainly can't be years.

There is some rational for a very driver specific thing, but EEPROM
and snoop? Seriously?

Jason

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

#1379216

On Thu, Apr 14, 2016 at 12:05:40PM -0600, Jason Gunthorpe wrote:
>There are some pretty obvious paths to make this saner that could only
>be a few weeks away, we haven't even had the first conversations
>yet. I think you are completely wrong there is no 'line of sight'
>
>It certainly can't be years.

Does fixing the current write()/writev() problem have any real impact on how 
we proceed for the "1 char dev to rule them all" idea?

>There is some rational for a very driver specific thing, but EEPROM
>and snoop? Seriously?

That's the thing, I think these are very driver specific [1]. I'm not dead 
set that the eprom needs to be its own device, it made sense to me, but if 
others feel the handling should be back in the hfi1 char device I'm fine 
with that.

As for the snoop stuff, perhaps that would be better in rdmavt?

[1] http://marc.info/?l=linux-rdma&m=146065638629146&w=2

-Denny

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

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

On Thu, Apr 14, 2016 at 02:42:01PM -0400, Dennis Dalessandro wrote:
> >It certainly can't be years.
> 
> Does fixing the current write()/writev() problem have any real
> impact on how we proceed for the "1 char dev to rule them all" idea?

We aren't going to take a bad uAPI into mainline. So how many times do
you want to redo the userspace? I have no objection to the patch
landing, just as long as it stays in staging until we have the uAPI
discussion as a community.

As for the 'one char device', I actually think it would be really
simple.

Add a new uverbs ioctl:

 int hfi1_fd = ioctl(uverbs_fd, RDMA_GET_DRIVER_OPS_FD, "psm2.intel.com");

 ioctl(hfi1_fd, HFI1_IOCTL_ASSIGN_CTXT, ...);
 write(hfi1_fd, ...);

At least that gives us far better options for discovery and versioning
of this stuff than a driver-specific char device.

[eg this would use anon_inode_getfile, like event fds, completion
channels, etc]

You guys need this the most, propose something already.

* driver specific ioctls might be nicer, but people argue that is not
  performant enough for what you want... Unclear to me.

Jason

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

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

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

On Thu, Apr 14, 2016 at 01:48:31PM -0400, Ira Weiny wrote:
> On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
> > On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
> > > This patch series removes the write() interface for user access in favor of an
> > > ioctl() based approach. This is in response to the complaint that we had
> > > different handlers for write() and writev() doing different things and expecting
> > > different types of data. See:
> > 
> > I think we should wait on applying these patches until we globally sort out
> > what to do with the rdma uapi.
> > 
> > It just doesn't make alot of sense for drivers to have their own personal
> > char devices. :(
> 
> I'm afraid I have to disagree at this time.  Someday we may have "1 char device
> to rule them all" but right now we don't have any line of sight to that
> solution.  It may be _years_ before we can agree to the semantics which will
> work for all high speed, kernel bypass, rdma, low latency, network devices.

You didn't ever try to come and work on the solution. We talked about
finite time frame (_months_) which is doable based on knowledge that user
space parts are developed by the same companies and all our future changes
will be in one subsystem.

You were supposed to prepare "wish list" from this new API as an initial
phase. If you do it, you will find that it is very short and in the
initial meeting you will see that it similar to other participants in
linux-rdma community.

> 
> We need to fix the write/writev problem now.[1]

No, this driver in staging and the proper way to move it out will be to
converge on common API and one clear path instead of duplicating the
interfaces and "inventing the wheel".

> 
> Ira
> 
> [1] https://www.spinics.net/lists/linux-rdma/msg34451.html
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

On Fri, Apr 15, 2016 at 07:01:26AM +0300, Leon Romanovsky wrote:
> On Thu, Apr 14, 2016 at 01:48:31PM -0400, Ira Weiny wrote:
> > On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
> > > On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
> > > > This patch series removes the write() interface for user access in favor of an
> > > > ioctl() based approach. This is in response to the complaint that we had
> > > > different handlers for write() and writev() doing different things and expecting
> > > > different types of data. See:
> > > 
> > > I think we should wait on applying these patches until we globally sort out
> > > what to do with the rdma uapi.
> > > 
> > > It just doesn't make alot of sense for drivers to have their own personal
> > > char devices. :(
> > 
> > I'm afraid I have to disagree at this time.  Someday we may have "1 char device
> > to rule them all" but right now we don't have any line of sight to that
> > solution.  It may be _years_ before we can agree to the semantics which will
> > work for all high speed, kernel bypass, rdma, low latency, network devices.
> 
> You didn't ever try to come and work on the solution. We talked about
> finite time frame (_months_) which is doable based on knowledge that user
> space parts are developed by the same companies and all our future changes
> will be in one subsystem.

How can you say that I am not working on a solution?

We spent most of last week discussing possible solutions and I am in support of
a more common core.  But ask yourself this.


If hfi1 did not support verbs at all would this even be an issue?


> 
> You were supposed to prepare "wish list" from this new API as an initial
> phase. If you do it, you will find that it is very short and in the
> initial meeting you will see that it similar to other participants in
> linux-rdma community.

The list of operations may be short.  But the way in which you do those in a
performant way for each hardware device is _very_ different.  This is a problem
which has been debated for years and no one has come up with an elegant
solution.  Every solution ends up being, to quote a presenter at last weeks
conference, "shoving a square peg into a round hole".

Until we all admit 2 things.

	1) That there are devices which don't operate on QPs
	2) That the High Speed interconnect core should present something more
	   abstract than a QP interface

we are not really creating a common layer.

I do admit Jasons idea has some merit but I'm just not sure it provides so much
benefit that it is worth the effort at this time.

Ira

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

#1379181

On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
>On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
>> This patch series removes the write() interface for user access in favor of an
>> ioctl() based approach. This is in response to the complaint that we had
>> different handlers for write() and writev() doing different things and expecting
>> different types of data. See:
>
>I think we should wait on applying these patches until we globally sort out
>what to do with the rdma uapi.

Perhaps there is a broader change to make to the rdma subsystem, but until 
that is fleshed out this patch set achieves our goal of fixing the 
write()/writev() problem and should be sufficient to let the driver come out 
of staging for 4.7?

>A second char dev for the eeprom? How is that OK? Why aren't you using
>the I2C layer for this?

I moved it because it is totally different in terms of functionality. The 
hfi1 device is for send/recv of packets across the wire. The eprom device is
for low level programming of the eprom on the chip. We do not use i2c for 
this because the eprom is directly attached to the chip and not accessible 
via i2c, requires register access.

>Why is there a snoop interface in here? How is that not something that
>belongs in a the core code?

The snoop interface is a low level diagnostic for the hfi. The intent is to 
grab packets before they are handed up to the verbs layer. It also lets us 
send all sorts of debug/diagnostic packets for testing.

-Denny

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

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

On Thu, Apr 14, 2016 at 01:52:44PM -0400, Dennis Dalessandro wrote:
> On Thu, Apr 14, 2016 at 10:45:50AM -0600, Jason Gunthorpe wrote:
> >On Thu, Apr 14, 2016 at 08:41:35AM -0700, Dennis Dalessandro wrote:
> >>This patch series removes the write() interface for user access in favor of an
> >>ioctl() based approach. This is in response to the complaint that we had
> >>different handlers for write() and writev() doing different things and expecting
> >>different types of data. See:
> >
> >I think we should wait on applying these patches until we globally sort out
> >what to do with the rdma uapi.
> 
> Perhaps there is a broader change to make to the rdma subsystem, but until
> that is fleshed out this patch set achieves our goal of fixing the
> write()/writev() problem and should be sufficient to let the driver come out
> of staging for 4.7?

No. Al and Linus have clearly put the kibosh on the idea that a driver
gets a pass on whatever uAPI stuff they want just because it is in a
driver.

If anything adding uAPIs to drivers should be *harder* than adding
them to the core kernel. You nedd a lot more justification why the
core code shouldn't have a well designed version of the function.

You guys need to integrate with the rest of the kernel in some way,
this is just not OK. We catch so much flack from the rest of the
kernel community for our shitty uAPIs, we need to grow up.

I accept the argument that you need special high speed hardware
specific uAPIs for PSM - fine, but that doesn't give hfi1 a free pass
to add whatever other kooky things you find convenient. No to eeprom,
no to snoop.

If you want to migrate out of staging quickly then drop the uAPI from
the driver and submit a sane uAPI later as patches.

IMHO, it was a mistake for Roland to accept ipath with all this uAPI
stuff, and a double mistake to give qib an equal pass. hfi1 is adding
*even more* stuff, with flimsy justification. Enough is enough.

> >A second char dev for the eeprom? How is that OK? Why aren't you using
> >the I2C layer for this?
> 
> I moved it because it is totally different in terms of
> functionality. The

Nobody else is doing something like this. It is crazy.

Add a common RDMA API for eeprom. net has one under ethtool, it is
about time we grow something too, all the vendors seem to have various
hacks in this department. Maybe it fits under RDMA's growing netlink
footprint.

> >Why is there a snoop interface in here? How is that not something that
> >belongs in a the core code?
> 
> The snoop interface is a low level diagnostic for the hfi. The intent is to
> grab packets before they are handed up to the verbs layer. It also lets us
> send all sorts of debug/diagnostic packets for testing.

So? Why is that unique to hfi1? Packet capture is a well understood
multi-vendor thing.

Nobody else is getting a pass on uAPI design. Thing is, I don't think
it is actually hard to do a good job with the uAPI here, you just
actually have to try. :P

I told John I'd give you guys some design advice. I suggest you give
it a good think, make your wishlist and lets do something sane.

Jason

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web