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

Groups > linux.kernel > #1480664 > unrolled thread

Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller

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: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Christoph Hellwig <hch@lst.de> - 2016-09-10 18:20 +0200
    Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-09-10 19:10 +0200
      Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Christoph Hellwig <hch@lst.de> - 2016-09-11 15:40 +0200
        Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Leon Romanovsky <leon@kernel.org> - 2016-09-11 16:40 +0200
          Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-09-11 19:20 +0200
            Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Christoph Hellwig <hch@lst.de> - 2016-09-11 19:30 +0200
              Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Jason Gunthorpe <jgunthorpe@obsidianresearch.com> - 2016-09-11 20:00 +0200
                Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Leon Romanovsky <leon@kernel.org> - 2016-09-12 07:10 +0200
                  Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-09-14 09:10 +0200
                    Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-09-14 11:30 +0200
                    Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Leon Romanovsky <leon@kernel.org> - 2016-09-15 21:00 +0200
                      Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-09-21 06:50 +0200
                        Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Tejun Heo <tj@kernel.org> - 2016-09-21 16:30 +0200
                          Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-09-21 18:10 +0200
                    Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller "Dalessandro, Dennis" <dennis.dalessandro@intel.com> - 2016-09-19 15:30 +0200
                      Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller Parav Pandit <pandit.parav@gmail.com> - 2016-09-19 19:10 +0200

#1480664 — Re: [PATCHv12 1/3] rdmacg: Added rdma cgroup controller

On Wed, Sep 07, 2016 at 11:51:42AM +0300, Matan Barak wrote:
> All recent proposals of the new ABI schema deals with extending the 
> flexibility of the current schema by letting drivers define their specific 
> types, actions, attributes, etc. Even more than that, the dispatching 
> starts from the driver and it chooses if it wants to use the common RDMA 
> core layer or have it's own wise implementation instead.
> Some drivers might even prefer not to implement the current verbs types.
> These decisions were made in the OFVWG meetings.

OFVWG meetings have absolutely zero relevance for Linux development.
More "flexibility" for drivers just means giving up on designing a
coherent API and leaving it to drivers authors to add crap to their
own drivers.  That's a major step backwards.

> Sounds reasonable, but what about drivers which ignore the common code and 
> implement it in their own way? What about drivers which don't support the 
> standard RDMA types at all?

They should not be using the code in drivers/infiniband.  usnic is such
an example of a driver that should never have been added in it's current
form.

[toc] | [next] | [standalone]

#1480676

On Sat, Sep 10, 2016 at 06:14:42PM +0200, Christoph Hellwig wrote:
> OFVWG meetings have absolutely zero relevance for Linux development.

Well, to be fair there are a fair number of kernel developers on that
particular call..

> More "flexibility" for drivers just means giving up on designing a
> coherent API and leaving it to drivers authors to add crap to their
> own drivers.  That's a major step backwards.

Sadly, it isn't a step backwards, it is status quo - at least as far
as the uapi is concerned.

Every single user space driver has its own private abi file, carefully
hidden in their driver, and dutifully copied over to user space:

providers/cxgb3/iwch-abi.h
providers/cxgb4/cxgb4-abi.h
providers/hfi1verbs/hfi-abi.h
providers/i40iw/i40iw-abi.h
providers/ipathverbs/ipath-abi.h
providers/mlx4/mlx4-abi.h
providers/mlx5/mlx5-abi.h
providers/mthca/mthca-abi.h
providers/nes/nes-abi.h
providers/ocrdma/ocrdma_abi.h
providers/rxe/rxe-abi.h

Just to pick two random examples:

struct mlx5_create_cq {
        struct ibv_create_cq            ibv_cmd;
        __u64                           buf_addr;
        __u64                           db_addr;
        __u32				cqe_size;
};

struct iwch_create_cq {
        struct ibv_create_cq ibv_cmd;
        uint64_t user_rptr_addr;
};

Love to hear ideas on a way forward that doesn't involve rewriting
everything :(

> They should not be using the code in drivers/infiniband.  usnic is such
> an example of a driver that should never have been added in it's current
> form.

+1

Jason

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

#1480803

On Sat, Sep 10, 2016 at 11:01:51AM -0600, Jason Gunthorpe wrote:
> Sadly, it isn't a step backwards, it is status quo - at least as far
> as the uapi is concerned.

Sort of, see below:

> struct mlx5_create_cq {
>         struct ibv_create_cq            ibv_cmd;
>         __u64                           buf_addr;
>         __u64                           db_addr;
>         __u32				cqe_size;
> };
> 
> struct iwch_create_cq {
>         struct ibv_create_cq ibv_cmd;
>         uint64_t user_rptr_addr;
> };
> 
> Love to hear ideas on a way forward that doesn't involve rewriting
> everything :(

We stil always have the common structure first.  And at least for
cgroups supports that's what matters.

Re the actual structures - we'll really need to make sure we

 a) expose proper userspace abi headers in the kernel for all code
    in the RDMA subsystem
 b) actually use that in the userspace components

I've posted some initial work toward a) a while ago, and once we
agree on adopting your common repo I'd really like to start through
with that work.  I think it's a pre-requisite for any major new
userspace ABI work.

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

#1480825

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

On Sun, Sep 11, 2016 at 03:34:21PM +0200, Christoph Hellwig wrote:
> On Sat, Sep 10, 2016 at 11:01:51AM -0600, Jason Gunthorpe wrote:
> > Sadly, it isn't a step backwards, it is status quo - at least as far
> > as the uapi is concerned.
>
> Sort of, see below:
>
> > struct mlx5_create_cq {
> >         struct ibv_create_cq            ibv_cmd;
> >         __u64                           buf_addr;
> >         __u64                           db_addr;
> >         __u32				cqe_size;
> > };
> >
> > struct iwch_create_cq {
> >         struct ibv_create_cq ibv_cmd;
> >         uint64_t user_rptr_addr;
> > };
> >
> > Love to hear ideas on a way forward that doesn't involve rewriting
> > everything :(
>
> We stil always have the common structure first.  And at least for
> cgroups supports that's what matters.
>
> Re the actual structures - we'll really need to make sure we
>
>  a) expose proper userspace abi headers in the kernel for all code
>     in the RDMA subsystem
>  b) actually use that in the userspace components
>
> I've posted some initial work toward a) a while ago, and once we
> agree on adopting your common repo I'd really like to start through
> with that work.  I think it's a pre-requisite for any major new
> userspace ABI work.

I started to work on it over weekend and it is worth do not do same work twice.

> --
> 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]

#1480839

On Sun, Sep 11, 2016 at 05:35:22PM +0300, Leon Romanovsky wrote:

> > We stil always have the common structure first.  And at least for
> > cgroups supports that's what matters.
> >
> > Re the actual structures - we'll really need to make sure we
> >
> >  a) expose proper userspace abi headers in the kernel for all code
> >     in the RDMA subsystem
> >  b) actually use that in the userspace components
> >
> > I've posted some initial work toward a) a while ago, and once we

Did it get merged? Do you have a pointer?

> > agree on adopting your common repo I'd really like to start through
> > with that work.  I think it's a pre-requisite for any major new
> > userspace ABI work.
> 
> I started to work on it over weekend and it is worth do not do same work twice.

Yes, I also agree that it is important before we tackle the uapi
conversion to get this fully sorted.

I've already done several cases working with the existing uapi headers:

https://github.com/jgunthorpe/rdma-plumbing/commit/f4f40689440dbc9c57b55548b04b15fe808a1767
https://github.com/jgunthorpe/rdma-plumbing/commit/0cf1893dce4791dafa035bcb6ee045a6ce0ff3c3
https://github.com/jgunthorpe/rdma-plumbing/commit/0522fc42aac4a5e8fc888dcca4341c9bc1dc58ca

[.. and this is a strong argument why we need the common repo, doing
this without it would be very hard, as everything is cross-linked, I
couldnn't unwind libibcm until I fixed a bit of verbs, and rdmacm can't
even include its uapi header until the duplicate definitions in the
verbs copy are delt with .. and I've also learned we are making
changing to the kernel uapi header and since nothing uses them we never even
compile test :( :( eg
https://github.com/torvalds/linux/commit/b493d91d333e867a043f7ff1397bcba6e2d0dda2]

However, everything under verbs is not straightforward. The files in
userspace are not copies...

user:

struct ibv_query_device {
       __u32 command;
       __u16 in_words;
       __u16 out_words;
       __u64 response;
       __u64 driver_data[0];
};

kernel:

struct ib_uverbs_query_device {
        __u64 response;
        __u64 driver_data[0];
};

eg the userspace version stuffs the header into the struct and the
kernel version does not. Presumably this is for efficiency so that no
copies are required when marshaling. This impacts everything :(

I'm thinking the best way forward might be to use a script and
transform userspace into:

struct ibv_query_device {
	struct ib_uverbs_cmd_hdr hdr;
	struct ib_uverbs_query_device cmd;
};

Jason

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

#1480841

On Sun, Sep 11, 2016 at 11:14:09AM -0600, Jason Gunthorpe wrote:
> > > We stil always have the common structure first.  And at least for
> > > cgroups supports that's what matters.
> > >
> > > Re the actual structures - we'll really need to make sure we
> > >
> > >  a) expose proper userspace abi headers in the kernel for all code
> > >     in the RDMA subsystem
> > >  b) actually use that in the userspace components
> > >
> > > I've posted some initial work toward a) a while ago, and once we
> 
> Did it get merged? Do you have a pointer?

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

> this without it would be very hard, as everything is cross-linked, I
> couldnn't unwind libibcm until I fixed a bit of verbs, and rdmacm can't
> even include its uapi header until the duplicate definitions in the
> verbs copy are delt with .. and I've also learned we are making
> changing to the kernel uapi header and since nothing uses them we never even
> compile test :( :( eg
> https://github.com/torvalds/linux/commit/b493d91d333e867a043f7ff1397bcba6e2d0dda2]

> However, everything under verbs is not straightforward. The files in
> userspace are not copies...
> 
> user:
> 
> struct ibv_query_device {
>        __u32 command;
>        __u16 in_words;
>        __u16 out_words;
>        __u64 response;
>        __u64 driver_data[0];
> };
> 
> kernel:
> 
> struct ib_uverbs_query_device {
>         __u64 response;
>         __u64 driver_data[0];
> };

We'll obviously need different strutures for the libibvers API
and the kernel interface in this case, and we'll need to figure out
how to properly translate them.  I think a cast, plus compile time
type checking ala BUILD_BUG_ON is the way to go.

> eg the userspace version stuffs the header into the struct and the
> kernel version does not. Presumably this is for efficiency so that no
> copies are required when marshaling. This impacts everything :(
> 
> I'm thinking the best way forward might be to use a script and
> transform userspace into:
> 
> struct ibv_query_device {
> 	struct ib_uverbs_cmd_hdr hdr;
> 	struct ib_uverbs_query_device cmd;
> };

That would break the users of the interface.  However automatically
generating the user ABI from the kernel one might still be a good idea
in the long run.

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

#1480844

On Sun, Sep 11, 2016 at 07:24:45PM +0200, Christoph Hellwig wrote:
> > > > I've posted some initial work toward a) a while ago, and once we
> > 
> > Did it get merged? Do you have a pointer?
> 
> http://www.spinics.net/lists/linux-rdma/msg31958.html

Right, I remember that. Certainly the right direction

> > However, everything under verbs is not straightforward. The files in
> > userspace are not copies...
> > 
> > user:
> > 
> > struct ibv_query_device {
> >        __u32 command;
> >        __u16 in_words;
> >        __u16 out_words;
> >        __u64 response;
> >        __u64 driver_data[0];
> > };
> > 
> > kernel:
> > 
> > struct ib_uverbs_query_device {
> >         __u64 response;
> >         __u64 driver_data[0];
> > };
> 
> We'll obviously need different strutures for the libibvers API
> and the kernel interface in this case, and we'll need to figure out
> how to properly translate them.  I think a cast, plus compile time
> type checking ala BUILD_BUG_ON is the way to go.

I'm not sure I follow, which would I cast?

BUILD_BUG_ON(sizeof(ibv_query_device) == sizeof(ib_uverbs_cmd_hdr) +
             sizeof(ib_uverbs_query_device))

?

> > I'm thinking the best way forward might be to use a script and
> > transform userspace into:
> > 
> > struct ibv_query_device {
> > 	struct ib_uverbs_cmd_hdr hdr;
> > 	struct ib_uverbs_query_device cmd;
> > };
> 
> That would break the users of the interface.

Sorry, I mean doing this inside rdma-plumbing. Since the change is ABI
identical the modified libibverbs would still be binary compatible
with all providers but not source compatible. Since all kernel
supported providers are in rdma-plumbing we can add the '.cmd.' at the
same time.

The kernel uapi header would stay the same.

> However automatically generating the user ABI from the kernel one
> might still be a good idea in the long run.

My preference would be to try and use the kernel headers directly.

Jason

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

#1480923

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

On Sun, Sep 11, 2016 at 11:52:35AM -0600, Jason Gunthorpe wrote:
> On Sun, Sep 11, 2016 at 07:24:45PM +0200, Christoph Hellwig wrote:
> > > > > I've posted some initial work toward a) a while ago, and once we
> > >
> > > Did it get merged? Do you have a pointer?
> >
> > http://www.spinics.net/lists/linux-rdma/msg31958.html
>
> Right, I remember that. Certainly the right direction
>
> > > However, everything under verbs is not straightforward. The files in
> > > userspace are not copies...
> > >
> > > user:
> > >
> > > struct ibv_query_device {
> > >        __u32 command;
> > >        __u16 in_words;
> > >        __u16 out_words;
> > >        __u64 response;
> > >        __u64 driver_data[0];
> > > };
> > >
> > > kernel:
> > >
> > > struct ib_uverbs_query_device {
> > >         __u64 response;
> > >         __u64 driver_data[0];
> > > };
> >
> > We'll obviously need different strutures for the libibvers API
> > and the kernel interface in this case, and we'll need to figure out
> > how to properly translate them.  I think a cast, plus compile time
> > type checking ala BUILD_BUG_ON is the way to go.
>
> I'm not sure I follow, which would I cast?
>
> BUILD_BUG_ON(sizeof(ibv_query_device) == sizeof(ib_uverbs_cmd_hdr) +
>              sizeof(ib_uverbs_query_device))
>
> ?
>
> > > I'm thinking the best way forward might be to use a script and
> > > transform userspace into:
> > >
> > > struct ibv_query_device {
> > > 	struct ib_uverbs_cmd_hdr hdr;
> > > 	struct ib_uverbs_query_device cmd;
> > > };
> >
> > That would break the users of the interface.
>
> Sorry, I mean doing this inside rdma-plumbing. Since the change is ABI
> identical the modified libibverbs would still be binary compatible
> with all providers but not source compatible. Since all kernel
> supported providers are in rdma-plumbing we can add the '.cmd.' at the
> same time.
>
> The kernel uapi header would stay the same.
>
> > However automatically generating the user ABI from the kernel one
> > might still be a good idea in the long run.
>
> My preference would be to try and use the kernel headers directly.

I thought the same, especially after realizing that they are almost
copy/paste from the vendor *-abi.h files.

>
> Jason

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

#1482967

Hi Dennis,

Do you know how would HFI1 driver would work along with rdma cgroup?

Hi Matan, Leon, Jason,
Apart from HFI1, is there any other concern?
Or Patch is good to go?

4.8 dates are close by (2 weeks) and there are two git trees involved
(that might cause merge error to Linus) so if there are no issues, I
would like to make request to Doug to consider it for 4.8 early on.

Parav

On Mon, Sep 12, 2016 at 10:37 AM, Leon Romanovsky <leon@kernel.org> wrote:
> On Sun, Sep 11, 2016 at 11:52:35AM -0600, Jason Gunthorpe wrote:
>> On Sun, Sep 11, 2016 at 07:24:45PM +0200, Christoph Hellwig wrote:
>> > > > > I've posted some initial work toward a) a while ago, and once we
>> > >
>> > > Did it get merged? Do you have a pointer?
>> >
>> > http://www.spinics.net/lists/linux-rdma/msg31958.html
>>
>> Right, I remember that. Certainly the right direction
>>
>> > > However, everything under verbs is not straightforward. The files in
>> > > userspace are not copies...
>> > >
>> > > user:
>> > >
>> > > struct ibv_query_device {
>> > >        __u32 command;
>> > >        __u16 in_words;
>> > >        __u16 out_words;
>> > >        __u64 response;
>> > >        __u64 driver_data[0];
>> > > };
>> > >
>> > > kernel:
>> > >
>> > > struct ib_uverbs_query_device {
>> > >         __u64 response;
>> > >         __u64 driver_data[0];
>> > > };
>> >
>> > We'll obviously need different strutures for the libibvers API
>> > and the kernel interface in this case, and we'll need to figure out
>> > how to properly translate them.  I think a cast, plus compile time
>> > type checking ala BUILD_BUG_ON is the way to go.
>>
>> I'm not sure I follow, which would I cast?
>>
>> BUILD_BUG_ON(sizeof(ibv_query_device) == sizeof(ib_uverbs_cmd_hdr) +
>>              sizeof(ib_uverbs_query_device))
>>
>> ?
>>
>> > > I'm thinking the best way forward might be to use a script and
>> > > transform userspace into:
>> > >
>> > > struct ibv_query_device {
>> > >   struct ib_uverbs_cmd_hdr hdr;
>> > >   struct ib_uverbs_query_device cmd;
>> > > };
>> >
>> > That would break the users of the interface.
>>
>> Sorry, I mean doing this inside rdma-plumbing. Since the change is ABI
>> identical the modified libibverbs would still be binary compatible
>> with all providers but not source compatible. Since all kernel
>> supported providers are in rdma-plumbing we can add the '.cmd.' at the
>> same time.
>>
>> The kernel uapi header would stay the same.
>>
>> > However automatically generating the user ABI from the kernel one
>> > might still be a good idea in the long run.
>>
>> My preference would be to try and use the kernel headers directly.
>
> I thought the same, especially after realizing that they are almost
> copy/paste from the vendor *-abi.h files.
>
>>
>> Jason

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

#1483126

Hi Matan,

On Wed, Sep 14, 2016 at 1:44 PM, Matan Barak <matanb@mellanox.com> wrote:
> On 14/09/2016 10:06, Parav Pandit wrote:
>>
>> Hi Dennis,
>>
>> Do you know how would HFI1 driver would work along with rdma cgroup?
>>
>> Hi Matan, Leon, Jason,
>> Apart from HFI1, is there any other concern?
>
>
> I just wonder how things like RSS will work. For example, a RSS QP doesn't
> really have a queue (if I recall, it's connected to work queues via an
> indirection table). So, when a user creates such a QP, do you want to
> account it as a regular QP?
> How are work queues accounted?

ib_create_rwq_ind_table verb allows creating indirection table.
I assume it allows creating multiple such tables.
If it is so, than number of tables should be a cgroup resource that we
can add in follow on patch.
By doing so, one container doesn't takeaway all the tables.

>
>
>> Or Patch is good to go?
>>
>> 4.8 dates are close by (2 weeks) and there are two git trees involved
>> (that might cause merge error to Linus) so if there are no issues, I
>> would like to make request to Doug to consider it for 4.8 early on.
>>
>> Parav
>>
>> On Mon, Sep 12, 2016 at 10:37 AM, Leon Romanovsky <leon@kernel.org> wrote:
>>>
>>> On Sun, Sep 11, 2016 at 11:52:35AM -0600, Jason Gunthorpe wrote:
>>>>
>>>> On Sun, Sep 11, 2016 at 07:24:45PM +0200, Christoph Hellwig wrote:
>>>>>>>>
>>>>>>>> I've posted some initial work toward a) a while ago, and once we
>>>>>>
>>>>>>
>>>>>> Did it get merged? Do you have a pointer?
>>>>>
>>>>>
>>>>> http://www.spinics.net/lists/linux-rdma/msg31958.html
>>>>
>>>>
>>>> Right, I remember that. Certainly the right direction
>>>>
>>>>>> However, everything under verbs is not straightforward. The files in
>>>>>> userspace are not copies...
>>>>>>
>>>>>> user:
>>>>>>
>>>>>> struct ibv_query_device {
>>>>>>        __u32 command;
>>>>>>        __u16 in_words;
>>>>>>        __u16 out_words;
>>>>>>        __u64 response;
>>>>>>        __u64 driver_data[0];
>>>>>> };
>>>>>>
>>>>>> kernel:
>>>>>>
>>>>>> struct ib_uverbs_query_device {
>>>>>>         __u64 response;
>>>>>>         __u64 driver_data[0];
>>>>>> };
>>>>>
>>>>>
>>>>> We'll obviously need different strutures for the libibvers API
>>>>> and the kernel interface in this case, and we'll need to figure out
>>>>> how to properly translate them.  I think a cast, plus compile time
>>>>> type checking ala BUILD_BUG_ON is the way to go.
>>>>
>>>>
>>>> I'm not sure I follow, which would I cast?
>>>>
>>>> BUILD_BUG_ON(sizeof(ibv_query_device) == sizeof(ib_uverbs_cmd_hdr) +
>>>>              sizeof(ib_uverbs_query_device))
>>>>
>>>> ?
>>>>
>>>>>> I'm thinking the best way forward might be to use a script and
>>>>>> transform userspace into:
>>>>>>
>>>>>> struct ibv_query_device {
>>>>>>   struct ib_uverbs_cmd_hdr hdr;
>>>>>>   struct ib_uverbs_query_device cmd;
>>>>>> };
>>>>>
>>>>>
>>>>> That would break the users of the interface.
>>>>
>>>>
>>>> Sorry, I mean doing this inside rdma-plumbing. Since the change is ABI
>>>> identical the modified libibverbs would still be binary compatible
>>>> with all providers but not source compatible. Since all kernel
>>>> supported providers are in rdma-plumbing we can add the '.cmd.' at the
>>>> same time.
>>>>
>>>> The kernel uapi header would stay the same.
>>>>
>>>>> However automatically generating the user ABI from the kernel one
>>>>> might still be a good idea in the long run.
>>>>
>>>>
>>>> My preference would be to try and use the kernel headers directly.
>>>
>>>
>>> I thought the same, especially after realizing that they are almost
>>> copy/paste from the vendor *-abi.h files.
>>>
>>>>
>>>> Jason
>
>

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

#1484465

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

On Wed, Sep 14, 2016 at 12:36:19PM +0530, Parav Pandit wrote:
> Hi Dennis,
>
> Do you know how would HFI1 driver would work along with rdma cgroup?
>
> Hi Matan, Leon, Jason,
> Apart from HFI1, is there any other concern?
> Or Patch is good to go?

I didn't review it yet :(.
Sorry

>
> 4.8 dates are close by (2 weeks) and there are two git trees involved
> (that might cause merge error to Linus) so if there are no issues, I
> would like to make request to Doug to consider it for 4.8 early on.
>
> Parav
>
> On Mon, Sep 12, 2016 at 10:37 AM, Leon Romanovsky <leon@kernel.org> wrote:
> > On Sun, Sep 11, 2016 at 11:52:35AM -0600, Jason Gunthorpe wrote:
> >> On Sun, Sep 11, 2016 at 07:24:45PM +0200, Christoph Hellwig wrote:
> >> > > > > I've posted some initial work toward a) a while ago, and once we
> >> > >
> >> > > Did it get merged? Do you have a pointer?
> >> >
> >> > http://www.spinics.net/lists/linux-rdma/msg31958.html
> >>
> >> Right, I remember that. Certainly the right direction
> >>
> >> > > However, everything under verbs is not straightforward. The files in
> >> > > userspace are not copies...
> >> > >
> >> > > user:
> >> > >
> >> > > struct ibv_query_device {
> >> > >        __u32 command;
> >> > >        __u16 in_words;
> >> > >        __u16 out_words;
> >> > >        __u64 response;
> >> > >        __u64 driver_data[0];
> >> > > };
> >> > >
> >> > > kernel:
> >> > >
> >> > > struct ib_uverbs_query_device {
> >> > >         __u64 response;
> >> > >         __u64 driver_data[0];
> >> > > };
> >> >
> >> > We'll obviously need different strutures for the libibvers API
> >> > and the kernel interface in this case, and we'll need to figure out
> >> > how to properly translate them.  I think a cast, plus compile time
> >> > type checking ala BUILD_BUG_ON is the way to go.
> >>
> >> I'm not sure I follow, which would I cast?
> >>
> >> BUILD_BUG_ON(sizeof(ibv_query_device) == sizeof(ib_uverbs_cmd_hdr) +
> >>              sizeof(ib_uverbs_query_device))
> >>
> >> ?
> >>
> >> > > I'm thinking the best way forward might be to use a script and
> >> > > transform userspace into:
> >> > >
> >> > > struct ibv_query_device {
> >> > >   struct ib_uverbs_cmd_hdr hdr;
> >> > >   struct ib_uverbs_query_device cmd;
> >> > > };
> >> >
> >> > That would break the users of the interface.
> >>
> >> Sorry, I mean doing this inside rdma-plumbing. Since the change is ABI
> >> identical the modified libibverbs would still be binary compatible
> >> with all providers but not source compatible. Since all kernel
> >> supported providers are in rdma-plumbing we can add the '.cmd.' at the
> >> same time.
> >>
> >> The kernel uapi header would stay the same.
> >>
> >> > However automatically generating the user ABI from the kernel one
> >> > might still be a good idea in the long run.
> >>
> >> My preference would be to try and use the kernel headers directly.
> >
> > I thought the same, especially after realizing that they are almost
> > copy/paste from the vendor *-abi.h files.
> >
> >>
> >> Jason

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

#1487803

Hi Leon,

On Fri, Sep 16, 2016 at 12:26 AM, Leon Romanovsky <leon@kernel.org> wrote:
> On Wed, Sep 14, 2016 at 12:36:19PM +0530, Parav Pandit wrote:
>> Hi Dennis,
>>
>> Do you know how would HFI1 driver would work along with rdma cgroup?
>>
>> Hi Matan, Leon, Jason,
>> Apart from HFI1, is there any other concern?
>> Or Patch is good to go?
>
> I didn't review it yet :(.
> Sorry
>

We have completed review from Tejun, Christoph.
HFI driver folks also provided feedback for Intel drivers.
Matan's also doesn't have any more comments.

If possible, if you can also review, it will be helpful.

I have some more changes unrelated to cgroup in same files in both the git tree.
Pushing them now either results into merge conflict later on for
Doug/Tejun, or requires rebase and resending patch.
If you can review, we can avoid such rework.

>>
>> 4.8 dates are close by (2 weeks) and there are two git trees involved
>> (that might cause merge error to Linus) so if there are no issues, I
>> would like to make request to Doug to consider it for 4.8 early on.
>>
>> Parav
>>
>> On Mon, Sep 12, 2016 at 10:37 AM, Leon Romanovsky <leon@kernel.org> wrote:
>> > On Sun, Sep 11, 2016 at 11:52:35AM -0600, Jason Gunthorpe wrote:
>> >> On Sun, Sep 11, 2016 at 07:24:45PM +0200, Christoph Hellwig wrote:
>> >> > > > > I've posted some initial work toward a) a while ago, and once we
>> >> > >
>> >> > > Did it get merged? Do you have a pointer?
>> >> >
>> >> > http://www.spinics.net/lists/linux-rdma/msg31958.html
>> >>
>> >> Right, I remember that. Certainly the right direction
>> >>
>> >> > > However, everything under verbs is not straightforward. The files in
>> >> > > userspace are not copies...
>> >> > >
>> >> > > user:
>> >> > >
>> >> > > struct ibv_query_device {
>> >> > >        __u32 command;
>> >> > >        __u16 in_words;
>> >> > >        __u16 out_words;
>> >> > >        __u64 response;
>> >> > >        __u64 driver_data[0];
>> >> > > };
>> >> > >
>> >> > > kernel:
>> >> > >
>> >> > > struct ib_uverbs_query_device {
>> >> > >         __u64 response;
>> >> > >         __u64 driver_data[0];
>> >> > > };
>> >> >
>> >> > We'll obviously need different strutures for the libibvers API
>> >> > and the kernel interface in this case, and we'll need to figure out
>> >> > how to properly translate them.  I think a cast, plus compile time
>> >> > type checking ala BUILD_BUG_ON is the way to go.
>> >>
>> >> I'm not sure I follow, which would I cast?
>> >>
>> >> BUILD_BUG_ON(sizeof(ibv_query_device) == sizeof(ib_uverbs_cmd_hdr) +
>> >>              sizeof(ib_uverbs_query_device))
>> >>
>> >> ?
>> >>
>> >> > > I'm thinking the best way forward might be to use a script and
>> >> > > transform userspace into:
>> >> > >
>> >> > > struct ibv_query_device {
>> >> > >   struct ib_uverbs_cmd_hdr hdr;
>> >> > >   struct ib_uverbs_query_device cmd;
>> >> > > };
>> >> >
>> >> > That would break the users of the interface.
>> >>
>> >> Sorry, I mean doing this inside rdma-plumbing. Since the change is ABI
>> >> identical the modified libibverbs would still be binary compatible
>> >> with all providers but not source compatible. Since all kernel
>> >> supported providers are in rdma-plumbing we can add the '.cmd.' at the
>> >> same time.
>> >>
>> >> The kernel uapi header would stay the same.
>> >>
>> >> > However automatically generating the user ABI from the kernel one
>> >> > might still be a good idea in the long run.
>> >>
>> >> My preference would be to try and use the kernel headers directly.
>> >
>> > I thought the same, especially after realizing that they are almost
>> > copy/paste from the vendor *-abi.h files.
>> >
>> >>
>> >> Jason

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

#1488155

Hello, Parav.

On Wed, Sep 21, 2016 at 10:13:38AM +0530, Parav Pandit wrote:
> We have completed review from Tejun, Christoph.
> HFI driver folks also provided feedback for Intel drivers.
> Matan's also doesn't have any more comments.
> 
> If possible, if you can also review, it will be helpful.
> 
> I have some more changes unrelated to cgroup in same files in both the git tree.
> Pushing them now either results into merge conflict later on for
> Doug/Tejun, or requires rebase and resending patch.
> If you can review, we can avoid such rework.

My impression of the thread was that there doesn't seem to be enough
of consensus around how rdma resources should be defined.  Is that
part agreed upon now?

Thanks.

-- 
tejun

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

#1488225

Hi Tejun,

On Wed, Sep 21, 2016 at 7:56 PM, Tejun Heo <tj@kernel.org> wrote:
> Hello, Parav.
>
> On Wed, Sep 21, 2016 at 10:13:38AM +0530, Parav Pandit wrote:
>> We have completed review from Tejun, Christoph.
>> HFI driver folks also provided feedback for Intel drivers.
>> Matan's also doesn't have any more comments.
>>
>> If possible, if you can also review, it will be helpful.
>>
>> I have some more changes unrelated to cgroup in same files in both the git tree.
>> Pushing them now either results into merge conflict later on for
>> Doug/Tejun, or requires rebase and resending patch.
>> If you can review, we can avoid such rework.
>
> My impression of the thread was that there doesn't seem to be enough
> of consensus around how rdma resources should be defined.  Is that
> part agreed upon now?
>

We ended up discussing few points on different thread [1].

There was confusion on how some non-rdma/non-IB drivers would work
with rdma cgroup from Matan.
Christoph explained how they don't fit in the rdma subsystem and
therefore its not prime target to addess.
Intel driver maintainer Denny also acknowledged same on [2].
IB compliant drivers of Intel support rdma cgroup as explained in [2].
With that usnic and Intel psm drivers falls out of rdma cgroup support
as they don't fit very well in the verbs definition.

[1] https://www.spinics.net/lists/linux-rdma/msg40340.html
[2] http://www.spinics.net/lists/linux-rdma/msg40717.html

I will wait for Leon's review comments if he has different view on architecture.
Back in April when I met face-to-face to Leon and Haggai, Leon was in
support to have kernel defined the rdma resources as suggested by
Christoph and Tejun instead of IB/RDMA subsystem.
I will wait for his comments if his views have changed with new uAPI
taking shape.

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

#1486515

On Wed, 2016-09-14 at 12:36 +0530, Parav Pandit wrote:
> Hi Dennis,
> 
> Do you know how would HFI1 driver would work along with rdma cgroup?

Keep in mind HFI1 driver has two "modes" of operation. We support
verbs, and would surely fall in line with whatever cgroups do for IB
core. For our psm interface, not sure how cgroups would come into play.
Psm is designed to expose the hw to user and avoid the kernel when
possible adding more kernel control is sort of contrary to that.

Now that being said, Christoph recently made mention of maybe having a
drivers/psm [1]. I really haven't had a chance to think about the
implications of that, but maybe it's worth considering, after all we
have two implementations, qib and hfi1. So anyway I'm not sure we need
to be too concerned about cgroups right now as far as psm side of
things goes.

Depending how things shake out for the uAPI rewrite, or verbs 2.0 or
whatever we are calling it today things may change.

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

-Denny

> Hi Matan, Leon, Jason,
> Apart from HFI1, is there any other concern?
> Or Patch is good to go?
> 
> 4.8 dates are close by (2 weeks) and there are two git trees involved
> (that might cause merge error to Linus) so if there are no issues, I
> would like to make request to Doug to consider it for 4.8 early on.
> 
> Parav
> 
> On Mon, Sep 12, 2016 at 10:37 AM, Leon Romanovsky <leon@kernel.org>
> wrote:
> > On Sun, Sep 11, 2016 at 11:52:35AM -0600, Jason Gunthorpe wrote:
> > > On Sun, Sep 11, 2016 at 07:24:45PM +0200, Christoph Hellwig
> > > wrote:
> > > > > > > I've posted some initial work toward a) a while ago, and
> > > > > > > once we
> > > > > 
> > > > > Did it get merged? Do you have a pointer?
> > > > 
> > > > http://www.spinics.net/lists/linux-rdma/msg31958.html
> > > 
> > > Right, I remember that. Certainly the right direction
> > > 
> > > > > However, everything under verbs is not straightforward. The
> > > > > files in
> > > > > userspace are not copies...
> > > > > 
> > > > > user:
> > > > > 
> > > > > struct ibv_query_device {
> > > > >        __u32 command;
> > > > >        __u16 in_words;
> > > > >        __u16 out_words;
> > > > >        __u64 response;
> > > > >        __u64 driver_data[0];
> > > > > };
> > > > > 
> > > > > kernel:
> > > > > 
> > > > > struct ib_uverbs_query_device {
> > > > >         __u64 response;
> > > > >         __u64 driver_data[0];
> > > > > };
> > > > 
> > > > We'll obviously need different strutures for the libibvers API
> > > > and the kernel interface in this case, and we'll need to figure
> > > > out
> > > > how to properly translate them.  I think a cast, plus compile
> > > > time
> > > > type checking ala BUILD_BUG_ON is the way to go.
> > > 
> > > I'm not sure I follow, which would I cast?
> > > 
> > > BUILD_BUG_ON(sizeof(ibv_query_device) ==
> > > sizeof(ib_uverbs_cmd_hdr) +
> > >              sizeof(ib_uverbs_query_device))
> > > 
> > > ?
> > > 
> > > > > I'm thinking the best way forward might be to use a script
> > > > > and
> > > > > transform userspace into:
> > > > > 
> > > > > struct ibv_query_device {
> > > > >   struct ib_uverbs_cmd_hdr hdr;
> > > > >   struct ib_uverbs_query_device cmd;
> > > > > };
> > > > 
> > > > That would break the users of the interface.
> > > 
> > > Sorry, I mean doing this inside rdma-plumbing. Since the change
> > > is ABI
> > > identical the modified libibverbs would still be binary
> > > compatible
> > > with all providers but not source compatible. Since all kernel
> > > supported providers are in rdma-plumbing we can add the '.cmd.'
> > > at the
> > > same time.
> > > 
> > > The kernel uapi header would stay the same.
> > > 
> > > > However automatically generating the user ABI from the kernel
> > > > one
> > > > might still be a good idea in the long run.
> > > 
> > > My preference would be to try and use the kernel headers
> > > directly.
> > 
> > I thought the same, especially after realizing that they are almost
> > copy/paste from the vendor *-abi.h files.
> > 
> > > 
> > > Jason
> --
> 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]

#1486663

Hi Denny,

On Mon, Sep 19, 2016 at 6:40 PM, Dalessandro, Dennis
<dennis.dalessandro@intel.com> wrote:
> On Wed, 2016-09-14 at 12:36 +0530, Parav Pandit wrote:
>> Hi Dennis,
>>
>> Do you know how would HFI1 driver would work along with rdma cgroup?
>
> Keep in mind HFI1 driver has two "modes" of operation. We support
> verbs, and would surely fall in line with whatever cgroups do for IB
> core.
Thanks for the feedback.

> For our psm interface, not sure how cgroups would come into play.
> Psm is designed to expose the hw to user and avoid the kernel when
> possible adding more kernel control is sort of contrary to that.
>
Yes, PSM is currently out of RDMA cgroup and in future we can take a
look on how things shape as subsystem if it does.

> Now that being said, Christoph recently made mention of maybe having a
> drivers/psm [1]. I really haven't had a chance to think about the
> implications of that, but maybe it's worth considering, after all we
> have two implementations, qib and hfi1. So anyway I'm not sure we need
> to be too concerned about cgroups right now as far as psm side of
> things goes.
>
o.k.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web