Re: [PATCH v2 0/5] Support for generalized use of make C={1,2} via a wrapper program

[Leon Romanovsky]

On Mon, Dec 18, 2017 at 07:39:50PM +0100, Knut Omang wrote:
> On Mon, 2017-12-18 at 17:56 +, Bart Van Assche wrote:
> > On Mon, 2017-12-18 at 10:46 -0700, Jason Gunthorpe wrote:
> > > On Sun, Dec 17, 2017 at 10:00:17PM -0800, Joe Perches wrote:
> > >
> > > > > Today when we run checkers we get so many warnings it is too hard to
> > > > > make any sense of it.
> > > >
> > > > Here is a list of the checkpatch messages for drivers/infiniband
> > > > sorted by type.
> > > >
> > > > Many of these might be corrected by using
> > > >
> > > > $ ./scripts/checkpatch.pl -f --fix-inplace --types= \
> > > >   $(git ls-files drivers/infiniband/)
> > >
> > > How many of these do you think it is worth to fix?
> > >
> > > We do get a steady trickle of changes in this topic every cycle.
> > >
> > > Is it better to just do a big number of them all at once? Do you have
> > > an idea how disruptive this kind of work is to the whole patch flow
> > > eg new patches no longer applying to for-next, backports no longer
> > > applying, merge conflicts?
> >
> > In my opinion patches that only change the coding style and do not change 
> > any
> > functionality are annoying. Before posting a patch that fixes a bug the 
> > change
> > history (git log -p) has to be cheched to figure out which patch introduced
> > the bug. Patches that only change coding style pollute the change history.
>
> I agree with you - the problem is that style issues should not have existed.
> But when they do it becomes a problem to remove them and a problem to
> keep them - for instance us who try to be compliant by having style helpers
> in our editor, we end up having to manually revert old style mistakes back in
> to avoid making unrelated whitespace changes or similar.

If the checkpatch.pl complains about coding style for the new patch in
newly added code, I'm asking from the author to prepare cleanup patch so
it will be applied before actual patch.

In case, complains are for code which patch are not touching, I'm
submitting it as is.

Thanks


signature.asc
Description: PGP signature

Re: [PATCHv14 0/3] rdmacg: IB/core: rdma controller support

[Leon Romanovsky]

On Tue, Jan 10, 2017 at 11:15:05AM -0500, Tejun Heo wrote:
> On Tue, Jan 10, 2017 at 12:02:12AM +, Parav Pandit wrote:
> > Patchset is compiled and tested against below Tejun's cgroup tree
> > using cgroup v1 and v2 mode.
> > URL: git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git
> > Branch: for-4.11
>
> Applied 1-3 to cgroup/for-4.11-rdmacg.

Thanks Tejun and Parav.

>
> Thanks.
>
> --
> tejun


signature.asc
Description: PGP signature

Re: [PATCHv12 0/3] rdmacg: IB/core: rdma controller support

[Leon Romanovsky]

On Wed, Aug 31, 2016 at 02:07:24PM +0530, Parav Pandit wrote:
> rdmacg: IB/core: rdma controller support
>
> Patch is generated and tested against below Doug's linux-rdma
> git tree.
>
> URL: git://git.kernel.org/pub/scm/linux/kernel/git/dledford/rdma.git
> Branch: master
>
> Patchset is also compiled and tested against below Tejun's cgroup tree
> using cgroup v2 mode.
> URL: git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git
> Branch: master
>
> Overview:
> Currently user space applications can easily take away all the rdma
> device specific resources such as AH, CQ, QP, MR etc. Due to which other
> applications in other cgroup or kernel space ULPs may not even get chance
> to allocate any rdma resources. This results into service unavailibility.
>
> RDMA cgroup addresses this issue by allowing resource accounting,
> limit enforcement on per cgroup, per rdma device basis.
>
> RDMA uverbs layer will enforce limits on well defined RDMA verb
> resources without any HCA vendor device driver involvement.
>
> RDMA uverbs layer will not do limit enforcement of HCA hw vendor
> specific resources. Instead rdma cgroup provides set of APIs
> through which vendor specific drivers can do resource accounting
> by making use of rdma cgroup.

Hi Parav,
I want to propose an extension to the RDMA cgroup which can be done as
follow-up patches.

Let's add new global type, which will control whole HCA (for example in 
percentages). It will
allow natively define new objects without need to introduce them to the user.

This HCA share will be overwritten by specific UVERBS types which you
already defined.

What do you think?

Except this proposal,
Reviewed-by: Leon Romanovsky <leo...@mellanox.com>

Thanks.


signature.asc
Description: PGP signature

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

[Leon Romanovsky]

On Wed, Oct 05, 2016 at 08:37:35AM +0200, Christoph Hellwig wrote:
> FYI, the patches look fine to me:
>
> Acked-by: Christoph Hellwig 
>
> but we're past the merge window for 4.9 now unfortunately.

IMHO, it still can make it.

Thanks


signature.asc
Description: PGP signature

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

[Leon Romanovsky]

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 <l...@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


signature.asc
Description: PGP signature

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

[Leon Romanovsky]

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


signature.asc
Description: PGP signature

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

[Leon Romanovsky]

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_cqibv_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 majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

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

[Leon Romanovsky]

On Wed, Sep 07, 2016 at 08:37:23PM +0530, Parav Pandit wrote:
> Hi Leon,
>
> >> Signed-off-by: Parav Pandit 
> >> +static struct rdmacg_resource_pool *
> >> +get_cg_rpool_locked(struct rdma_cgroup *cg, struct rdmacg_device *device)
> >> +{
> >> + struct rdmacg_resource_pool *rpool;
> >> +
> >> + rpool = find_cg_rpool_locked(cg, device);
> >> + if (rpool)
> >> + return rpool;
> >> +
> >> + rpool = kzalloc(sizeof(*rpool), GFP_KERNEL);
> >> + if (!rpool)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + rpool->device = device;
> >> + set_all_resource_max_limit(rpool);
> >> +
> >> + INIT_LIST_HEAD(>cg_node);
> >> + INIT_LIST_HEAD(>dev_node);
> >> + list_add_tail(>cg_node, >rpools);
> >> + list_add_tail(>dev_node, >rpools);
> >> + return rpool;
> >> +}
> >
> > <...>
> >
> >> + for (p = cg; p; p = parent_rdmacg(p)) {
> >> + rpool = get_cg_rpool_locked(p, device);
> >> + if (IS_ERR_OR_NULL(rpool)) {
> >
> > get_cg_rpool_locked always returns !NULL (error, or pointer)
>
> Can this change go as incremental change after this patch, since this
> patch is close to completion?
> Or I need to revise v13?

Sure, it is cleanup. It is not worth of respinning.

>
> >
> >> + ret = PTR_ERR(rpool);
> >> + goto err;
> >
> > I didn't review the whole series yet.
>
> Did you get a chance to review the series?

We need to decide on fundamental question before reviewing it, which is
"how to fit rdmacg to new ABI model".

Thanks


signature.asc
Description: PGP signature

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

[Leon Romanovsky]

On Wed, Aug 31, 2016 at 02:07:25PM +0530, Parav Pandit wrote:
> Added rdma cgroup controller that does accounting, limit enforcement
> on rdma/IB verbs and hw resources.
>
> Added rdma cgroup header file which defines its APIs to perform
> charing/uncharing functionality. It also defined APIs for RDMA/IB
> stack for device registration. Devices which are registered will
> participate in controller functions of accounting and limit
> enforcements. It define rdmacg_device structure to bind IB stack
> and RDMA cgroup controller.
>
> RDMA resources are tracked using resource pool. Resource pool is per
> device, per cgroup entity which allows setting up accounting limits
> on per device basis.
>
> Currently resources are defined by the RDMA cgroup.
>
> Resource pool is created/destroyed dynamically whenever
> charging/uncharging occurs respectively and whenever user
> configuration is done. Its a tradeoff of memory vs little more code
> space that creates resource pool object whenever necessary, instead of
> creating them during cgroup creation and device registration time.
>
> Signed-off-by: Parav Pandit 
> ---

<...>

> +
> +static struct rdmacg_resource_pool *
> +get_cg_rpool_locked(struct rdma_cgroup *cg, struct rdmacg_device *device)
> +{
> + struct rdmacg_resource_pool *rpool;
> +
> + rpool = find_cg_rpool_locked(cg, device);
> + if (rpool)
> + return rpool;
> +
> + rpool = kzalloc(sizeof(*rpool), GFP_KERNEL);
> + if (!rpool)
> + return ERR_PTR(-ENOMEM);
> +
> + rpool->device = device;
> + set_all_resource_max_limit(rpool);
> +
> + INIT_LIST_HEAD(>cg_node);
> + INIT_LIST_HEAD(>dev_node);
> + list_add_tail(>cg_node, >rpools);
> + list_add_tail(>dev_node, >rpools);
> + return rpool;
> +}

<...>

> + for (p = cg; p; p = parent_rdmacg(p)) {
> + rpool = get_cg_rpool_locked(p, device);
> + if (IS_ERR_OR_NULL(rpool)) {

get_cg_rpool_locked always returns !NULL (error, or pointer)

> + ret = PTR_ERR(rpool);
> + goto err;

I didn't review the whole series yet.


signature.asc
Description: PGP signature

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

[Leon Romanovsky]

On Tue, Apr 05, 2016 at 05:55:26AM -0700, Parav Pandit wrote:
> Hi Christoph,
> 
> On Tue, Apr 5, 2016 at 5:42 AM, Christoph Hellwig  wrote:
> > On Tue, Apr 05, 2016 at 05:39:21AM -0700, Parav Pandit wrote:
> >> I am not really trying to address OFED issues here. I am sure you
> >> understand that if ib_core.ko kernel module is in-kernel module than,
> >> for all the fixes/enhancements that goes to it would require people to
> >> upgrade to newer kernel, instead of just modules upgrade. Such heavy
> >> weight upgrade slows down the adoption which i am trying to avoid here
> >> by placing knobs in right kernel module's hand.
> >
> > What a load of rubbish.  The Linux kernel is one program and should be
> > upgraded together.
> 
> Just because we add one more rdma resource, we need to ask someone to
> upgrade kernel?

It doesn't make sense. Kernel and modules are always coming together,
the attempts to mix kernel and modules from different versions can lead
to many interesting debug scenarios.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

On Sat, Feb 20, 2016 at 04:30:04PM +0530, Parav Pandit wrote:
> Added rdma cgroup controller that does accounting, limit enforcement
> on rdma/IB verbs and hw resources.
> 
> Added rdma cgroup header file which defines its APIs to perform
> charing/uncharing functionality and device registration which will
> participate in controller functions of accounting and limit
> enforcements. It also define rdmacg_device structure to bind IB stack
> and RDMA cgroup controller.
> 
> RDMA resources are tracked using resource pool. Resource pool is per
> device, per cgroup entity which allows setting up accounting limits
> on per device basis.
> 
> Resources are not defined by the RDMA cgroup, instead they are defined
> by the external module IB stack. This allows extending IB stack
> without changing kernel, as IB stack is going through changes
> and enhancements.
> 
> Resource pool is created/destroyed dynamically whenever
> charging/uncharging occurs respectively and whenever user
> configuration is done. Its a tradeoff of memory vs little more code
> space that creates resource pool whenever necessary,
> instead of creating them during cgroup creation and device registration
> time.
> 
> Signed-off-by: Parav Pandit 
> ---
>  include/linux/cgroup_rdma.h   |  53 +++
>  include/linux/cgroup_subsys.h |   4 +
>  init/Kconfig  |  10 +
>  kernel/Makefile   |   1 +
>  kernel/cgroup_rdma.c  | 753 
> ++
>  5 files changed, 821 insertions(+)
>  create mode 100644 include/linux/cgroup_rdma.h
>  create mode 100644 kernel/cgroup_rdma.c
> 
> diff --git a/include/linux/cgroup_rdma.h b/include/linux/cgroup_rdma.h
> new file mode 100644
> index 000..b370733
> --- /dev/null
> +++ b/include/linux/cgroup_rdma.h
> @@ -0,0 +1,53 @@
> +#ifndef _CGROUP_RDMA_H
> +#define _CGROUP_RDMA_H
> +
> +#include 
> +
> +struct rdma_cgroup {
> +#ifdef CONFIG_CGROUP_RDMA
> + struct cgroup_subsys_state  css;
> +
> + spinlock_t rpool_list_lock; /* protects resource pool list */
> + struct list_head rpool_head;/* head to keep track of all resource
> +  * pools that belongs to this cgroup.
> +  */
> +#endif
> +};
> +
> +#ifdef CONFIG_CGROUP_RDMA

I'm sure that you already asked about that, but why do you need ifdef
embedded in struct rdma_cgroup and right after that the same one?
Can you place this ifdef before declaring struct rdma_cgroup?

> +
> +struct rdmacg_device;
> +

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Leon Romanovsky]

On Sun, Feb 21, 2016 at 07:41:08PM +0530, Parav Pandit wrote:
> CONFIG_CGROUP_RDMA
> 
> On Sun, Feb 21, 2016 at 7:15 PM, Leon Romanovsky <l...@leon.nu> wrote:
> > On Sun, Feb 21, 2016 at 05:03:05PM +0530, Parav Pandit wrote:
> >> On Sun, Feb 21, 2016 at 1:13 PM, Leon Romanovsky <l...@leon.nu> wrote:
> >> > On Sat, Feb 20, 2016 at 04:30:04PM +0530, Parav Pandit wrote:
> >> > Can you place this ifdef before declaring struct rdma_cgroup?
> >> Yes. I missed out this cleanup. Done locally now.
> >
> > Great, additional thing which spotted my attention was related to
> > declaring and using the new cgroups functions. There are number of
> > places where you protected the calls by specific ifdefs in the
> > IB/core c-files and not in h-files as it is usually done.
> >
> ib_device_register_rdmacg, ib_device_unregister_rdmacg are the only
> two functions called from IB/core as its tied to functionality.
> They can also be implemented as NULL call when CONFIG_CGROUP_RDMA is 
> undefined.
> (Similar to ib_rdmacg_try_charge and others).
> I didn't do because occurrence of call of register and unregister is
> limited to single file and only twice compare to charge/uncharge
> functions.
> Either way is fine with me, I can make the changes which you
> described. Let me know.

Please do,
IMHO, it is better to have one place which handles all relevant ifdefs
and functions. IB/core doesn't need to know about cgroups implementation.

Thanks
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html