* I Hope You Get My Message This Time
From: Friedrich Mayrhofer @ 2016-10-08 2:13 UTC (permalink / raw)
This is the second time i am sending you this mail.
I, Friedrich Mayrhofer Donate $ 1,000,000.00 to You, Email Me personally
for more details.
Regards.
Friedrich Mayrhofer
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: iscsi_trx going into D state
From: Zhu Lingshan @ 2016-10-08 2:59 UTC (permalink / raw)
To: Robert LeBlanc, linux-rdma; +Cc: linux-scsi
In-Reply-To: <CAANLjFoj9-qscJOSf2jtKYt2+4cQxMHNJ9q2QTey4wyG5OTSAA@mail.gmail.com>
Hi Robert,
I also see this issue, but this is not the only code path can trigger
this problem, I think you may also see iscsi_np in D status. I fixed one
code path whitch still not merged to mainline. I will forward you my
patch later. Note: my patch only fixed one code path, you may see other
call statck with D status.
Thanks,
BR
Zhu Lingshan
在 2016/10/1 1:14, Robert LeBlanc 写道:
> We are having a reoccurring problem where iscsi_trx is going into D
> state. It seems like it is waiting for a session tear down to happen
> or something, but keeps waiting. We have to reboot these targets on
> occasion. This is running the 4.4.12 kernel and we have seen it on
> several previous 4.4.x and 4.2.x kernels. There is no message in dmesg
> or /var/log/messages. This seems to happen with increased frequency
> when we have a disruption in our Infiniband fabric, but can happen
> without any changes to the fabric (other than hosts rebooting).
>
> # ps aux | grep iscsi | grep D
> root 4185 0.0 0.0 0 0 ? D Sep29 0:00 [iscsi_trx]
> root 18505 0.0 0.0 0 0 ? D Sep29 0:00 [iscsi_np]
>
> # cat /proc/4185/stack
> [<ffffffff814cc999>] target_wait_for_sess_cmds+0x49/0x1a0
> [<ffffffffa087292b>] isert_wait_conn+0x1ab/0x2f0 [ib_isert]
> [<ffffffff814f0de2>] iscsit_close_connection+0x162/0x840
> [<ffffffff814df8df>] iscsit_take_action_for_connection_exit+0x7f/0x100
> [<ffffffff814effc0>] iscsi_target_rx_thread+0x5a0/0xe80
> [<ffffffff8109c6f8>] kthread+0xd8/0xf0
> [<ffffffff8172004f>] ret_from_fork+0x3f/0x70
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> # cat /proc/18505/stack
> [<ffffffff814f0c71>] iscsit_stop_session+0x1b1/0x1c0
> [<ffffffff814e2436>] iscsi_check_for_session_reinstatement+0x1e6/0x270
> [<ffffffff814e4df0>] iscsi_target_check_for_existing_instances+0x30/0x40
> [<ffffffff814e4f40>] iscsi_target_do_login+0x140/0x640
> [<ffffffff814e62dc>] iscsi_target_start_negotiation+0x1c/0xb0
> [<ffffffff814e402b>] iscsi_target_login_thread+0xa9b/0xfc0
> [<ffffffff8109c6f8>] kthread+0xd8/0xf0
> [<ffffffff8172004f>] ret_from_fork+0x3f/0x70
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> What can we do to help get this resolved?
>
> Thanks,
>
> ----------------
> Robert LeBlanc
> PGP Fingerprint 79A2 9CA4 6CC4 45DD A904 C70E E654 3BB2 FA62 B9F1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: Introduction of libqedr to the Consolidated Userspace RDMA Library Repo
From: Amrani, Ram @ 2016-10-08 12:49 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Elior, Ariel,
Kalderon, Michal, Borundia, Rajesh,
dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
In-Reply-To: <20161006163427.GG1224-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
>If you prepare a branch of rdma-core on github I can give it a quick
>look over.
>
>The technical steps are fairly simple, create a
>provider/qedr directory, add your source code, copy the CMakeLists.txt
>style from another provider, add a line to the sorted list in the top
>level CMakeLists.txt, update README.md, COPYING.md and MAINTAINERS
>
>You will want to look at the various patches I've prepared and ensure
>you cover off the basic cleanups that have already been done, and that
>your code compiles warning-free on FC24.
>
>Once you feel everything is ready then post it to the mailing list and
>send a pull request. For the mailing list you can just split the
>patches by file..
>
>Jason
Thanks, we'll start on it.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH qedr 04/10] qedr: Add support for PD,PKEY and CQ verbs
From: Doug Ledford @ 2016-10-08 13:35 UTC (permalink / raw)
To: Leon Romanovsky, Elior, Ariel
Cc: Kalderon, Michal, Mintz, Yuval, Borundia, Rajesh,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Amrani, Ram
In-Reply-To: <20161007142454.GU9282-2ukJVAZIZ/Y@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 2070 bytes --]
On 10/7/2016 10:24 AM, Leon Romanovsky wrote:
> On Fri, Oct 07, 2016 at 10:47:18AM +0000, Elior, Ariel wrote:
>>>> @@ -66,6 +85,12 @@ struct rdma_cqe_common {
>>>> struct regpair qp_handle;
>>>> __le16 reserved1[7];
>>>> u8 flags;
>>>> +#define RDMA_CQE_COMMON_TOGGLE_BIT_MASK 0x1
>>>> +#define RDMA_CQE_COMMON_TOGGLE_BIT_SHIFT 0
>>>> +#define RDMA_CQE_COMMON_TYPE_MASK 0x3
>>>> +#define RDMA_CQE_COMMON_TYPE_SHIFT 1
>>>> +#define RDMA_CQE_COMMON_RESERVED2_MASK 0x1F
>>>> +#define RDMA_CQE_COMMON_RESERVED2_SHIFT 3
>>>> u8 status;
>>>
>>> It is VERY uncommon to mix defines and structs together.
>>> Please don't do it, it confuses a lot and doesn't help to
>>> readability/debug.
>>
>> Hi Leon,
>> Firstly, thanks for investing your time in reviewing our driver.
>> As for mixed defines and structures, far from being very uncommon, they are
>> actually ubiquitous throughout the kernel and are used by the foremost
>> developers (Dave Miller, Linus, Jeff Kirsher).
>
> Net subsystem is very different from other kernel community.
> For example, this article from LWN [1] - "Coding-style exceptionalism"
> talks about it.
> [1] https://lwn.net/Articles/694755/
That article only refers to multi-line comments, not to embedding
#defines inside of structs that the #defines are used with.
My personal taste on things like this is that if you had something like
a variable with a result code, then use a separate enum for the possible
options. However, in this case, you have a multi-mask item and the
defines are the three masks and their shifts. I'm OK with that being
mixed in or being separate, but if it's separate, I would want it
immediately before the struct with a comment specifying that this is the
format of the status byte in the struct. What I wouldn't want is the
#defines moved far away from the struct with a bunch of other defines
where the context of the struct is lost.
--
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
GPG Key ID: 0E572FDD
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]
^ permalink raw reply
* Re: [PATCH qedr 04/10] qedr: Add support for PD,PKEY and CQ verbs
From: Leon Romanovsky @ 2016-10-08 15:39 UTC (permalink / raw)
To: Doug Ledford
Cc: Elior, Ariel, Kalderon, Michal, Mintz, Yuval, Borundia, Rajesh,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Amrani, Ram
In-Reply-To: <a146f562-1970-e939-1bd5-074af868f7d6-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2554 bytes --]
On Sat, Oct 08, 2016 at 09:35:06AM -0400, Doug Ledford wrote:
> On 10/7/2016 10:24 AM, Leon Romanovsky wrote:
> > On Fri, Oct 07, 2016 at 10:47:18AM +0000, Elior, Ariel wrote:
> >>>> @@ -66,6 +85,12 @@ struct rdma_cqe_common {
> >>>> struct regpair qp_handle;
> >>>> __le16 reserved1[7];
> >>>> u8 flags;
> >>>> +#define RDMA_CQE_COMMON_TOGGLE_BIT_MASK 0x1
> >>>> +#define RDMA_CQE_COMMON_TOGGLE_BIT_SHIFT 0
> >>>> +#define RDMA_CQE_COMMON_TYPE_MASK 0x3
> >>>> +#define RDMA_CQE_COMMON_TYPE_SHIFT 1
> >>>> +#define RDMA_CQE_COMMON_RESERVED2_MASK 0x1F
> >>>> +#define RDMA_CQE_COMMON_RESERVED2_SHIFT 3
> >>>> u8 status;
> >>>
> >>> It is VERY uncommon to mix defines and structs together.
> >>> Please don't do it, it confuses a lot and doesn't help to
> >>> readability/debug.
> >>
> >> Hi Leon,
> >> Firstly, thanks for investing your time in reviewing our driver.
> >> As for mixed defines and structures, far from being very uncommon, they are
> >> actually ubiquitous throughout the kernel and are used by the foremost
> >> developers (Dave Miller, Linus, Jeff Kirsher).
> >
> > Net subsystem is very different from other kernel community.
> > For example, this article from LWN [1] - "Coding-style exceptionalism"
> > talks about it.
>
> > [1] https://lwn.net/Articles/694755/
>
> That article only refers to multi-line comments, not to embedding
> #defines inside of structs that the #defines are used with.
That article supports my claim that net subsystem is different from the
rest of the kernel.
>
> My personal taste on things like this is that if you had something like
> a variable with a result code, then use a separate enum for the possible
> options. However, in this case, you have a multi-mask item and the
> defines are the three masks and their shifts. I'm OK with that being
> mixed in or being separate, but if it's separate, I would want it
> immediately before the struct with a comment specifying that this is the
> format of the status byte in the struct. What I wouldn't want is the
> #defines moved far away from the struct with a bunch of other defines
> where the context of the struct is lost.
It looks like you are neutral on the topic, and I'm against mixing these
specific defines with structures. Every change in such define changes
struct as well which can be easily missed out.
Ram,
Please invest an extra effort and help the reviewers to accomplish their
task.
Thanks
>
> --
> Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> GPG Key ID: 0E572FDD
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH] i40iw: Add Quality of Service support
From: Leon Romanovsky @ 2016-10-08 15:41 UTC (permalink / raw)
To: Henry Orosco
Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
e1000-rdma-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
In-Reply-To: <20161006212659.GB6664-ZmvEvTIhtuUfyugFOqMDN/ooFf0ArEBIu+b9c/7xato@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2323 bytes --]
On Thu, Oct 06, 2016 at 04:26:59PM -0500, Henry Orosco wrote:
> On Thu, Oct 06, 2016 at 07:00:16PM +0300, Leon Romanovsky wrote:
> > On Thu, Oct 06, 2016 at 10:21:28AM -0500, Henry Orosco wrote:
> > > Add support for QoS on QPs. Upon device initialization,
> > > a map is created from user priority to queue set
> > > handles. On QP creation, use ToS to look up the queue
> > > set handle for use with the QP.
> > >
> > > Signed-off-by: Faisal Latif <faisal.latif-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Shiraz Saleem <shiraz.saleem-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > Signed-off-by: Henry Orosco <henry.orosco-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > ---
> > > drivers/infiniband/hw/i40iw/i40iw.h | 9 ++
> > > drivers/infiniband/hw/i40iw/i40iw_cm.c | 30 +++++-
> > > drivers/infiniband/hw/i40iw/i40iw_cm.h | 2 +-
> > > drivers/infiniband/hw/i40iw/i40iw_ctrl.c | 151 +++++++++++++++++++++++++++++-
> > > drivers/infiniband/hw/i40iw/i40iw_d.h | 2 +
> > > drivers/infiniband/hw/i40iw/i40iw_hw.c | 25 ++---
> > > drivers/infiniband/hw/i40iw/i40iw_main.c | 64 +++++++++++--
> > > drivers/infiniband/hw/i40iw/i40iw_osdep.h | 2 +
> > > drivers/infiniband/hw/i40iw/i40iw_p.h | 2 +
> > > drivers/infiniband/hw/i40iw/i40iw_puda.c | 3 +-
> > > drivers/infiniband/hw/i40iw/i40iw_type.h | 18 +++-
> > > drivers/infiniband/hw/i40iw/i40iw_utils.c | 45 +++++++++
> > > drivers/infiniband/hw/i40iw/i40iw_verbs.c | 6 +-
> > > 13 files changed, 323 insertions(+), 36 deletions(-)
> >
> > I believe that it can be split to series.
>
> As all this code is a logical unit for QoS support, we thought
> it best to submit as a single patch.
Large logical unit --> patchset
~400 lines of code --> large unit
>
> Doug, what is your preference?
> >
> > > mode change 100644 => 100755 drivers/infiniband/hw/i40iw/i40iw_cm.c
> > > mode change 100644 => 100755 drivers/infiniband/hw/i40iw/i40iw_ctrl.c
> >
> > Why did you change file's mode from 644 to 755?
>
> Nice catch! Will send out another version with this fixed.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* RE: [PATCH qedr 04/10] qedr: Add support for PD,PKEY and CQ verbs
From: Elior, Ariel @ 2016-10-08 23:20 UTC (permalink / raw)
To: Leon Romanovsky, dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org
Cc: Mintz, Yuval, Amrani, Ram, Kalderon, Michal, Borundia, Rajesh,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20161008153942.GY9282-2ukJVAZIZ/Y@public.gmane.org>
> > My personal taste on things like this is that if you had something like
> > a variable with a result code, then use a separate enum for the possible
> > options. However, in this case, you have a multi-mask item and the
> > defines are the three masks and their shifts. I'm OK with that being
> > mixed in or being separate, but if it's separate, I would want it
> > immediately before the struct with a comment specifying that this is the
> > format of the status byte in the struct. What I wouldn't want is the
> > #defines moved far away from the struct with a bunch of other defines
> > where the context of the struct is lost.
>
> It looks like you are neutral on the topic, and I'm against mixing these
> specific defines with structures. Every change in such define changes
> struct as well which can be easily missed out.
>
Leon, I don't think that is reasonable. I can accept that refactoring all of
our code so that defines end up outside the structures would make it
easier for you personally to review, but I don't agree there is any general
improved readability. Quite the opposite. The whole purpose of storing
the defines adjacent to their fields is so you can easily associate them
together. The reason that this style is used by so many people is *for*
improved debuggability and readability. This style is prevalent throughout
the kernel. Even if you think net is a bad example (while I think it is an
excellent example), you can find examples in every corner of the kernel
so this is not "a net thing".
Here are just a few examples outside of net (there are thousands):
fs/cachefiles/internal.h line 86
block/partitions/acorn.c line 69
crypto/jitterentropy.c line line 78
kernel/sched/sched.h lines 594 and 1250
drivers/block/drbd/drbd_int.h line 996
drivers/gpu/drm/gma500/psb_intel_drv.h line 128
As Doug is comfortable with this style, we are going to leave it as is.
Try thinking of our entrance to linux-rdma as an opportunity to cross-
pollinate and bring over some new techniques and new people into
the subsystem.
Thanks,
Ariel
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH rdma-core 1/5] Pull uninitialized_var into util/compiler.h
From: Leon Romanovsky @ 2016-10-09 5:46 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1475879772-29458-2-git-send-email-jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 5750 bytes --]
On Fri, Oct 07, 2016 at 04:36:08PM -0600, Jason Gunthorpe wrote:
> The new common definition turns it off for new compilers since
> it is not needed and is too easy to abuse.
Jason,
I have two comments.
1. Can we remove this uninitialized_var at all and simply replace by
default values?
We have 5 places like this:
➜ rdma-core git:(master) grep -r uninitialized_var */src/*.c
libcxgb4/src/cq.c: struct t4_cqe uninitialized_var(cqe), *rd_cqe;
libcxgb4/src/qp.c: u8 uninitialized_var(len16);
libmlx4/src/qp.c: struct mlx4_wqe_ctrl_seg *uninitialized_var(ctrl);
libmlx5/src/qp.c: uint32_t *uninitialized_var(p);
libmlx5/src/qp.c: int uninitialized_var(sz);
2. Do we really want additional folder to ccan with such common headers?
What about rename ccan to be util and put this new file there (in case
you proceed with it)?
Thanks
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> ---
> CMakeLists.txt | 1 +
> providers/cxgb4/cq.c | 1 +
> providers/cxgb4/libcxgb4.h | 2 --
> providers/cxgb4/qp.c | 1 +
> providers/mlx4/mlx4.h | 4 ----
> providers/mlx4/qp.c | 1 +
> providers/mlx5/mlx5.h | 4 ----
> providers/mlx5/qp.c | 1 +
> util/CMakeLists.txt | 3 +++
> util/compiler.h | 18 ++++++++++++++++++
> 10 files changed, 26 insertions(+), 10 deletions(-)
> create mode 100644 util/CMakeLists.txt
> create mode 100644 util/compiler.h
>
> diff --git a/CMakeLists.txt b/CMakeLists.txt
> index b0864da660fc..1611e90d7933 100644
> --- a/CMakeLists.txt
> +++ b/CMakeLists.txt
> @@ -253,6 +253,7 @@ configure_file("${BUILDLIB}/config.h.in" "${BUILD_INCLUDE}/config.h" ESCAPE_QUOT
> #-------------------------
> # Sub-directories
> add_subdirectory(ccan)
> +add_subdirectory(util)
> # Libraries
> add_subdirectory(libibumad)
> add_subdirectory(libibumad/man)
> diff --git a/providers/cxgb4/cq.c b/providers/cxgb4/cq.c
> index 1ed7dfdb88d4..5f662ac49d0c 100644
> --- a/providers/cxgb4/cq.c
> +++ b/providers/cxgb4/cq.c
> @@ -37,6 +37,7 @@
> #include <sys/errno.h>
> #include <netinet/in.h>
> #include <infiniband/opcode.h>
> +#include <util/compiler.h>
> #include "libcxgb4.h"
> #include "cxgb4-abi.h"
>
> diff --git a/providers/cxgb4/libcxgb4.h b/providers/cxgb4/libcxgb4.h
> index 4c8383209287..9a4bc98f34e9 100644
> --- a/providers/cxgb4/libcxgb4.h
> +++ b/providers/cxgb4/libcxgb4.h
> @@ -264,6 +264,4 @@ void dump_state();
> extern int stall_to;
> #endif
>
> -#define uninitialized_var(x) x = x
> -
> #endif /* IWCH_H */
> diff --git a/providers/cxgb4/qp.c b/providers/cxgb4/qp.c
> index 384bf11369ac..95b459a1a9b4 100644
> --- a/providers/cxgb4/qp.c
> +++ b/providers/cxgb4/qp.c
> @@ -37,6 +37,7 @@
> #include <string.h>
> #include <stdio.h>
> #include <netinet/in.h>
> +#include <util/compiler.h>
> #include "libcxgb4.h"
>
> #ifdef STATS
> diff --git a/providers/mlx4/mlx4.h b/providers/mlx4/mlx4.h
> index 95a6521c457b..a2d39e169c15 100644
> --- a/providers/mlx4/mlx4.h
> +++ b/providers/mlx4/mlx4.h
> @@ -43,10 +43,6 @@
>
> #define MLX4_PORTS_NUM 2
>
> -#ifndef uninitialized_var
> -#define uninitialized_var(x) x = x
> -#endif
> -
> #include <valgrind/memcheck.h>
>
> #define PFX "mlx4: "
> diff --git a/providers/mlx4/qp.c b/providers/mlx4/qp.c
> index 4b5acd71108e..268fb7dc83dd 100644
> --- a/providers/mlx4/qp.c
> +++ b/providers/mlx4/qp.c
> @@ -39,6 +39,7 @@
> #include <pthread.h>
> #include <string.h>
> #include <errno.h>
> +#include <util/compiler.h>
>
> #include "mlx4.h"
> #include "doorbell.h"
> diff --git a/providers/mlx5/mlx5.h b/providers/mlx5/mlx5.h
> index f8674c7a90db..cb65429b51f7 100644
> --- a/providers/mlx5/mlx5.h
> +++ b/providers/mlx5/mlx5.h
> @@ -48,10 +48,6 @@
> #define unlikely(x) __builtin_expect((x), 0)
> #endif
>
> -#ifndef uninitialized_var
> -#define uninitialized_var(x) x = x
> -#endif
> -
> #include <valgrind/memcheck.h>
>
> #ifdef HAVE_FUNC_ATTRIBUTE_ALWAYS_INLINE
> diff --git a/providers/mlx5/qp.c b/providers/mlx5/qp.c
> index 04abe1588d6e..e82b1a0bebc3 100644
> --- a/providers/mlx5/qp.c
> +++ b/providers/mlx5/qp.c
> @@ -38,6 +38,7 @@
> #include <string.h>
> #include <errno.h>
> #include <stdio.h>
> +#include <util/compiler.h>
>
> #include "mlx5.h"
> #include "doorbell.h"
> diff --git a/util/CMakeLists.txt b/util/CMakeLists.txt
> new file mode 100644
> index 000000000000..1cda8905d8f4
> --- /dev/null
> +++ b/util/CMakeLists.txt
> @@ -0,0 +1,3 @@
> +publish_internal_headers(util
> + compiler.h
> + )
> diff --git a/util/compiler.h b/util/compiler.h
> new file mode 100644
> index 000000000000..9b57e048df4b
> --- /dev/null
> +++ b/util/compiler.h
> @@ -0,0 +1,18 @@
> +/* GPLv2 or OpenIB.org BSD (MIT) See COPYING file */
> +#ifndef UTIL_COMPILER_H
> +#define UTIL_COMPILER_H
> +
> +/* Use to tag a variable that causes compiler warnings. Use as:
> + int uninitialized_var(sz)
> +
> + This is only enabled for old compilers. gcc 6.x and beyond have excellent
> + static flow analysis. If code solicits a warning from 6.x it is almost
> + certainly too complex for a human to understand.
> +*/
> +#if __GNUC__ >= 6 || defined(__clang__)
> +#define uninitialized_var(x) x
> +#else
> +#define uninitialized_var(x) x = x
> +#endif
> +
> +#endif
> --
> 2.1.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH qedr 05/10] qedr: Add support for QP verbs
From: Leon Romanovsky @ 2016-10-09 6:27 UTC (permalink / raw)
To: Ram Amrani
Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA,
Ariel.Elior-YGCgFSpz5w/QT0dZR+AlfA,
Michal.Kalderon-YGCgFSpz5w/QT0dZR+AlfA,
Yuval.Mintz-YGCgFSpz5w/QT0dZR+AlfA,
rajesh.borundia-YGCgFSpz5w/QT0dZR+AlfA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1475682483-9878-6-git-send-email-Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 647 bytes --]
On Wed, Oct 05, 2016 at 06:47:58PM +0300, Ram Amrani wrote:
> Add support for Queue Pair verbs which adds, deletes,
> modifies and queries Queue Pairs.
>
> Signed-off-by: Rajesh Borundia <rajesh.borundia-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> Signed-off-by: Ram Amrani <Ram.Amrani-YGCgFSpz5w/QT0dZR+AlfA@public.gmane.org>
> ---
> + qp->wqe_wr_id = kcalloc(qp->sq.max_wr, sizeof(*qp->wqe_wr_id),
> + GFP_KERNEL);
> + if (!qp->wqe_wr_id) {
> + DP_ERR(dev, "create qp: failed SQ shadow memory allocation\n");
> + return -ENOMEM;
> + }
Please remove prints for ENOMEM failures for all code. k*alloc
interfaces will print it for you.
Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH 26/28] Avoid gcc 5.4 warning -Wunused-result
From: Yishai Hadas @ 2016-10-09 13:31 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA, Devesh Sharma,
Hal Rosenstock, Mike Marciniszyn, Moni Shoua, Sean Hefty,
Steve Wise, Tatyana Nikolova, Vladimir Sokolovsky, Yishai Hadas
In-Reply-To: <1473109698-31408-27-git-send-email-jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
On 9/6/2016 12:08 AM, Jason Gunthorpe wrote:
> It used to be you could suppress this with (void), however the gcc
> developers have decided to get rid of that.
>
> So, look closely at each occurrence and decide what to do:
> - *pingpong: Join the error handling with the if statement directly
> above
> - niegh: read on a timer_fd should never fail, so just use assert.
> The assert is compiled out for Release builds so this is no-change
> - acm: Failure of ucma_set_server_port is detected by a 0 return
> so check fscanf and return appropriately. This is no change since
> fscanf failure was assumed to have left server_port as 0 (though
> I doubt the standard supports that usage)
> - rsocket: This looks super sketchy. At least lets make the intent clear
> with a read_all/write_all wrapper that calls assert. Most likely
> this code is wrong..
> Mangle the code with failable_fscanf to make it clear, but as with
> acm, I don't think the standard supports this usage.
>
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
This patch breaks rc/uc/ud/srq pingpong in a basic run, please see below.
Can you please prepare some fix for that as it was already merged ?
> ---
> libibverbs/examples/rc_pingpong.c | 15 +++++-----
> libibverbs/examples/srq_pingpong.c | 13 +++++++--
> libibverbs/examples/uc_pingpong.c | 15 +++++-----
> libibverbs/examples/ud_pingpong.c | 17 +++++-------
> libibverbs/src/neigh.c | 7 +++--
> librdmacm/src/acm.c | 3 +-
> librdmacm/src/rsocket.c | 56 +++++++++++++++++++++++++++-----------
> 7 files changed, 78 insertions(+), 48 deletions(-)
>
> diff --git a/libibverbs/examples/rc_pingpong.c b/libibverbs/examples/rc_pingpong.c
> index 1fad16a0be7c..7bcc413a0f1d 100644
> --- a/libibverbs/examples/rc_pingpong.c
> +++ b/libibverbs/examples/rc_pingpong.c
> @@ -208,14 +208,13 @@ static struct pingpong_dest *pp_client_exch_dest(const char *servername, int por
> goto out;
> }
>
> - if (read(sockfd, msg, sizeof msg) != sizeof msg) {
> - perror("client read");
> - fprintf(stderr, "Couldn't read remote address\n");
> + if (read(sockfd, msg, sizeof msg) != sizeof msg ||
> + write(sockfd, "done", sizeof "done") != sizeof "done") {
> + perror("client read/write");
> + fprintf(stderr, "Couldn't read/write remote address\n");
> goto out;
> }
>
> - write(sockfd, "done", sizeof "done");
> -
> rem_dest = malloc(sizeof *rem_dest);
> if (!rem_dest)
> goto out;
> @@ -316,14 +315,14 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
> gid_to_wire_gid(&my_dest->gid, gid);
> sprintf(msg, "%04x:%06x:%06x:%s", my_dest->lid, my_dest->qpn,
> my_dest->psn, gid);
> - if (write(connfd, msg, sizeof msg) != sizeof msg) {
> - fprintf(stderr, "Couldn't send local address\n");
> + if (write(connfd, msg, sizeof msg) != sizeof msg ||
> + read(connfd, msg, sizeof msg) != sizeof msg) {
> + fprintf(stderr, "Couldn't send/recv local address\n");
At that step the server expects to read a "done" response from its
client, checking whether the read was done for sizeof msg which is much
larger will fail.
Same issue appears below in several places in srq/ud/uc pingpong.
> free(rem_dest);
> rem_dest = NULL;
> goto out;
> }
>
> - read(connfd, msg, sizeof msg);
>
> out:
> close(connfd);
> diff --git a/libibverbs/examples/srq_pingpong.c b/libibverbs/examples/srq_pingpong.c
> index 929b736545c7..e6492dc553fd 100644
> --- a/libibverbs/examples/srq_pingpong.c
> +++ b/libibverbs/examples/srq_pingpong.c
> @@ -222,8 +222,10 @@ static struct pingpong_dest *pp_client_exch_dest(const char *servername, int por
> wire_gid_to_gid(gid, &rem_dest[i].gid);
> }
>
> - write(sockfd, "done", sizeof "done");
> -
> + if (write(sockfd, "done", sizeof "done") != sizeof "done") {
> + perror("client write");
> + goto out;
> + }
> out:
> close(sockfd);
> return rem_dest;
> @@ -333,7 +335,12 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
> }
> }
>
> - read(connfd, msg, sizeof msg);
> + if (read(connfd, msg, sizeof msg) != sizeof msg) {
> + perror("client write");
> + free(rem_dest);
> + rem_dest = NULL;
> + goto out;
> + }
>
> out:
> close(connfd);
> diff --git a/libibverbs/examples/uc_pingpong.c b/libibverbs/examples/uc_pingpong.c
> index 3802e3821773..d132de98694a 100644
> --- a/libibverbs/examples/uc_pingpong.c
> +++ b/libibverbs/examples/uc_pingpong.c
> @@ -176,13 +176,13 @@ static struct pingpong_dest *pp_client_exch_dest(const char *servername, int por
> goto out;
> }
>
> - if (read(sockfd, msg, sizeof msg) != sizeof msg) {
> - perror("client read");
> - fprintf(stderr, "Couldn't read remote address\n");
> + if (read(sockfd, msg, sizeof msg) != sizeof msg ||
> + write(sockfd, "done", sizeof "done") != sizeof "done") {
> + perror("client read/write");
> + fprintf(stderr, "Couldn't read/write remote address\n");
> goto out;
> }
>
> - write(sockfd, "done", sizeof "done");
>
> rem_dest = malloc(sizeof *rem_dest);
> if (!rem_dest)
> @@ -284,15 +284,14 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
> gid_to_wire_gid(&my_dest->gid, gid);
> sprintf(msg, "%04x:%06x:%06x:%s", my_dest->lid, my_dest->qpn,
> my_dest->psn, gid);
> - if (write(connfd, msg, sizeof msg) != sizeof msg) {
> - fprintf(stderr, "Couldn't send local address\n");
> + if (write(connfd, msg, sizeof msg) != sizeof msg ||
> + read(connfd, msg, sizeof msg) != sizeof msg) {
> + fprintf(stderr, "Couldn't send/recv local address\n");
> free(rem_dest);
> rem_dest = NULL;
> goto out;
> }
>
> - read(connfd, msg, sizeof msg);
> -
> out:
> close(connfd);
> return rem_dest;
> diff --git a/libibverbs/examples/ud_pingpong.c b/libibverbs/examples/ud_pingpong.c
> index fa99b9e51dfb..67da4bd90f32 100644
> --- a/libibverbs/examples/ud_pingpong.c
> +++ b/libibverbs/examples/ud_pingpong.c
> @@ -176,14 +176,13 @@ static struct pingpong_dest *pp_client_exch_dest(const char *servername, int por
> goto out;
> }
>
> - if (read(sockfd, msg, sizeof msg) != sizeof msg) {
> - perror("client read");
> - fprintf(stderr, "Couldn't read remote address\n");
> + if (read(sockfd, msg, sizeof msg) != sizeof msg ||
> + write(sockfd, "done", sizeof "done") != sizeof "done") {
> + perror("client read/write");
> + fprintf(stderr, "Couldn't read/write remote address\n");
> goto out;
> }
>
> - write(sockfd, "done", sizeof "done");
> -
> rem_dest = malloc(sizeof *rem_dest);
> if (!rem_dest)
> goto out;
> @@ -282,15 +281,13 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
> gid_to_wire_gid(&my_dest->gid, gid);
> sprintf(msg, "%04x:%06x:%06x:%s", my_dest->lid, my_dest->qpn,
> my_dest->psn, gid);
> - if (write(connfd, msg, sizeof msg) != sizeof msg) {
> - fprintf(stderr, "Couldn't send local address\n");
> + if (write(connfd, msg, sizeof msg) != sizeof msg ||
> + read(connfd, msg, sizeof msg) != sizeof msg) {
> + fprintf(stderr, "Couldn't send/recv local address\n");
> free(rem_dest);
> rem_dest = NULL;
> goto out;
> }
> -
> - read(connfd, msg, sizeof msg);
> -
> out:
> close(connfd);
> return rem_dest;
> diff --git a/libibverbs/src/neigh.c b/libibverbs/src/neigh.c
> index 6b6e58cd52f8..5acfcf06fcde 100644
> --- a/libibverbs/src/neigh.c
> +++ b/libibverbs/src/neigh.c
> @@ -19,6 +19,7 @@
> #include <unistd.h>
> #include <ifaddrs.h>
> #include <netdb.h>
> +#include <assert.h>
> #ifndef _LINUX_IF_H
> #include <net/if.h>
> #else
> @@ -372,9 +373,11 @@ static struct nl_addr *process_get_neigh_mac(
>
> if (FD_ISSET(timer_fd, &fdset)) {
> uint64_t read_val;
> + ssize_t rc;
>
> - (void)read(timer_fd, &read_val,
> - sizeof(read_val));
> + rc =
> + read(timer_fd, &read_val, sizeof(read_val));
> + assert(rc == sizeof(read_val));
> if (++retries >= NUM_OF_TRIES) {
> if (!errno)
> errno = EDESTADDRREQ;
> diff --git a/librdmacm/src/acm.c b/librdmacm/src/acm.c
> index f0da01e6d286..c097bb923b55 100644
> --- a/librdmacm/src/acm.c
> +++ b/librdmacm/src/acm.c
> @@ -121,7 +121,8 @@ static int ucma_set_server_port(void)
> FILE *f;
>
> if ((f = fopen("/var/run/ibacm.port", "r" STREAM_CLOEXEC))) {
> - fscanf(f, "%" SCNu16, &server_port);
> + if (fscanf(f, "%" SCNu16, &server_port) != 1)
> + server_port = 0;
> fclose(f);
> }
> return server_port;
> diff --git a/librdmacm/src/rsocket.c b/librdmacm/src/rsocket.c
> index 818505fbe02e..5645f40d2460 100644
> --- a/librdmacm/src/rsocket.c
> +++ b/librdmacm/src/rsocket.c
> @@ -404,6 +404,20 @@ struct ds_udp_header {
>
> #define ds_next_qp(qp) container_of((qp)->list.next, struct ds_qp, list)
>
> +static void write_all(int fd, const void *msg, size_t len)
> +{
> + // FIXME: if fd is a socket this really needs to handle EINTR and other conditions.
> + ssize_t rc = write(fd, msg, len);
> + assert(rc == len);
> +}
> +
> +static void read_all(int fd, void *msg, size_t len)
> +{
> + // FIXME: if fd is a socket this really needs to handle EINTR and other conditions.
> + ssize_t rc = read(fd, msg, len);
> + assert(rc == len);
> +}
> +
> static void ds_insert_qp(struct rsocket *rs, struct ds_qp *qp)
> {
> if (!rs->qp_list)
> @@ -444,8 +458,8 @@ static int rs_notify_svc(struct rs_svc *svc, struct rsocket *rs, int cmd)
> msg.cmd = cmd;
> msg.status = EINVAL;
> msg.rs = rs;
> - write(svc->sock[0], &msg, sizeof msg);
> - read(svc->sock[0], &msg, sizeof msg);
> + write_all(svc->sock[0], &msg, sizeof msg);
> + read_all(svc->sock[0], &msg, sizeof msg);
> ret = rdma_seterrno(msg.status);
> if (svc->cnt)
> goto unlock;
> @@ -484,6 +498,15 @@ static int rs_scale_to_value(int value, int bits)
> value : (value & ~(1 << (bits - 1))) << bits;
> }
>
> +/* gcc > ~5 will not allow (void)fscanf to suppress -Wunused-result, but this
> + will do it. In this case ignoring the result is OK (but horribly
> + unfriendly to user) since the library has a sane default. */
> +#define failable_fscanf(f, fmt, ...) \
> + { \
> + int rc = fscanf(f, fmt, __VA_ARGS__); \
> + (void) rc; \
> + }
> +
> void rs_configure(void)
> {
> FILE *f;
> @@ -501,27 +524,27 @@ void rs_configure(void)
> ucma_ib_init();
>
> if ((f = fopen(RS_CONF_DIR "/polling_time", "r"))) {
> - (void) fscanf(f, "%u", &polling_time);
> + failable_fscanf(f, "%u", &polling_time);
> fclose(f);
> }
>
> if ((f = fopen(RS_CONF_DIR "/inline_default", "r"))) {
> - (void) fscanf(f, "%hu", &def_inline);
> + failable_fscanf(f, "%hu", &def_inline);
> fclose(f);
> }
>
> if ((f = fopen(RS_CONF_DIR "/sqsize_default", "r"))) {
> - (void) fscanf(f, "%hu", &def_sqsize);
> + failable_fscanf(f, "%hu", &def_sqsize);
> fclose(f);
> }
>
> if ((f = fopen(RS_CONF_DIR "/rqsize_default", "r"))) {
> - (void) fscanf(f, "%hu", &def_rqsize);
> + failable_fscanf(f, "%hu", &def_rqsize);
> fclose(f);
> }
>
> if ((f = fopen(RS_CONF_DIR "/mem_default", "r"))) {
> - (void) fscanf(f, "%u", &def_mem);
> + failable_fscanf(f, "%u", &def_mem);
> fclose(f);
>
> if (def_mem < 1)
> @@ -529,14 +552,14 @@ void rs_configure(void)
> }
>
> if ((f = fopen(RS_CONF_DIR "/wmem_default", "r"))) {
> - (void) fscanf(f, "%u", &def_wmem);
> + failable_fscanf(f, "%u", &def_wmem);
> fclose(f);
> if (def_wmem < RS_SNDLOWAT)
> def_wmem = RS_SNDLOWAT << 1;
> }
>
> if ((f = fopen(RS_CONF_DIR "/iomap_size", "r"))) {
> - (void) fscanf(f, "%hu", &def_iomap_size);
> + failable_fscanf(f, "%hu", &def_iomap_size);
> fclose(f);
>
> /* round to supported values */
> @@ -3345,7 +3368,8 @@ static int rs_set_keepalive(struct rsocket *rs, int on)
> if (on) {
> if (!rs->keepalive_time) {
> if ((f = fopen("/proc/sys/net/ipv4/tcp_keepalive_time", "r"))) {
> - (void) fscanf(f, "%u", &rs->keepalive_time);
> + if (fscanf(f, "%u", &rs->keepalive_time) != 1)
> + rs->keepalive_time = 7200;
> fclose(f);
> } else {
> rs->keepalive_time = 7200;
> @@ -3985,7 +4009,7 @@ static void udp_svc_process_sock(struct rs_svc *svc)
> {
> struct rs_svc_msg msg;
>
> - read(svc->sock[1], &msg, sizeof msg);
> + read_all(svc->sock[1], &msg, sizeof msg);
> switch (msg.cmd) {
> case RS_SVC_ADD_DGRAM:
> msg.status = rs_svc_add_rs(svc, msg.rs);
> @@ -4009,7 +4033,7 @@ static void udp_svc_process_sock(struct rs_svc *svc)
> break;
> }
>
> - write(svc->sock[1], &msg, sizeof msg);
> + write_all(svc->sock[1], &msg, sizeof msg);
> }
>
> static uint8_t udp_svc_sgid_index(struct ds_dest *dest, union ibv_gid *sgid)
> @@ -4184,7 +4208,7 @@ static void *udp_svc_run(void *arg)
> ret = rs_svc_grow_sets(svc, 4);
> if (ret) {
> msg.status = ret;
> - write(svc->sock[1], &msg, sizeof msg);
> + write_all(svc->sock[1], &msg, sizeof msg);
> return (void *) (uintptr_t) ret;
> }
>
> @@ -4222,7 +4246,7 @@ static void tcp_svc_process_sock(struct rs_svc *svc)
> struct rs_svc_msg msg;
> int i;
>
> - read(svc->sock[1], &msg, sizeof msg);
> + read_all(svc->sock[1], &msg, sizeof msg);
> switch (msg.cmd) {
> case RS_SVC_ADD_KEEPALIVE:
> msg.status = rs_svc_add_rs(svc, msg.rs);
> @@ -4253,7 +4277,7 @@ static void tcp_svc_process_sock(struct rs_svc *svc)
> default:
> break;
> }
> - write(svc->sock[1], &msg, sizeof msg);
> + write_all(svc->sock[1], &msg, sizeof msg);
> }
>
> /*
> @@ -4282,7 +4306,7 @@ static void *tcp_svc_run(void *arg)
> ret = rs_svc_grow_sets(svc, 16);
> if (ret) {
> msg.status = ret;
> - write(svc->sock[1], &msg, sizeof msg);
> + write_all(svc->sock[1], &msg, sizeof msg);
> return (void *) (uintptr_t) ret;
> }
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 26/28] Avoid gcc 5.4 warning -Wunused-result
From: Bart Van Assche @ 2016-10-09 15:14 UTC (permalink / raw)
To: Jason Gunthorpe, Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA
Cc: Devesh Sharma, Hal Rosenstock, Mike Marciniszyn, Moni Shoua,
Sean Hefty, Steve Wise, Tatyana Nikolova, Vladimir Sokolovsky,
Yishai Hadas
In-Reply-To: <1473109698-31408-27-git-send-email-jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
On 09/05/16 14:08, Jason Gunthorpe wrote:
> It used to be you could suppress this with (void), however the gcc
> developers have decided to get rid of that.
>
> So, look closely at each occurrence and decide what to do:
> [ ... ]
Hi Jason,
Are you aware that there are less intrusive ways to suppress "unused
result" warnings, e.g. by using a macro like the one below?
#define UNUSED_VALUE(v) do { if (v) { } } while (0)
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH rdma-core 1/5] Pull uninitialized_var into util/compiler.h
From: Jason Gunthorpe @ 2016-10-09 23:01 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161009054601.GB9282-2ukJVAZIZ/Y@public.gmane.org>
On Sun, Oct 09, 2016 at 08:46:01AM +0300, Leon Romanovsky wrote:
> 1. Can we remove this uninitialized_var at all and simply replace by
> default values?
I think people would complain about the extra stores. gcc 6 will
eliminate them, but older compilers will not.
But yes, we could do this.
> 2. Do we really want additional folder to ccan with such common headers?
> What about rename ccan to be util and put this new file there (in case
> you proceed with it)?
I see the ccan directory as stuf from ccan that we are not going to
change, while util is for new stuff specific to this tree.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 26/28] Avoid gcc 5.4 warning -Wunused-result
From: Jason Gunthorpe @ 2016-10-09 23:12 UTC (permalink / raw)
To: Bart Van Assche
Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA, Devesh Sharma,
Hal Rosenstock, Mike Marciniszyn, Moni Shoua, Sean Hefty,
Steve Wise, Tatyana Nikolova, Vladimir Sokolovsky, Yishai Hadas
In-Reply-To: <3884fb5e-13a2-3ab6-0cce-66f73fbe84b7-HInyCGIudOg@public.gmane.org>
On Sun, Oct 09, 2016 at 08:14:26AM -0700, Bart Van Assche wrote:
> Are you aware that there are less intrusive ways to suppress "unused result"
> warnings, e.g. by using a macro like the one below?
>
> #define UNUSED_VALUE(v) do { if (v) { } } while (0)
Hmm, I think the only place I'd use that is the fscanf stuff - and
that is only because I don't know about about that code to put in
sensible error handling. It doesn't seem much better than the
failable_fscanf wrapper?
All the other places gained at least some kind of improvement in error
handling..
Generally this warning is not wrong so I'm not excited to have
'official' infrastructure to ignore it.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 26/28] Avoid gcc 5.4 warning -Wunused-result
From: Jason Gunthorpe @ 2016-10-09 23:19 UTC (permalink / raw)
To: Yishai Hadas
Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA, Devesh Sharma,
Hal Rosenstock, Mike Marciniszyn, Moni Shoua, Sean Hefty,
Steve Wise, Tatyana Nikolova, Vladimir Sokolovsky, Yishai Hadas
In-Reply-To: <9ae84345-4427-4689-e6d1-0bfa3eb19630-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
On Sun, Oct 09, 2016 at 04:31:17PM +0300, Yishai Hadas wrote:
> >- if (write(connfd, msg, sizeof msg) != sizeof msg) {
> >- fprintf(stderr, "Couldn't send local address\n");
> >+ if (write(connfd, msg, sizeof msg) != sizeof msg ||
> >+ read(connfd, msg, sizeof msg) != sizeof msg) {
> >+ fprintf(stderr, "Couldn't send/recv local address\n");
>
> At that step the server expects to read a "done" response from its client,
> checking whether the read was done for sizeof msg which is much larger will
> fail.
This OK?
>From db525af53140c3b7604ab45406ed8845cb6171e1 Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
Date: Sun, 9 Oct 2016 17:17:54 -0600
Subject: [PATCH] verbs: Fix read error check in pingpong
Turns out these reads rely on the short readed caused by EOS.
Fixes: f3912df771db (Avoid gcc 5.4 warning -Wunused-result)
Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
---
libibverbs/examples/rc_pingpong.c | 2 +-
libibverbs/examples/srq_pingpong.c | 2 +-
libibverbs/examples/uc_pingpong.c | 2 +-
libibverbs/examples/ud_pingpong.c | 2 +-
4 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/libibverbs/examples/rc_pingpong.c b/libibverbs/examples/rc_pingpong.c
index c92e551c00e6..aca7bac4491b 100644
--- a/libibverbs/examples/rc_pingpong.c
+++ b/libibverbs/examples/rc_pingpong.c
@@ -309,7 +309,7 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
sprintf(msg, "%04x:%06x:%06x:%s", my_dest->lid, my_dest->qpn,
my_dest->psn, gid);
if (write(connfd, msg, sizeof msg) != sizeof msg ||
- read(connfd, msg, sizeof msg) != sizeof msg) {
+ read(connfd, msg, sizeof msg) != sizeof "done") {
fprintf(stderr, "Couldn't send/recv local address\n");
free(rem_dest);
rem_dest = NULL;
diff --git a/libibverbs/examples/srq_pingpong.c b/libibverbs/examples/srq_pingpong.c
index 863ff10dd0f4..8f55d78e87d4 100644
--- a/libibverbs/examples/srq_pingpong.c
+++ b/libibverbs/examples/srq_pingpong.c
@@ -333,7 +333,7 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
}
}
- if (read(connfd, msg, sizeof msg) != sizeof msg) {
+ if (read(connfd, msg, sizeof msg) != sizeof "done") {
perror("client write");
free(rem_dest);
rem_dest = NULL;
diff --git a/libibverbs/examples/uc_pingpong.c b/libibverbs/examples/uc_pingpong.c
index 2b105b947cf3..b565bacaff2a 100644
--- a/libibverbs/examples/uc_pingpong.c
+++ b/libibverbs/examples/uc_pingpong.c
@@ -283,7 +283,7 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
sprintf(msg, "%04x:%06x:%06x:%s", my_dest->lid, my_dest->qpn,
my_dest->psn, gid);
if (write(connfd, msg, sizeof msg) != sizeof msg ||
- read(connfd, msg, sizeof msg) != sizeof msg) {
+ read(connfd, msg, sizeof msg) != sizeof "done") {
fprintf(stderr, "Couldn't send/recv local address\n");
free(rem_dest);
rem_dest = NULL;
diff --git a/libibverbs/examples/ud_pingpong.c b/libibverbs/examples/ud_pingpong.c
index d0cd73cc0fae..ddb68cf8624b 100644
--- a/libibverbs/examples/ud_pingpong.c
+++ b/libibverbs/examples/ud_pingpong.c
@@ -280,7 +280,7 @@ static struct pingpong_dest *pp_server_exch_dest(struct pingpong_context *ctx,
sprintf(msg, "%04x:%06x:%06x:%s", my_dest->lid, my_dest->qpn,
my_dest->psn, gid);
if (write(connfd, msg, sizeof msg) != sizeof msg ||
- read(connfd, msg, sizeof msg) != sizeof msg) {
+ read(connfd, msg, sizeof msg) != sizeof "done") {
fprintf(stderr, "Couldn't send/recv local address\n");
free(rem_dest);
rem_dest = NULL;
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: [PULL REQUEST] Please pull rdma.git (tag for-linus)
From: Linus Torvalds @ 2016-10-10 0:14 UTC (permalink / raw)
To: Doug Ledford, Steve Wise; +Cc: linux-rdma
In-Reply-To: <00af6acf-ec90-ea0e-68d6-f9e962c89bd3-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Fri, Oct 7, 2016 at 3:24 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>
> 1) cxgb4 - keep the head version in your tree, my tree and the common
> ancestor were prior to a refactoring in your tree that moved uld_attach
> from cxgb4_main.c to cxgb4_uld.c
Hmm. As far as I can tell, a proper merge also involves adding the line
lld->fr_nsmr_tpte_wr_support = adap->params.fr_nsmr_tpte_wr_support;
to uld_init() in cxgb4_uld.c.
I did so, and it all seems to build find for me, but I obviously
cannot do any testing. Somebody with the proper cxgb4 hardware should
verify.
Of course, it only affects the very special IB_WR_REG_MR fastpath
case, so maybe nobody much cares. Adding Steve Wise to the cc, to make
sure somebody actually tests this.
(Side note: current git has what appears to be a use-after-free
netfilter buglet, but it shouldn't trigger problems often enough to
keep people from testing. It kept me from doing my merge window pulls
while I investigated, though).
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PULL REQUEST] Please pull rdma.git (tag for-linus)
From: Doug Ledford @ 2016-10-10 3:07 UTC (permalink / raw)
To: Linus Torvalds, Steve Wise; +Cc: linux-rdma
In-Reply-To: <CA+55aFw5tftp1Da5Hs8ks+=hiodZ_NTCH3Z9efPxDVo7h75RAw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1.1: Type: text/plain, Size: 847 bytes --]
On 10/9/2016 8:14 PM, Linus Torvalds wrote:
> On Fri, Oct 7, 2016 at 3:24 PM, Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> 1) cxgb4 - keep the head version in your tree, my tree and the common
>> ancestor were prior to a refactoring in your tree that moved uld_attach
>> from cxgb4_main.c to cxgb4_uld.c
>
> Hmm. As far as I can tell, a proper merge also involves adding the line
>
> lld->fr_nsmr_tpte_wr_support = adap->params.fr_nsmr_tpte_wr_support;
>
> to uld_init() in cxgb4_uld.c.
>
> I did so, and it all seems to build find for me, but I obviously
> cannot do any testing. Somebody with the proper cxgb4 hardware should
> verify.
Thanks for that catch, we'll get it tested.
--
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
GPG Key ID: 0E572FDD
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]
^ permalink raw reply
* Re: [PATCH 0/5 for rdma-core] Five rxe_cfg patches
From: Leon Romanovsky @ 2016-10-10 4:02 UTC (permalink / raw)
To: Bart Van Assche; +Cc: Doug Ledford, Jason Gunthorpe, Moni Shoua, linux-rdma
In-Reply-To: <bf5dba39-7e14-23ee-4aa1-2a276d629fe9-XdAiOPVOjttBDgjK7y7TUQ@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 994 bytes --]
On Fri, Oct 07, 2016 at 11:37:43AM -0700, Bart Van Assche wrote:
> Hello Doug, Jason and Moni,
>
> These five patches is what I came up with after having had a look at the
> rxe_cfg source code. It would be appreciated if these patches would be
> considered for inclusion in the rdma-core repository. The filenames of these
> patches are:
>
> 0001-rxe_cfg-Use-Perl-functions-instead-of-parsing-the-ou.patch
> 0002-rxe_cfg-Initialize-rxe_mtu-even-if-ibv_definfo-fails.patch
> 0003-rxe_cfg-Do-not-suppress-stderr.patch
> 0004-rxe_cfg-Remove-dead-code-from-show_module_status.patch
> 0005-rxe_cfg-Use-printf-instead-of-using-a-loop.patch
Hi Bart,
Thank you for investing time, Moni started to look/test them and update
us on the result.
Thanks.
>
> Thanks,
>
> Bart.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH rdma-core 1/5] Pull uninitialized_var into util/compiler.h
From: Leon Romanovsky @ 2016-10-10 4:09 UTC (permalink / raw)
To: Jason Gunthorpe; +Cc: Doug Ledford, linux-rdma-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20161009230150.GB12551-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 957 bytes --]
On Sun, Oct 09, 2016 at 05:01:51PM -0600, Jason Gunthorpe wrote:
> On Sun, Oct 09, 2016 at 08:46:01AM +0300, Leon Romanovsky wrote:
>
> > 1. Can we remove this uninitialized_var at all and simply replace by
> > default values?
>
> I think people would complain about the extra stores. gcc 6 will
> eliminate them, but older compilers will not.
This unintialized_var(x) adds extra store too (... x = x ...).
>
> But yes, we could do this.
>
> > 2. Do we really want additional folder to ccan with such common headers?
> > What about rename ccan to be util and put this new file there (in case
> > you proceed with it)?
>
> I see the ccan directory as stuf from ccan that we are not going to
> change, while util is for new stuff specific to this tree.
And I see this folder as a location of shared code with different utils
(ccan, custom, e.t.c), but it doesn't really matter now and I will be fine
with any directory structure layout.
Thanks
>
> Jason
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH V2] i40iw: Add Quality of Service support
From: Leon Romanovsky @ 2016-10-10 4:27 UTC (permalink / raw)
To: Henry Orosco
Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA,
linux-rdma-u79uwXL29TY76Z2rM5mHXA,
e1000-rdma-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f
In-Reply-To: <20161007204747.6420-1-henry.orosco-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 785 bytes --]
On Fri, Oct 07, 2016 at 03:47:47PM -0500, Henry Orosco wrote:
<...>
>
> switch (iwdev->init_state) {
> case RDMA_DEV_REGISTERED:
> @@ -1628,6 +1642,7 @@ static int i40iw_open(struct i40e_info *ldev, struct i40e_client *client)
> iwdev->init_state = RDMA_DEV_REGISTERED;
> iwdev->iw_status = 1;
> i40iw_port_ibevent(iwdev);
> + iwdev->param_wq = create_singlethread_workqueue("l2params");
> i40iw_pr_info("i40iw_open completed\n");
No, please don't use this interface, For reference, see latest work from Bhaktipriya
Shridhar to eliminate this call. Doug and Dave sent a pull request with it to Linus in
last merge cycle.
Also, 1) you should check if this call succeeded and 2) ask yourself if
special workqueue is needed.
> return 0;
> } while (0);
Thanks.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCHv12 0/3] rdmacg: IB/core: rdma controller support
From: Leon Romanovsky @ 2016-10-10 4:46 UTC (permalink / raw)
To: Parav Pandit
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
Tejun Heo, Li Zefan, Johannes Weiner, Doug Ledford,
Christoph Hellwig, Liran Liss, Hefty, Sean, Jason Gunthorpe,
Haggai Eran, james.l.morris-QHcLZuEGTsvQT0dZR+AlfA, Or Gerlitz,
Matan Barak
In-Reply-To: <CAG53R5VNVb=8-LJbDRqjtOZG347ucPuc420bcfnDgBKMoKqU-w-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 4430 bytes --]
On Thu, Oct 06, 2016 at 07:19:24PM +0530, Parav Pandit wrote:
> Hi Leon,
>
> On Wed, Oct 5, 2016 at 4:52 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, Aug 31, 2016 at 02:07:24PM +0530, Parav Pandit wrote:
> >> rdmacg: IB/core: rdma controller support
> >>
> >> 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.
> >
> In other cgroup such as CPU, this is done using cpu.weight API. Where
> percentage or weight is configured by the user.
> In this mode, resources taken away from other cgroup proportionately.
> It works for cpu because its mainly stateless resource unlike rdma
> resources.
> So if we want to simplify user configuration similarly,
> percentage/weight configuration can be extended.
> This way they need not be introduced to users.
> I hope your definition of "user" is actual end-user and not rdma cgroup. Right?
Yes, "user" -> "admin".
I think that percentage is more intuitive to them and will be much easier to
explain how to use it. I always have in mind "swappiness" field and the
numerous questions on how to configure it.
> In other words, new object should be still added as new enum value in
> rdma_cgroup.h?
Yes, I had in mind something like IB_CGROUP_HCA, this is why it can be
done as a future work after accepting current patches.
> Only than it can be overwritten by specific UVERBs type as you
> described below. I think thats what you meant as you described below.
Exactly.
>
> Otherwise charging/uncharging this new percentage resource can get messy.
Agree
>
> > This HCA share will be overwritten by specific UVERBS types which you
> > already defined.
> >
> > What do you think?
>
> So to refine your proposal from cgroup perspective, instead of adding
> new resource type in rdma_cgroup.h for percentage, I prefer to have
>
> Existing
> 1. rdma.max
> 2. rdma.current
> New,
> 3. rdma.weight
> This ABI will have similar API to say
> echo "mlx4_0 50" > rdma.weight.
> Where 50 is weight of the resources.
> For example,
> for one cgroup instance weight=sum=100% resource for a given cgroup.
> for three cgroup instances percentage=(weight/sum)% = 50/(50+50+50) = 33%.
> One cgroup gets 33% resource.
>
> Weight can be in range of 1 to 10,000 similar to cpu cgroup.
This is exactly what I don't like, the percentage will remove from the
user the translation needs between weight and actual limitation.
IMHO CPU used weights because everything there is in weights :).
>
> This might work if applications running in all cgroups are similar.
> But weight doesn't do justice, when there are different type of
> applications running in each cgroup. Such as few running libfabric
> based apps, few running MPI, others directly using ibverbs.
> So as you said rdma.max configuration would be required for management
> plane to override weight (percentage) for certain resources.
Why?
The device exposes max values during initialization and if user asked
for 20% percent of HCA, he will get max*0.2.
>
>
> >
> > Except this proposal,
> > Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >
> > Thanks.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCHv12 0/3] rdmacg: IB/core: rdma controller support
From: Parav Pandit @ 2016-10-10 6:29 UTC (permalink / raw)
To: Leon Romanovsky
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, linux-rdma, Tejun Heo, Li Zefan,
Johannes Weiner, Doug Ledford, Christoph Hellwig, Liran Liss,
Hefty, Sean, Jason Gunthorpe, Haggai Eran,
james.l.morris-QHcLZuEGTsvQT0dZR+AlfA, Or Gerlitz, Matan Barak
In-Reply-To: <20161010044623.GI9282-2ukJVAZIZ/Y@public.gmane.org>
Hi Leon,
On Mon, Oct 10, 2016 at 10:16 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Thu, Oct 06, 2016 at 07:19:24PM +0530, Parav Pandit wrote:
>> Hi Leon,
>>
>> On Wed, Oct 5, 2016 at 4:52 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > On Wed, Aug 31, 2016 at 02:07:24PM +0530, Parav Pandit wrote:
>> >> rdmacg: IB/core: rdma controller support
>> >>
>> >> 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.
>> >
>> In other cgroup such as CPU, this is done using cpu.weight API. Where
>> percentage or weight is configured by the user.
>> In this mode, resources taken away from other cgroup proportionately.
>> It works for cpu because its mainly stateless resource unlike rdma
>> resources.
>> So if we want to simplify user configuration similarly,
>> percentage/weight configuration can be extended.
>> This way they need not be introduced to users.
>> I hope your definition of "user" is actual end-user and not rdma cgroup. Right?
>
> Yes, "user" -> "admin".
> I think that percentage is more intuitive to them and will be much easier to
> explain how to use it. I always have in mind "swappiness" field and the
> numerous questions on how to configure it.
>
>> In other words, new object should be still added as new enum value in
>> rdma_cgroup.h?
>
> Yes, I had in mind something like IB_CGROUP_HCA, this is why it can be
> done as a future work after accepting current patches.
>
What I meant is,
today we have RDMACG_VERB_RESOURCE_QP etc,
We will additionally have RDMACG_VERB_RESOURCE_INDIRECT_TBL etc in
cgroup_rdma.h.
So that its available for admin to override it.
>> Only than it can be overwritten by specific UVERBs type as you
>> described below. I think thats what you meant as you described below.
>
> Exactly.
>
>>
>> Otherwise charging/uncharging this new percentage resource can get messy.
>
> Agree
>
>>
>> > This HCA share will be overwritten by specific UVERBS types which you
>> > already defined.
>> >
>> > What do you think?
>>
>> So to refine your proposal from cgroup perspective, instead of adding
>> new resource type in rdma_cgroup.h for percentage, I prefer to have
>>
>> Existing
>> 1. rdma.max
>> 2. rdma.current
>> New,
>> 3. rdma.weight
>> This ABI will have similar API to say
>> echo "mlx4_0 50" > rdma.weight.
>> Where 50 is weight of the resources.
>> For example,
>> for one cgroup instance weight=sum=100% resource for a given cgroup.
>> for three cgroup instances percentage=(weight/sum)% = 50/(50+50+50) = 33%.
>> One cgroup gets 33% resource.
>>
>> Weight can be in range of 1 to 10,000 similar to cpu cgroup.
>
> This is exactly what I don't like, the percentage will remove from the
> user the translation needs between weight and actual limitation.
>
> IMHO CPU used weights because everything there is in weights :).
>
I admit weight are not very intuitive, I was aligning to the existing
other cgroup interfaces which achieves similar functionality.
I will let Tejun approve the "percentage" or "ratio" new file
interface as its little different than weight.
>>
>> This might work if applications running in all cgroups are similar.
>> But weight doesn't do justice, when there are different type of
>> applications running in each cgroup. Such as few running libfabric
>> based apps, few running MPI, others directly using ibverbs.
>> So as you said rdma.max configuration would be required for management
>> plane to override weight (percentage) for certain resources.
>
> Why?
> The device exposes max values during initialization and if user asked
> for 20% percent of HCA, he will get max*0.2.
Because every application may not be equivalent of other application.
For example, some require one to one QP and PD mapping.
Some share single PD across multiple QPs.
Some have ratio of 100 MRs per QP, as factor of memory size and operations.
some servers like to have 1K MRs per QP.
So if we have just weight, it will equally distributes MRs per QP in
all cgroup and that either leads to unused resource per cgroup or,
lesser number of cg instances.
So fine tuning required for individual one, which we already have.
weight or percentage helps in abstracting as starting point. So I like
to add it too.
>
>>
>>
>> >
>> > Except this proposal,
>> > Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> >
>> > Thanks.
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* RE: [PATCH qedr 04/10] qedr: Add support for PD,PKEY and CQ verbs
From: Amrani, Ram @ 2016-10-10 6:34 UTC (permalink / raw)
To: Leon Romanovsky
Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, Elior, Ariel,
Kalderon, Michal, Mintz, Yuval, Borundia, Rajesh,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20161006133357.GP9282-2ukJVAZIZ/Y@public.gmane.org>
> > +/* * CQE requester status enumeration */ enum
> > +rdma_cqe_requester_status_enum {
> > + RDMA_CQE_REQ_STS_OK,
> > + RDMA_CQE_REQ_STS_BAD_RESPONSE_ERR,
> > + RDMA_CQE_REQ_STS_LOCAL_LENGTH_ERR,
> > + RDMA_CQE_REQ_STS_LOCAL_QP_OPERATION_ERR,
> > + RDMA_CQE_REQ_STS_LOCAL_PROTECTION_ERR,
> > + RDMA_CQE_REQ_STS_MEMORY_MGT_OPERATION_ERR,
> > + RDMA_CQE_REQ_STS_REMOTE_INVALID_REQUEST_ERR,
> > + RDMA_CQE_REQ_STS_REMOTE_ACCESS_ERR,
> > + RDMA_CQE_REQ_STS_REMOTE_OPERATION_ERR,
> > + RDMA_CQE_REQ_STS_RNR_NAK_RETRY_CNT_ERR,
> > + RDMA_CQE_REQ_STS_TRANSPORT_RETRY_CNT_ERR,
> > + RDMA_CQE_REQ_STS_WORK_REQUEST_FLUSHED_ERR,
> > + MAX_RDMA_CQE_REQUESTER_STATUS_ENUM
>
> Please add "," at the last line of enums, it will allow future changes to these
> enums with minimal churn.
This is a good idea and I've applied it to several enums in the patch.
However, I think that this specific enum should left be as is since the last element should always remain last as it is a MAX_*
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH qedr 04/10] qedr: Add support for PD,PKEY and CQ verbs
From: Leon Romanovsky @ 2016-10-10 7:24 UTC (permalink / raw)
To: Amrani, Ram
Cc: dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, Elior, Ariel,
Kalderon, Michal, Mintz, Yuval, Borundia, Rajesh,
linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <SN1PR07MB22070099EE40A67B81D14C75F8DB0-mikhvbZlbf8TSoR2DauN2+FPX92sqiQdvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1328 bytes --]
On Mon, Oct 10, 2016 at 06:34:43AM +0000, Amrani, Ram wrote:
> > > +/* * CQE requester status enumeration */ enum
> > > +rdma_cqe_requester_status_enum {
> > > + RDMA_CQE_REQ_STS_OK,
> > > + RDMA_CQE_REQ_STS_BAD_RESPONSE_ERR,
> > > + RDMA_CQE_REQ_STS_LOCAL_LENGTH_ERR,
> > > + RDMA_CQE_REQ_STS_LOCAL_QP_OPERATION_ERR,
> > > + RDMA_CQE_REQ_STS_LOCAL_PROTECTION_ERR,
> > > + RDMA_CQE_REQ_STS_MEMORY_MGT_OPERATION_ERR,
> > > + RDMA_CQE_REQ_STS_REMOTE_INVALID_REQUEST_ERR,
> > > + RDMA_CQE_REQ_STS_REMOTE_ACCESS_ERR,
> > > + RDMA_CQE_REQ_STS_REMOTE_OPERATION_ERR,
> > > + RDMA_CQE_REQ_STS_RNR_NAK_RETRY_CNT_ERR,
> > > + RDMA_CQE_REQ_STS_TRANSPORT_RETRY_CNT_ERR,
> > > + RDMA_CQE_REQ_STS_WORK_REQUEST_FLUSHED_ERR,
> > > + MAX_RDMA_CQE_REQUESTER_STATUS_ENUM
> >
> > Please add "," at the last line of enums, it will allow future changes to these
> > enums with minimal churn.
>
> This is a good idea and I've applied it to several enums in the patch.
> However, I think that this specific enum should left be as is since the last element should always remain last as it is a MAX_*
It makes sense,
Thanks
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCHv12 0/3] rdmacg: IB/core: rdma controller support
From: Leon Romanovsky @ 2016-10-10 7:33 UTC (permalink / raw)
To: Parav Pandit
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, linux-rdma, Tejun Heo, Li Zefan,
Johannes Weiner, Doug Ledford, Christoph Hellwig, Liran Liss,
Hefty, Sean, Jason Gunthorpe, Haggai Eran,
james.l.morris-QHcLZuEGTsvQT0dZR+AlfA, Or Gerlitz, Matan Barak
In-Reply-To: <CAG53R5UM6nSTZ7=0S9reKGX45CpNBi8soSDVZyXkN-z0_XXWWQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 6486 bytes --]
On Mon, Oct 10, 2016 at 11:59:45AM +0530, Parav Pandit wrote:
> Hi Leon,
>
> On Mon, Oct 10, 2016 at 10:16 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> > On Thu, Oct 06, 2016 at 07:19:24PM +0530, Parav Pandit wrote:
> >> Hi Leon,
> >>
> >> On Wed, Oct 5, 2016 at 4:52 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >> > On Wed, Aug 31, 2016 at 02:07:24PM +0530, Parav Pandit wrote:
> >> >> rdmacg: IB/core: rdma controller support
> >> >>
> >> >> 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.
> >> >
> >> In other cgroup such as CPU, this is done using cpu.weight API. Where
> >> percentage or weight is configured by the user.
> >> In this mode, resources taken away from other cgroup proportionately.
> >> It works for cpu because its mainly stateless resource unlike rdma
> >> resources.
> >> So if we want to simplify user configuration similarly,
> >> percentage/weight configuration can be extended.
> >> This way they need not be introduced to users.
> >> I hope your definition of "user" is actual end-user and not rdma cgroup. Right?
> >
> > Yes, "user" -> "admin".
> > I think that percentage is more intuitive to them and will be much easier to
> > explain how to use it. I always have in mind "swappiness" field and the
> > numerous questions on how to configure it.
> >
> >> In other words, new object should be still added as new enum value in
> >> rdma_cgroup.h?
> >
> > Yes, I had in mind something like IB_CGROUP_HCA, this is why it can be
> > done as a future work after accepting current patches.
> >
> What I meant is,
> today we have RDMACG_VERB_RESOURCE_QP etc,
> We will additionally have RDMACG_VERB_RESOURCE_INDIRECT_TBL etc in
> cgroup_rdma.h.
> So that its available for admin to override it.
IMHO, we are talking about the same. My global HCA object will be
overwritten by more granular VERBS objects in case they exists.
>
> >> Only than it can be overwritten by specific UVERBs type as you
> >> described below. I think thats what you meant as you described below.
> >
> > Exactly.
> >
> >>
> >> Otherwise charging/uncharging this new percentage resource can get messy.
> >
> > Agree
> >
> >>
> >> > This HCA share will be overwritten by specific UVERBS types which you
> >> > already defined.
> >> >
> >> > What do you think?
> >>
> >> So to refine your proposal from cgroup perspective, instead of adding
> >> new resource type in rdma_cgroup.h for percentage, I prefer to have
> >>
> >> Existing
> >> 1. rdma.max
> >> 2. rdma.current
> >> New,
> >> 3. rdma.weight
> >> This ABI will have similar API to say
> >> echo "mlx4_0 50" > rdma.weight.
> >> Where 50 is weight of the resources.
> >> For example,
> >> for one cgroup instance weight=sum=100% resource for a given cgroup.
> >> for three cgroup instances percentage=(weight/sum)% = 50/(50+50+50) = 33%.
> >> One cgroup gets 33% resource.
> >>
> >> Weight can be in range of 1 to 10,000 similar to cpu cgroup.
> >
> > This is exactly what I don't like, the percentage will remove from the
> > user the translation needs between weight and actual limitation.
> >
> > IMHO CPU used weights because everything there is in weights :).
> >
> I admit weight are not very intuitive, I was aligning to the existing
> other cgroup interfaces which achieves similar functionality.
> I will let Tejun approve the "percentage" or "ratio" new file
> interface as its little different than weight.
Sure, let's close the main idea first and see if it makes sense for
other participants.
>
> >>
> >> This might work if applications running in all cgroups are similar.
> >> But weight doesn't do justice, when there are different type of
> >> applications running in each cgroup. Such as few running libfabric
> >> based apps, few running MPI, others directly using ibverbs.
> >> So as you said rdma.max configuration would be required for management
> >> plane to override weight (percentage) for certain resources.
> >
> > Why?
> > The device exposes max values during initialization and if user asked
> > for 20% percent of HCA, he will get max*0.2.
>
> Because every application may not be equivalent of other application.
> For example, some require one to one QP and PD mapping.
> Some share single PD across multiple QPs.
> Some have ratio of 100 MRs per QP, as factor of memory size and operations.
> some servers like to have 1K MRs per QP.
> So if we have just weight, it will equally distributes MRs per QP in
> all cgroup and that either leads to unused resource per cgroup or,
> lesser number of cg instances.
> So fine tuning required for individual one, which we already have.
I afraid that it is over complicating which can be done by curious user
in his user-space scripts: limit the global HCA -> read max values ->
overwrite with specific mapping.
>
> weight or percentage helps in abstracting as starting point. So I like
> to add it too.
Let's start simple
Thanks.
>
>
>
> >
> >>
> >>
> >> >
> >> > Except this proposal,
> >> > Reviewed-by: Leon Romanovsky <leonro-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> >> >
> >> > Thanks.
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> >> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> More majordomo info at http://vger.kernel.org/majordomo-info.html
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCHv12 0/3] rdmacg: IB/core: rdma controller support
From: Parav Pandit @ 2016-10-10 8:35 UTC (permalink / raw)
To: Leon Romanovsky
Cc: cgroups-u79uwXL29TY76Z2rM5mHXA, linux-rdma, Tejun Heo, Li Zefan,
Johannes Weiner, Doug Ledford, Christoph Hellwig, Liran Liss,
Hefty, Sean, Jason Gunthorpe, Haggai Eran,
james.l.morris-QHcLZuEGTsvQT0dZR+AlfA, Or Gerlitz, Matan Barak
In-Reply-To: <20161010073343.GK9282-2ukJVAZIZ/Y@public.gmane.org>
On Mon, Oct 10, 2016 at 1:03 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> On Mon, Oct 10, 2016 at 11:59:45AM +0530, Parav Pandit wrote:
>> Hi Leon,
>>
>> On Mon, Oct 10, 2016 at 10:16 AM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> > On Thu, Oct 06, 2016 at 07:19:24PM +0530, Parav Pandit wrote:
>> >> Hi Leon,
>> >>
>> >> On Wed, Oct 5, 2016 at 4:52 PM, Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>> >> > On Wed, Aug 31, 2016 at 02:07:24PM +0530, Parav Pandit wrote:
>> >> >> rdmacg: IB/core: rdma controller support
>> >> >>
>> >> >> 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.
>> >> >
>> >> In other cgroup such as CPU, this is done using cpu.weight API. Where
>> >> percentage or weight is configured by the user.
>> >> In this mode, resources taken away from other cgroup proportionately.
>> >> It works for cpu because its mainly stateless resource unlike rdma
>> >> resources.
>> >> So if we want to simplify user configuration similarly,
>> >> percentage/weight configuration can be extended.
>> >> This way they need not be introduced to users.
>> >> I hope your definition of "user" is actual end-user and not rdma cgroup. Right?
>> >
>> > Yes, "user" -> "admin".
>> > I think that percentage is more intuitive to them and will be much easier to
>> > explain how to use it. I always have in mind "swappiness" field and the
>> > numerous questions on how to configure it.
>> >
>> >> In other words, new object should be still added as new enum value in
>> >> rdma_cgroup.h?
>> >
>> > Yes, I had in mind something like IB_CGROUP_HCA, this is why it can be
>> > done as a future work after accepting current patches.
>> >
>> What I meant is,
>> today we have RDMACG_VERB_RESOURCE_QP etc,
>> We will additionally have RDMACG_VERB_RESOURCE_INDIRECT_TBL etc in
>> cgroup_rdma.h.
>> So that its available for admin to override it.
>
> IMHO, we are talking about the same. My global HCA object will be
> overwritten by more granular VERBS objects in case they exists.
>
>>
>> >> Only than it can be overwritten by specific UVERBs type as you
>> >> described below. I think thats what you meant as you described below.
>> >
>> > Exactly.
>> >
>> >>
>> >> Otherwise charging/uncharging this new percentage resource can get messy.
>> >
>> > Agree
>> >
>> >>
>> >> > This HCA share will be overwritten by specific UVERBS types which you
>> >> > already defined.
>> >> >
>> >> > What do you think?
>> >>
>> >> So to refine your proposal from cgroup perspective, instead of adding
>> >> new resource type in rdma_cgroup.h for percentage, I prefer to have
>> >>
>> >> Existing
>> >> 1. rdma.max
>> >> 2. rdma.current
>> >> New,
>> >> 3. rdma.weight
>> >> This ABI will have similar API to say
>> >> echo "mlx4_0 50" > rdma.weight.
>> >> Where 50 is weight of the resources.
>> >> For example,
>> >> for one cgroup instance weight=sum=100% resource for a given cgroup.
>> >> for three cgroup instances percentage=(weight/sum)% = 50/(50+50+50) = 33%.
>> >> One cgroup gets 33% resource.
>> >>
>> >> Weight can be in range of 1 to 10,000 similar to cpu cgroup.
>> >
>> > This is exactly what I don't like, the percentage will remove from the
>> > user the translation needs between weight and actual limitation.
>> >
>> > IMHO CPU used weights because everything there is in weights :).
>> >
>> I admit weight are not very intuitive, I was aligning to the existing
>> other cgroup interfaces which achieves similar functionality.
>> I will let Tejun approve the "percentage" or "ratio" new file
>> interface as its little different than weight.
>
> Sure, let's close the main idea first and see if it makes sense for
> other participants.
>
>>
>> >>
>> >> This might work if applications running in all cgroups are similar.
>> >> But weight doesn't do justice, when there are different type of
>> >> applications running in each cgroup. Such as few running libfabric
>> >> based apps, few running MPI, others directly using ibverbs.
>> >> So as you said rdma.max configuration would be required for management
>> >> plane to override weight (percentage) for certain resources.
>> >
>> > Why?
>> > The device exposes max values during initialization and if user asked
>> > for 20% percent of HCA, he will get max*0.2.
>>
>> Because every application may not be equivalent of other application.
>> For example, some require one to one QP and PD mapping.
>> Some share single PD across multiple QPs.
>> Some have ratio of 100 MRs per QP, as factor of memory size and operations.
>> some servers like to have 1K MRs per QP.
>> So if we have just weight, it will equally distributes MRs per QP in
>> all cgroup and that either leads to unused resource per cgroup or,
>> lesser number of cg instances.
>> So fine tuning required for individual one, which we already have.
>
> I afraid that it is over complicating which can be done by curious user
> in his user-space scripts: limit the global HCA -> read max values ->
> overwrite with specific mapping.
>
>>
>> weight or percentage helps in abstracting as starting point. So I like
>> to add it too.
>
> Let's start simple
Yes. I will rebase and test my patch today and see if requires resending.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox