Re: New subsystem for acceleration devices

[Jason Gunthorpe <jgg@nvidia.com>]

On Thu, Aug 04, 2022 at 08:48:28PM +0300, Oded Gabbay wrote:

> > The flip is true of DRM - DRM is pretty general. I bet I could
> > implement an RDMA device under DRM - but that doesn't mean it should
> > be done.
> >
> > My biggest concern is that this subsystem not turn into a back door
> > for a bunch of abuse of kernel APIs going forward. Though things
> > are
>
> How do you suggest to make sure it won't happen ?

Well, if you launch the subsystem then it is your job to make sure it
doesn't happen - or endure the complaints :)

Accelerators have this nasty tendancy to become co-designed with their
SOCs in nasty intricate ways and then there is a desire to punch
through all the inconvenient layers to make it go faster.

> > better now, we still see this in DRM where expediency or performance
> > justifies hacky shortcuts instead of good in-kernel architecture. At
> > least DRM has reliable and experienced review these days.
> Definitely. DRM has some parts that are really well written. For
> example, the whole char device handling with sysfs/debugfs and managed
> resources code. 

Arguably this should all be common code in the driver core/etc - what
value is a subsystem adding beyond that besides using it properly?

It would be nice to at least identify something that could obviously
be common, like some kind of enumeration and metadata kind of stuff
(think like ethtool, devlink, rdma tool, nvemctl etc)

> I think that it is clear from my previous email what I intended to
> provide. A clean, simple framework for devices to register with, get
> services for the most basic stuff such as device char handling,
> sysfs/debugfs. 

This should all be trivially done by any driver using the core codes,
if you see gaps I'd ask why not improve the core code?

> Later on, add more simple stuff such as memory manager
> and uapi for memory handling. I guess someone can say all that exists
> in drm, but like I said it exists in other subsystems as well.

This makes sense to me, all accelerators need a way to set a memory
map, but on the other hand we are doing some very nifty stuff in this
area with iommufd and it might be very interesting to just have the
accelerator driver link to that API instead of building yet another
copy of pin_user_pages() code.. Especially with PASID likely becoming
part of any accelerator toolkit.

> I want to be perfectly honest and say there is nothing special here
> for AI. It's actually the opposite, it is a generic framework for
> compute only. Think of it as an easier path to upstream if you just
> want to do compute acceleration. Maybe in the future it will be more,
> but I can't predict the future.

I can't either, and to be clear I'm only questioning the merit of a
"subsystem" eg with a struct class, rigerous uAPI, etc.

The idea that these kinds of accel drivers deserve specialized review
makes sense to me, even if they remain as unorganized char
devices. Just understand that is what you are signing up for :)

Jason