* Re: [PATCH] mlx5: avoid 64-bit division
From: Ariel Levkovich @ 2019-05-28 14:44 UTC (permalink / raw)
To: Leon Romanovsky, Michal Kubecek
Cc: Doug Ledford, Jason Gunthorpe, linux-rdma@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190528140145.GO4633@mtr-leonro.mtl.com>
On Mon, May 20, 2019 at 02:28:35PM +0300, Leon Romanovsky wrote:
> > On Mon, May 20, 2019 at 01:19:02PM +0200, Michal Kubecek wrote:
> > > Commit 25c13324d03d ("IB/mlx5: Add steering SW ICM device memory type")
> > > breaks i386 build by introducing three 64-bit divisions. As the divisor
> > > is MLX5_SW_ICM_BLOCK_SIZE() which is always a power of 2, we can replace
> > > the division with bit operations.
> >
> > Interesting, we tried to solve it differently.
> > I added it to our regression to be on the same side.
> This patch works for us.
Yes, this value is guaranteed to be a power of 2. We safely use round_up() instead as suggested in the patch.
^ permalink raw reply
* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Andrew Murray @ 2019-05-28 14:54 UTC (permalink / raw)
To: Catalin Marinas
Cc: Andrey Konovalov, Mark Rutland, kvm, Szabolcs Nagy, Will Deacon,
dri-devel, linux-mm, linux-kselftest, Felix Kuehling,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Dmitry Vyukov, Dave Martin, Evgeniy Stepanov,
linux-media, Kevin Brodsky, Kees Cook, Ruben Ayrapetyan,
Ramana Radhakrishnan <Ramana.>
In-Reply-To: <20190527143719.GA59948@MBP.local>
On Mon, May 27, 2019 at 03:37:20PM +0100, Catalin Marinas wrote:
> On Mon, May 06, 2019 at 06:30:51PM +0200, Andrey Konovalov wrote:
> > This patch is a part of a series that extends arm64 kernel ABI to allow to
> > pass tagged user pointers (with the top byte set to something else other
> > than 0x00) as syscall arguments.
> >
> > This patch allows tagged pointers to be passed to the following memory
> > syscalls: brk, get_mempolicy, madvise, mbind, mincore, mlock, mlock2,
> > mmap, mmap_pgoff, mprotect, mremap, msync, munlock, munmap,
> > remap_file_pages, shmat and shmdt.
> >
> > This is done by untagging pointers passed to these syscalls in the
> > prologues of their handlers.
> >
> > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
>
> Actually, I don't think any of these wrappers get called (have you
> tested this patch?). Following commit 4378a7d4be30 ("arm64: implement
> syscall wrappers"), I think we have other macro names for overriding the
> sys_* ones.
What is the value in adding these wrappers?
The untagged_addr macro is defined for all in linux/mm.h and these patches
already use untagged_addr in generic code. Thus adding a few more
untagged_addr in the generic syscall handlers (which turn to a nop for most)
is surely better than adding wrappers?
Even if other architectures implement untagged_addr in the future it would
be more consistent if they untagged in the same places and thus not adding
these wrappers enforces that.
Thanks,
Andrew Murray
>
> --
> Catalin
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Catalin Marinas @ 2019-05-28 15:40 UTC (permalink / raw)
To: Andrew Murray
Cc: Andrey Konovalov, Mark Rutland, kvm, Szabolcs Nagy, Will Deacon,
dri-devel, linux-mm, linux-kselftest, Felix Kuehling,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Dmitry Vyukov, Dave Martin, Evgeniy Stepanov,
linux-media, Kevin Brodsky, Kees Cook, Ruben Ayrapetyan,
Ramana Radhakrishnan <Ramana.>
In-Reply-To: <20190528145411.GA709@e119886-lin.cambridge.arm.com>
On Tue, May 28, 2019 at 03:54:11PM +0100, Andrew Murray wrote:
> On Mon, May 27, 2019 at 03:37:20PM +0100, Catalin Marinas wrote:
> > On Mon, May 06, 2019 at 06:30:51PM +0200, Andrey Konovalov wrote:
> > > This patch is a part of a series that extends arm64 kernel ABI to allow to
> > > pass tagged user pointers (with the top byte set to something else other
> > > than 0x00) as syscall arguments.
> > >
> > > This patch allows tagged pointers to be passed to the following memory
> > > syscalls: brk, get_mempolicy, madvise, mbind, mincore, mlock, mlock2,
> > > mmap, mmap_pgoff, mprotect, mremap, msync, munlock, munmap,
> > > remap_file_pages, shmat and shmdt.
> > >
> > > This is done by untagging pointers passed to these syscalls in the
> > > prologues of their handlers.
> > >
> > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> >
> > Actually, I don't think any of these wrappers get called (have you
> > tested this patch?). Following commit 4378a7d4be30 ("arm64: implement
> > syscall wrappers"), I think we have other macro names for overriding the
> > sys_* ones.
>
> What is the value in adding these wrappers?
Not much value, initially proposed just to keep the core changes small.
I'm fine with moving them back in the generic code (but see below).
I think another aspect is how we define the ABI. Is allowing tags to
mlock() for example something specific to arm64 or would sparc ADI need
the same? In the absence of other architectures defining such ABI, my
preference would be to keep the wrappers in the arch code.
Assuming sparc won't implement untagged_addr(), we can place the macros
back in the generic code but, as per the review here, we need to be more
restrictive on where we allow tagged addresses. For example, if mmap()
gets a tagged address with MAP_FIXED, is it expected to return the tag?
My thoughts on allowing tags (quick look):
brk - no
get_mempolicy - yes
madvise - yes
mbind - yes
mincore - yes
mlock, mlock2, munlock - yes
mmap - no (we may change this with MTE but not for TBI)
mmap_pgoff - not used on arm64
mprotect - yes
mremap - yes for old_address, no for new_address (on par with mmap)
msync - yes
munmap - probably no (mmap does not return tagged ptrs)
remap_file_pages - no (also deprecated syscall)
shmat, shmdt - shall we allow tagged addresses on shared memory?
The above is only about the TBI ABI while ignoring hardware MTE. For the
latter, we may want to change the mmap() to allow pre-colouring on page
fault which means that munmap()/mprotect() should also support tagged
pointers. Possibly mremap() as well but we need to decide whether it
should allow re-colouring the page (probably no, in which case
old_address and new_address should have the same tag). For some of these
we'll end up with arm64 specific wrappers again, unless sparc ADI adopts
exactly the same ABI restrictions.
--
Catalin
^ permalink raw reply
* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Dave Martin @ 2019-05-28 15:56 UTC (permalink / raw)
To: Catalin Marinas
Cc: Andrew Murray, Mark Rutland, kvm, Christian Koenig, Szabolcs Nagy,
Will Deacon, dri-devel, linux-mm, Lee Smith, linux-kselftest,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, linux-arm-kernel, Evgeniy Stepanov, linux-media,
Kees Cook, Ruben Ayrapetyan, Andrey Konovalov,
Kevin Brodsky <kevin.brodsk>
In-Reply-To: <20190528154057.GD32006@arrakis.emea.arm.com>
On Tue, May 28, 2019 at 04:40:58PM +0100, Catalin Marinas wrote:
[...]
> My thoughts on allowing tags (quick look):
>
> brk - no
[...]
> mlock, mlock2, munlock - yes
> mmap - no (we may change this with MTE but not for TBI)
[...]
> mprotect - yes
I haven't following this discussion closely... what's the rationale for
the inconsistencies here (feel free to refer me back to the discussion
if it's elsewhere).
Cheers
---Dave
^ permalink raw reply
* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Catalin Marinas @ 2019-05-28 16:34 UTC (permalink / raw)
To: Dave Martin
Cc: Andrew Murray, Mark Rutland, kvm, Christian Koenig, Szabolcs Nagy,
Will Deacon, dri-devel, linux-mm, Lee Smith, linux-kselftest,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, linux-arm-kernel, Evgeniy Stepanov, linux-media,
Kees Cook, Ruben Ayrapetyan, Andrey Konovalov,
Kevin Brodsky <kevin.brodsk>
In-Reply-To: <20190528155644.GD28398@e103592.cambridge.arm.com>
On Tue, May 28, 2019 at 04:56:45PM +0100, Dave P Martin wrote:
> On Tue, May 28, 2019 at 04:40:58PM +0100, Catalin Marinas wrote:
>
> [...]
>
> > My thoughts on allowing tags (quick look):
> >
> > brk - no
>
> [...]
>
> > mlock, mlock2, munlock - yes
> > mmap - no (we may change this with MTE but not for TBI)
>
> [...]
>
> > mprotect - yes
>
> I haven't following this discussion closely... what's the rationale for
> the inconsistencies here (feel free to refer me back to the discussion
> if it's elsewhere).
_My_ rationale (feel free to disagree) is that mmap() by default would
not return a tagged address (ignoring MTE for now). If it gets passed a
tagged address or a "tagged NULL" (for lack of a better name) we don't
have clear semantics of whether the returned address should be tagged in
this ABI relaxation. I'd rather reserve this specific behaviour if we
overload the non-zero tag meaning of mmap() for MTE. Similar reasoning
for mremap(), at least on the new_address argument (not entirely sure
about old_address).
munmap() should probably follow the mmap() rules.
As for brk(), I don't see why the user would need to pass a tagged
address, we can't associate any meaning to this tag.
For the rest, since it's likely such addresses would have been tagged by
malloc() in user space, we should allow tagged pointers.
--
Catalin
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Catalin Marinas @ 2019-05-28 17:02 UTC (permalink / raw)
To: Kees Cook
Cc: enh, Evgenii Stepanov, Andrey Konovalov, Khalid Aziz, Linux ARM,
Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas
In-Reply-To: <201905231327.77CA8D0A36@keescook>
On Thu, May 23, 2019 at 02:31:16PM -0700, Kees Cook wrote:
> syzkaller already attempts to randomly inject non-canonical and
> 0xFFFF....FFFF addresses for user pointers in syscalls in an effort to
> find bugs like CVE-2017-5123 where waitid() via unchecked put_user() was
> able to write directly to kernel memory[1].
>
> It seems that using TBI by default and not allowing a switch back to
> "normal" ABI without a reboot actually means that userspace cannot inject
> kernel pointers into syscalls any more, since they'll get universally
> stripped now. Is my understanding correct, here? i.e. exploiting
> CVE-2017-5123 would be impossible under TBI?
>
> If so, then I think we should commit to the TBI ABI and have a boot
> flag to disable it, but NOT have a process flag, as that would allow
> attackers to bypass the masking. The only flag should be "TBI or MTE".
>
> If so, can I get top byte masking for other architectures too? Like,
> just to strip high bits off userspace addresses? ;)
Just for fun, hack/attempt at your idea which should not interfere with
TBI. Only briefly tested on arm64 (and the s390 __TYPE_IS_PTR macro is
pretty weird ;)):
--------------------------8<---------------------------------
diff --git a/arch/s390/include/asm/compat.h b/arch/s390/include/asm/compat.h
index 63b46e30b2c3..338455a74eff 100644
--- a/arch/s390/include/asm/compat.h
+++ b/arch/s390/include/asm/compat.h
@@ -11,9 +11,6 @@
#include <asm-generic/compat.h>
-#define __TYPE_IS_PTR(t) (!__builtin_types_compatible_p( \
- typeof(0?(__force t)0:0ULL), u64))
-
#define __SC_DELOUSE(t,v) ({ \
BUILD_BUG_ON(sizeof(t) > 4 && !__TYPE_IS_PTR(t)); \
(__force t)(__TYPE_IS_PTR(t) ? ((v) & 0x7fffffff) : (v)); \
diff --git a/include/linux/syscalls.h b/include/linux/syscalls.h
index e2870fe1be5b..b1b9fe8502da 100644
--- a/include/linux/syscalls.h
+++ b/include/linux/syscalls.h
@@ -119,8 +119,15 @@ struct io_uring_params;
#define __TYPE_IS_L(t) (__TYPE_AS(t, 0L))
#define __TYPE_IS_UL(t) (__TYPE_AS(t, 0UL))
#define __TYPE_IS_LL(t) (__TYPE_AS(t, 0LL) || __TYPE_AS(t, 0ULL))
+#define __TYPE_IS_PTR(t) (!__builtin_types_compatible_p(typeof(0 ? (__force t)0 : 0ULL), u64))
#define __SC_LONG(t, a) __typeof(__builtin_choose_expr(__TYPE_IS_LL(t), 0LL, 0L)) a
+#ifdef CONFIG_64BIT
+#define __SC_CAST(t, a) (__TYPE_IS_PTR(t) \
+ ? (__force t) ((__u64)a & ~(1UL << 55)) \
+ : (__force t) a)
+#else
#define __SC_CAST(t, a) (__force t) a
+#endif
#define __SC_ARGS(t, a) a
#define __SC_TEST(t, a) (void)BUILD_BUG_ON_ZERO(!__TYPE_IS_LL(t) && sizeof(t) > sizeof(long))
--
Catalin
^ permalink raw reply related
* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Khalid Aziz @ 2019-05-28 23:33 UTC (permalink / raw)
To: Catalin Marinas, Andrew Murray
Cc: Mark Rutland, kvm, Christian Koenig, Szabolcs Nagy, Will Deacon,
dri-devel, linux-mm, Lee Smith, linux-kselftest,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, linux-arm-kernel, Dave Martin, Evgeniy Stepanov,
linux-media, Kees Cook, Ruben Ayrapetyan, Andrey Konovalov,
Kevin Brodsky, Alex Williamson, Mauro Carvalho Chehab
In-Reply-To: <20190528154057.GD32006@arrakis.emea.arm.com>
On Tue, 2019-05-28 at 16:40 +0100, Catalin Marinas wrote:
> On Tue, May 28, 2019 at 03:54:11PM +0100, Andrew Murray wrote:
> > On Mon, May 27, 2019 at 03:37:20PM +0100, Catalin Marinas wrote:
> > > On Mon, May 06, 2019 at 06:30:51PM +0200, Andrey Konovalov wrote:
> > > > This patch is a part of a series that extends arm64 kernel ABI
> > > > to allow to
> > > > pass tagged user pointers (with the top byte set to something
> > > > else other
> > > > than 0x00) as syscall arguments.
> > > >
> > > > This patch allows tagged pointers to be passed to the following
> > > > memory
> > > > syscalls: brk, get_mempolicy, madvise, mbind, mincore, mlock,
> > > > mlock2,
> > > > mmap, mmap_pgoff, mprotect, mremap, msync, munlock, munmap,
> > > > remap_file_pages, shmat and shmdt.
> > > >
> > > > This is done by untagging pointers passed to these syscalls in
> > > > the
> > > > prologues of their handlers.
> > > >
> > > > Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> > >
> > > Actually, I don't think any of these wrappers get called (have
> > > you
> > > tested this patch?). Following commit 4378a7d4be30 ("arm64:
> > > implement
> > > syscall wrappers"), I think we have other macro names for
> > > overriding the
> > > sys_* ones.
> >
> > What is the value in adding these wrappers?
>
> Not much value, initially proposed just to keep the core changes
> small.
> I'm fine with moving them back in the generic code (but see below).
>
> I think another aspect is how we define the ABI. Is allowing tags to
> mlock() for example something specific to arm64 or would sparc ADI
> need
> the same? In the absence of other architectures defining such ABI, my
> preference would be to keep the wrappers in the arch code.
>
> Assuming sparc won't implement untagged_addr(), we can place the
> macros
> back in the generic code but, as per the review here, we need to be
> more
> restrictive on where we allow tagged addresses. For example, if
> mmap()
> gets a tagged address with MAP_FIXED, is it expected to return the
> tag?
I would recommend against any ABI differences between ARM64 MTE/TBI and
sparc ADI unless it simply can not be helped. My understanding of
MTE/TBI is limited, so I will explain how sparc ADI works. On sparc, a
tagged address has no meaning until following steps happen:
1. set the user mode PSTATE.mcde bit. This acts as the master switch to
enable ADI for a process.
2. set TTE.mcd bit on TLB entries that match the address range ADI is
being enabled on.
3. Store version tag for the range of addresses userspace wants ADI
enabled on using "stxa" instruction. These tags are stored in physical
memory by the memory controller.
Steps 1 and 2 are accomplished by userspace by calling mprotect() with
PROT_ADI. Tags are set by storing tags in a loop, for example:
version = 10;
tmp_addr = shmaddr;
end = shmaddr + BUFFER_SIZE;
while (tmp_addr < end) {
asm volatile(
"stxa %1, [%0]0x90\n\t"
:
: "r" (tmp_addr), "r" (version));
tmp_addr += adi_blksz;
}
With these semantics, giving mmap() or shamat() a tagged address is
meaningless since no tags have been stored at the addresses mmap() will
allocate and one can not store tags before memory range has been
allocated. If we choose to allow tagged addresses to come into mmap()
and shmat(), sparc code can strip the tags unconditionally and that may
help simplify ABI and/or code.
>
> My thoughts on allowing tags (quick look):
>
> brk - no
> get_mempolicy - yes
> madvise - yes
> mbind - yes
> mincore - yes
> mlock, mlock2, munlock - yes
> mmap - no (we may change this with MTE but not for TBI)
> mmap_pgoff - not used on arm64
> mprotect - yes
> mremap - yes for old_address, no for new_address (on par with mmap)
> msync - yes
> munmap - probably no (mmap does not return tagged ptrs)
> remap_file_pages - no (also deprecated syscall)
> shmat, shmdt - shall we allow tagged addresses on shared memory?
>
> The above is only about the TBI ABI while ignoring hardware MTE. For
> the
> latter, we may want to change the mmap() to allow pre-colouring on
> page
> fault which means that munmap()/mprotect() should also support tagged
> pointers. Possibly mremap() as well but we need to decide whether it
> should allow re-colouring the page (probably no, in which case
> old_address and new_address should have the same tag). For some of
> these
> we'll end up with arm64 specific wrappers again, unless sparc ADI
> adopts
> exactly the same ABI restrictions.
>
Let us keep any restrictions common across ARM64 and sparc. pre-
coloring on sparc in the kernel would mean kernel will have to execute
stxa instructions in a loop for each page being faulted in. Not that
big a deal but doesn't that assume the entire page has the same tag
which is dedcued from the upper bits of address? Shouldn't we support
tags at the same granularity level as what the hardware supports? We
went through this discussion for sparc and decision was to support tags
at the same granularity as hardware. That means we can not deduce tags
from the first address that pioints into an mmap or shmat region. Those
tags and the upper bytes of colored address could change for every
cacheline sized block (64-bytes on sparc M7). We can try to store tags
for an entire region in vma but that is expensive, plus on sparc tags
are set in userspace with no participation from kernel and now we need
a way for userspace to communicate the tags to kernel. From sparc point
of view, making kernel responsible for assigning tags to a page on page
fault is full of pitfalls.
Thanks,
Khalid
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Christoph Hellwig @ 2019-05-29 6:11 UTC (permalink / raw)
To: Andrey Konovalov
Cc: Catalin Marinas, Kees Cook, Evgenii Stepanov, Linux ARM,
Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas, Felix Kuehling
In-Reply-To: <CAAeHK+yAUsZWhp6xPAbWewX5Nbw+-G3svUyPmhXu5MVeEDKYvA@mail.gmail.com>
On Tue, May 28, 2019 at 04:14:45PM +0200, Andrey Konovalov wrote:
> Thanks for a lot of valuable input! I've read through all the replies
> and got somewhat lost. What are the changes I need to do to this
> series?
>
> 1. Should I move untagging for memory syscalls back to the generic
> code so other arches would make use of it as well, or should I keep
> the arm64 specific memory syscalls wrappers and address the comments
> on that patch?
It absolutely needs to move to common code. Having arch code leads
to pointless (often unintentional) semantic difference between
architectures, and lots of boilerplate code.
Btw, can anyone of the arm crowd or Khalid comment on the linux-mm
thread on generic gup where I'm dealing with the pre-existing ADI
case of pointer untagging?
^ permalink raw reply
* Re: [PATCH rdma-next v2 05/17] RDMA/counter: Add set/clear per-port auto mode support
From: Leon Romanovsky @ 2019-05-29 10:12 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, RDMA mailing list, Majd Dibbiny, Mark Zhang,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190522165608.GA14554@ziepe.ca>
On Wed, May 22, 2019 at 01:56:08PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 29, 2019 at 11:34:41AM +0300, Leon Romanovsky wrote:
> > From: Mark Zhang <markz@mellanox.com>
> >
> > Add an API to support set/clear per-port auto mode.
> >
> > Signed-off-by: Mark Zhang <markz@mellanox.com>
> > Reviewed-by: Majd Dibbiny <majd@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > drivers/infiniband/core/Makefile | 2 +-
> > drivers/infiniband/core/counters.c | 77 ++++++++++++++++++++++++++++++
> > drivers/infiniband/core/device.c | 4 ++
> > include/rdma/ib_verbs.h | 2 +
> > include/rdma/rdma_counter.h | 24 ++++++++++
> > include/uapi/rdma/rdma_netlink.h | 26 ++++++++++
> > 6 files changed, 134 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/infiniband/core/counters.c
> >
> > diff --git a/drivers/infiniband/core/Makefile b/drivers/infiniband/core/Makefile
> > index 313f2349b518..cddf748c15c9 100644
> > +++ b/drivers/infiniband/core/Makefile
> > @@ -12,7 +12,7 @@ ib_core-y := packer.o ud_header.o verbs.o cq.o rw.o sysfs.o \
> > device.o fmr_pool.o cache.o netlink.o \
> > roce_gid_mgmt.o mr_pool.o addr.o sa_query.o \
> > multicast.o mad.o smi.o agent.o mad_rmpp.o \
> > - nldev.o restrack.o
> > + nldev.o restrack.o counters.o
> >
> > ib_core-$(CONFIG_SECURITY_INFINIBAND) += security.o
> > ib_core-$(CONFIG_CGROUP_RDMA) += cgroup.o
> > diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> > new file mode 100644
> > index 000000000000..bda8d945a758
> > +++ b/drivers/infiniband/core/counters.c
> > @@ -0,0 +1,77 @@
> > +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> > +/*
> > + * Copyright (c) 2019 Mellanox Technologies. All rights reserved.
> > + */
> > +#include <rdma/ib_verbs.h>
> > +#include <rdma/rdma_counter.h>
> > +
> > +#include "core_priv.h"
> > +#include "restrack.h"
> > +
> > +#define ALL_AUTO_MODE_MASKS (RDMA_COUNTER_MASK_QP_TYPE)
> > +
> > +static int __counter_set_mode(struct rdma_counter_mode *curr,
> > + enum rdma_nl_counter_mode new_mode,
> > + enum rdma_nl_counter_mask new_mask)
> > +{
> > + if ((new_mode == RDMA_COUNTER_MODE_AUTO) &&
> > + ((new_mask & (~ALL_AUTO_MODE_MASKS)) ||
> > + (curr->mode != RDMA_COUNTER_MODE_NONE)))
> > + return -EINVAL;
> > +
> > + curr->mode = new_mode;
> > + curr->mask = new_mask;
> > + return 0;
> > +}
> > +
> > +/**
> > + * rdma_counter_set_auto_mode() - Turn on/off per-port auto mode
> > + *
> > + * When @on is true, the @mask must be set
> > + */
> > +int rdma_counter_set_auto_mode(struct ib_device *dev, u8 port,
> > + bool on, enum rdma_nl_counter_mask mask)
> > +{
> > + struct rdma_port_counter *port_counter;
> > + int ret;
> > +
> > + if (!rdma_is_port_valid(dev, port))
> > + return -EINVAL;
> > +
> > + port_counter = &dev->port_data[port].port_counter;
> > + mutex_lock(&port_counter->lock);
> > + if (on) {
> > + ret = __counter_set_mode(&port_counter->mode,
> > + RDMA_COUNTER_MODE_AUTO, mask);
> > + } else {
> > + if (port_counter->mode.mode != RDMA_COUNTER_MODE_AUTO) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > + ret = __counter_set_mode(&port_counter->mode,
> > + RDMA_COUNTER_MODE_NONE, 0);
> > + }
> > +
> > +out:
> > + mutex_unlock(&port_counter->lock);
> > + return ret;
> > +}
> > +
> > +void rdma_counter_init(struct ib_device *dev)
> > +{
> > + struct rdma_port_counter *port_counter;
> > + u32 port;
> > +
> > + if (!dev->ops.alloc_hw_stats)
> > + return;
> > +
> > + rdma_for_each_port(dev, port) {
> > + port_counter = &dev->port_data[port].port_counter;
> > + port_counter->mode.mode = RDMA_COUNTER_MODE_NONE;
> > + mutex_init(&port_counter->lock);
> > + }
> > +}
> > +
> > +void rdma_counter_cleanup(struct ib_device *dev)
> > +{
> > +}
>
> Please don't add empty functions
It is brought here for symmetry, the function is going to be filled in
patch "RDMA/core: Get sum value of all counters when perform a sysfs
stat read".
>
> > @@ -1304,6 +1307,7 @@ static void __ib_unregister_device(struct ib_device *ib_dev)
> > goto out;
> >
> > disable_device(ib_dev);
> > + rdma_counter_cleanup(ib_dev);
>
> This is the wrong place to call this, the patch that actually adds a
> body is just doing kfree's so it is properly called
> 'rdma_counter_release' and it belongs in ib_device_release()
I'll move.
>
> And it shouldn't test hw_stats, and it shouldn't have a 'fail' stanza
> for allocation either.
Not all devices implement hw_stat.
>
> Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 05/17] RDMA/counter: Add set/clear per-port auto mode support
From: Leon Romanovsky @ 2019-05-29 10:43 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, RDMA mailing list, Majd Dibbiny, Mark Zhang,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190529101218.GS4633@mtr-leonro.mtl.com>
On Wed, May 29, 2019 at 01:12:18PM +0300, Leon Romanovsky wrote:
> On Wed, May 22, 2019 at 01:56:08PM -0300, Jason Gunthorpe wrote:
> > On Mon, Apr 29, 2019 at 11:34:41AM +0300, Leon Romanovsky wrote:
> > > From: Mark Zhang <markz@mellanox.com>
> > >
> > > Add an API to support set/clear per-port auto mode.
> > >
> > > Signed-off-by: Mark Zhang <markz@mellanox.com>
> > > Reviewed-by: Majd Dibbiny <majd@mellanox.com>
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > drivers/infiniband/core/Makefile | 2 +-
> > > drivers/infiniband/core/counters.c | 77 ++++++++++++++++++++++++++++++
> > > drivers/infiniband/core/device.c | 4 ++
> > > include/rdma/ib_verbs.h | 2 +
> > > include/rdma/rdma_counter.h | 24 ++++++++++
> > > include/uapi/rdma/rdma_netlink.h | 26 ++++++++++
> > > 6 files changed, 134 insertions(+), 1 deletion(-)
> > > create mode 100644 drivers/infiniband/core/counters.c
> > >
> > > diff --git a/drivers/infiniband/core/Makefile b/drivers/infiniband/core/Makefile
> > > index 313f2349b518..cddf748c15c9 100644
> > > +++ b/drivers/infiniband/core/Makefile
> > > @@ -12,7 +12,7 @@ ib_core-y := packer.o ud_header.o verbs.o cq.o rw.o sysfs.o \
> > > device.o fmr_pool.o cache.o netlink.o \
> > > roce_gid_mgmt.o mr_pool.o addr.o sa_query.o \
> > > multicast.o mad.o smi.o agent.o mad_rmpp.o \
> > > - nldev.o restrack.o
> > > + nldev.o restrack.o counters.o
> > >
> > > ib_core-$(CONFIG_SECURITY_INFINIBAND) += security.o
> > > ib_core-$(CONFIG_CGROUP_RDMA) += cgroup.o
> > > diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> > > new file mode 100644
> > > index 000000000000..bda8d945a758
> > > +++ b/drivers/infiniband/core/counters.c
> > > @@ -0,0 +1,77 @@
> > > +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> > > +/*
> > > + * Copyright (c) 2019 Mellanox Technologies. All rights reserved.
> > > + */
> > > +#include <rdma/ib_verbs.h>
> > > +#include <rdma/rdma_counter.h>
> > > +
> > > +#include "core_priv.h"
> > > +#include "restrack.h"
> > > +
> > > +#define ALL_AUTO_MODE_MASKS (RDMA_COUNTER_MASK_QP_TYPE)
> > > +
> > > +static int __counter_set_mode(struct rdma_counter_mode *curr,
> > > + enum rdma_nl_counter_mode new_mode,
> > > + enum rdma_nl_counter_mask new_mask)
> > > +{
> > > + if ((new_mode == RDMA_COUNTER_MODE_AUTO) &&
> > > + ((new_mask & (~ALL_AUTO_MODE_MASKS)) ||
> > > + (curr->mode != RDMA_COUNTER_MODE_NONE)))
> > > + return -EINVAL;
> > > +
> > > + curr->mode = new_mode;
> > > + curr->mask = new_mask;
> > > + return 0;
> > > +}
> > > +
> > > +/**
> > > + * rdma_counter_set_auto_mode() - Turn on/off per-port auto mode
> > > + *
> > > + * When @on is true, the @mask must be set
> > > + */
> > > +int rdma_counter_set_auto_mode(struct ib_device *dev, u8 port,
> > > + bool on, enum rdma_nl_counter_mask mask)
> > > +{
> > > + struct rdma_port_counter *port_counter;
> > > + int ret;
> > > +
> > > + if (!rdma_is_port_valid(dev, port))
> > > + return -EINVAL;
> > > +
> > > + port_counter = &dev->port_data[port].port_counter;
> > > + mutex_lock(&port_counter->lock);
> > > + if (on) {
> > > + ret = __counter_set_mode(&port_counter->mode,
> > > + RDMA_COUNTER_MODE_AUTO, mask);
> > > + } else {
> > > + if (port_counter->mode.mode != RDMA_COUNTER_MODE_AUTO) {
> > > + ret = -EINVAL;
> > > + goto out;
> > > + }
> > > + ret = __counter_set_mode(&port_counter->mode,
> > > + RDMA_COUNTER_MODE_NONE, 0);
> > > + }
> > > +
> > > +out:
> > > + mutex_unlock(&port_counter->lock);
> > > + return ret;
> > > +}
> > > +
> > > +void rdma_counter_init(struct ib_device *dev)
> > > +{
> > > + struct rdma_port_counter *port_counter;
> > > + u32 port;
> > > +
> > > + if (!dev->ops.alloc_hw_stats)
> > > + return;
> > > +
> > > + rdma_for_each_port(dev, port) {
> > > + port_counter = &dev->port_data[port].port_counter;
> > > + port_counter->mode.mode = RDMA_COUNTER_MODE_NONE;
> > > + mutex_init(&port_counter->lock);
> > > + }
> > > +}
> > > +
> > > +void rdma_counter_cleanup(struct ib_device *dev)
> > > +{
> > > +}
> >
> > Please don't add empty functions
>
> It is brought here for symmetry, the function is going to be filled in
> patch "RDMA/core: Get sum value of all counters when perform a sysfs
> stat read".
>
> >
> > > @@ -1304,6 +1307,7 @@ static void __ib_unregister_device(struct ib_device *ib_dev)
> > > goto out;
> > >
> > > disable_device(ib_dev);
> > > + rdma_counter_cleanup(ib_dev);
> >
> > This is the wrong place to call this, the patch that actually adds a
> > body is just doing kfree's so it is properly called
> > 'rdma_counter_release' and it belongs in ib_device_release()
>
> I'll move.
>
> >
> > And it shouldn't test hw_stats, and it shouldn't have a 'fail' stanza
> > for allocation either.
>
> Not all devices implement hw_stat.
ok, I think that I found a way to rewrite the code without need to check hw_stat.
Thanks
>
> >
> > Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 13/17] RDMA/core: Get sum value of all counters when perform a sysfs stat read
From: Leon Romanovsky @ 2019-05-29 11:05 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, RDMA mailing list, Majd Dibbiny, Mark Zhang,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190522172636.GF15023@ziepe.ca>
On Wed, May 22, 2019 at 02:26:36PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 29, 2019 at 11:34:49AM +0300, Leon Romanovsky wrote:
> > diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> > index c56ffc61ab1e..8ae4906a60e7 100644
> > +++ b/drivers/infiniband/core/device.c
> > @@ -1255,7 +1255,11 @@ int ib_register_device(struct ib_device *device, const char *name)
> > goto dev_cleanup;
> > }
> >
> > - rdma_counter_init(device);
> > + ret = rdma_counter_init(device);
> > + if (ret) {
> > + dev_warn(&device->dev, "Couldn't initialize counter\n");
> > + goto sysfs_cleanup;
> > + }
>
> Don't put this things randomly, if there is some reason it should be
> after sysfs it needs a comment, otherwise if it is just allocating
> memory it belongs earlier, and the unwind should be done in release.
>
> I also think it is very strange/wrong that both sysfs and counters are
> allocating the same alloc_hw_stats object
>
> Why can't they share?
They can, but we wanted to separate "legacy" counters which were exposed
through sysfs and "new" counters which can be enabled/disable automatically.
Thanks
>
> Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 13/17] RDMA/core: Get sum value of all counters when perform a sysfs stat read
From: Leon Romanovsky @ 2019-05-29 11:15 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, RDMA mailing list, Majd Dibbiny, Mark Zhang,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190522171042.GA15023@ziepe.ca>
On Wed, May 22, 2019 at 02:10:42PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 29, 2019 at 11:34:49AM +0300, Leon Romanovsky wrote:
> > From: Mark Zhang <markz@mellanox.com>
> >
> > Since a QP can only be bound to one counter, then if it is bound to a
> > separate counter, for backward compatibility purpose, the statistic
> > value must be:
> > * stat of default counter
> > + stat of all running allocated counters
> > + stat of all deallocated counters (history stats)
> >
> > Signed-off-by: Mark Zhang <markz@mellanox.com>
> > Reviewed-by: Majd Dibbiny <majd@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > drivers/infiniband/core/counters.c | 99 +++++++++++++++++++++++++++++-
> > drivers/infiniband/core/device.c | 8 ++-
> > drivers/infiniband/core/sysfs.c | 10 ++-
> > include/rdma/rdma_counter.h | 5 +-
> > 4 files changed, 113 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> > index 36cd9eca1e46..f598b1cdb241 100644
> > +++ b/drivers/infiniband/core/counters.c
> > @@ -146,6 +146,20 @@ static int __rdma_counter_bind_qp(struct rdma_counter *counter,
> > return ret;
> > }
> >
> > +static void counter_history_stat_update(const struct rdma_counter *counter)
> > +{
> > + struct ib_device *dev = counter->device;
> > + struct rdma_port_counter *port_counter;
> > + int i;
> > +
> > + port_counter = &dev->port_data[counter->port].port_counter;
> > + if (!port_counter->hstats)
> > + return;
> > +
> > + for (i = 0; i < counter->stats->num_counters; i++)
> > + port_counter->hstats->value[i] += counter->stats->value[i];
> > +}
> > +
> > static int __rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> > {
> > struct rdma_counter *counter = qp->counter;
> > @@ -285,8 +299,10 @@ int rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> > return ret;
> >
> > rdma_restrack_put(&counter->res);
> > - if (atomic_dec_and_test(&counter->usecnt))
> > + if (atomic_dec_and_test(&counter->usecnt)) {
> > + counter_history_stat_update(counter);
> > rdma_counter_dealloc(counter);
> > + }
> >
> > return 0;
> > }
> > @@ -307,21 +323,98 @@ int rdma_counter_query_stats(struct rdma_counter *counter)
> > return ret;
> > }
> >
> > -void rdma_counter_init(struct ib_device *dev)
> > +static u64 get_running_counters_hwstat_sum(struct ib_device *dev,
> > + u8 port, u32 index)
> > +{
> > + struct rdma_restrack_entry *res;
> > + struct rdma_restrack_root *rt;
> > + struct rdma_counter *counter;
> > + unsigned long id = 0;
> > + u64 sum = 0;
> > +
> > + rt = &dev->res[RDMA_RESTRACK_COUNTER];
> > + xa_lock(&rt->xa);
> > + xa_for_each(&rt->xa, id, res) {
> > + if (!rdma_restrack_get(res))
> > + continue;
>
> Why do we need to get refcounts if we are holding the xa_lock?
Don't we need to protect an entry itself from disappearing?
>
> > +
> > + counter = container_of(res, struct rdma_counter, res);
> > + if ((counter->device != dev) || (counter->port != port))
> > + goto next;
> > +
> > + if (rdma_counter_query_stats(counter))
> > + goto next;
>
> And rdma_counter_query_stats does
>
> + mutex_lock(&counter->lock);
>
> So this was never tested as it will insta-crash with lockdep.
>
> Presumably this is why it is using xa_for_each and restrack_get - but
> it needs to drop the lock after successful get.
>
> This sort of comment applies to nearly evey place in this series that
> uses xa_for_each.
>
> This needs to be tested with lockdep.
I use LOCKDEP.
>
> Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 13/17] RDMA/core: Get sum value of all counters when perform a sysfs stat read
From: Leon Romanovsky @ 2019-05-29 11:17 UTC (permalink / raw)
To: Doug Ledford, Jason Gunthorpe
Cc: RDMA mailing list, Majd Dibbiny, Mark Zhang, Saeed Mahameed,
linux-netdev
In-Reply-To: <20190429083453.16654-14-leon@kernel.org>
On Mon, Apr 29, 2019 at 11:34:49AM +0300, Leon Romanovsky wrote:
> From: Mark Zhang <markz@mellanox.com>
>
> Since a QP can only be bound to one counter, then if it is bound to a
> separate counter, for backward compatibility purpose, the statistic
> value must be:
> * stat of default counter
> + stat of all running allocated counters
> + stat of all deallocated counters (history stats)
>
> Signed-off-by: Mark Zhang <markz@mellanox.com>
> Reviewed-by: Majd Dibbiny <majd@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
> drivers/infiniband/core/counters.c | 99 +++++++++++++++++++++++++++++-
> drivers/infiniband/core/device.c | 8 ++-
> drivers/infiniband/core/sysfs.c | 10 ++-
> include/rdma/rdma_counter.h | 5 +-
> 4 files changed, 113 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> index 36cd9eca1e46..f598b1cdb241 100644
> --- a/drivers/infiniband/core/counters.c
> +++ b/drivers/infiniband/core/counters.c
> @@ -146,6 +146,20 @@ static int __rdma_counter_bind_qp(struct rdma_counter *counter,
> return ret;
> }
>
> +static void counter_history_stat_update(const struct rdma_counter *counter)
> +{
> + struct ib_device *dev = counter->device;
> + struct rdma_port_counter *port_counter;
> + int i;
> +
> + port_counter = &dev->port_data[counter->port].port_counter;
> + if (!port_counter->hstats)
> + return;
> +
> + for (i = 0; i < counter->stats->num_counters; i++)
> + port_counter->hstats->value[i] += counter->stats->value[i];
> +}
> +
> static int __rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> {
> struct rdma_counter *counter = qp->counter;
> @@ -285,8 +299,10 @@ int rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> return ret;
>
> rdma_restrack_put(&counter->res);
> - if (atomic_dec_and_test(&counter->usecnt))
> + if (atomic_dec_and_test(&counter->usecnt)) {
> + counter_history_stat_update(counter);
> rdma_counter_dealloc(counter);
> + }
>
> return 0;
> }
> @@ -307,21 +323,98 @@ int rdma_counter_query_stats(struct rdma_counter *counter)
> return ret;
> }
>
> -void rdma_counter_init(struct ib_device *dev)
> +static u64 get_running_counters_hwstat_sum(struct ib_device *dev,
> + u8 port, u32 index)
> +{
> + struct rdma_restrack_entry *res;
> + struct rdma_restrack_root *rt;
> + struct rdma_counter *counter;
> + unsigned long id = 0;
> + u64 sum = 0;
> +
> + rt = &dev->res[RDMA_RESTRACK_COUNTER];
> + xa_lock(&rt->xa);
> + xa_for_each(&rt->xa, id, res) {
> + if (!rdma_restrack_get(res))
> + continue;
> +
> + counter = container_of(res, struct rdma_counter, res);
> + if ((counter->device != dev) || (counter->port != port))
> + goto next;
> +
> + if (rdma_counter_query_stats(counter))
> + goto next;
> +
> + sum += counter->stats->value[index];
> +next:
> + rdma_restrack_put(res);
> + }
> +
> + xa_unlock(&rt->xa);
> + return sum;
> +}
> +
> +/**
> + * rdma_counter_get_hwstat_value() - Get the sum value of all counters on a
> + * specific port, including the running ones and history data
> + */
> +u64 rdma_counter_get_hwstat_value(struct ib_device *dev, u8 port, u32 index)
> +{
> + struct rdma_port_counter *port_counter;
> + u64 sum;
> +
> + if (!rdma_is_port_valid(dev, port))
> + return -EINVAL;
> +
> + port_counter = &dev->port_data[port].port_counter;
> + if (index >= port_counter->hstats->num_counters)
> + return -EINVAL;
> +
> + sum = get_running_counters_hwstat_sum(dev, port, index);
> + sum += port_counter->hstats->value[index];
> +
> + return sum;
This function is wrong, it can't return negative value (error) while
return type is declared as u64.
> +}
> +
> +int rdma_counter_init(struct ib_device *dev)
> {
> struct rdma_port_counter *port_counter;
> u32 port;
>
> if (!dev->ops.alloc_hw_stats)
> - return;
> + return 0;
>
> rdma_for_each_port(dev, port) {
> port_counter = &dev->port_data[port].port_counter;
> port_counter->mode.mode = RDMA_COUNTER_MODE_NONE;
> mutex_init(&port_counter->lock);
> +
> + port_counter->hstats = dev->ops.alloc_hw_stats(dev, port);
> + if (!port_counter->hstats)
> + goto fail;
> }
> +
> + return 0;
> +
> +fail:
> + rdma_for_each_port(dev, port) {
> + port_counter = &dev->port_data[port].port_counter;
> + kfree(port_counter->hstats);
> + }
> +
> + return -ENOMEM;
> }
>
> void rdma_counter_cleanup(struct ib_device *dev)
> {
> + struct rdma_port_counter *port_counter;
> + u32 port;
> +
> + if (!dev->ops.alloc_hw_stats)
> + return;
> +
> + rdma_for_each_port(dev, port) {
> + port_counter = &dev->port_data[port].port_counter;
> + kfree(port_counter->hstats);
> + }
> }
> diff --git a/drivers/infiniband/core/device.c b/drivers/infiniband/core/device.c
> index c56ffc61ab1e..8ae4906a60e7 100644
> --- a/drivers/infiniband/core/device.c
> +++ b/drivers/infiniband/core/device.c
> @@ -1255,7 +1255,11 @@ int ib_register_device(struct ib_device *device, const char *name)
> goto dev_cleanup;
> }
>
> - rdma_counter_init(device);
> + ret = rdma_counter_init(device);
> + if (ret) {
> + dev_warn(&device->dev, "Couldn't initialize counter\n");
> + goto sysfs_cleanup;
> + }
>
> ret = enable_device_and_get(device);
> if (ret) {
> @@ -1283,6 +1287,8 @@ int ib_register_device(struct ib_device *device, const char *name)
>
> return 0;
>
> +sysfs_cleanup:
> + ib_device_unregister_sysfs(device);
> dev_cleanup:
> device_del(&device->dev);
> cg_cleanup:
> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> index 2fe89754e592..8d1cf1bbb5f5 100644
> --- a/drivers/infiniband/core/sysfs.c
> +++ b/drivers/infiniband/core/sysfs.c
> @@ -43,6 +43,7 @@
> #include <rdma/ib_mad.h>
> #include <rdma/ib_pma.h>
> #include <rdma/ib_cache.h>
> +#include <rdma/rdma_counter.h>
>
> struct ib_port;
>
> @@ -795,9 +796,12 @@ static int update_hw_stats(struct ib_device *dev, struct rdma_hw_stats *stats,
> return 0;
> }
>
> -static ssize_t print_hw_stat(struct rdma_hw_stats *stats, int index, char *buf)
> +static ssize_t print_hw_stat(struct ib_device *dev, int port_num,
> + struct rdma_hw_stats *stats, int index, char *buf)
> {
> - return sprintf(buf, "%llu\n", stats->value[index]);
> + u64 v = rdma_counter_get_hwstat_value(dev, port_num, index);
> +
> + return sprintf(buf, "%llu\n", stats->value[index] + v);
> }
>
> static ssize_t show_hw_stats(struct kobject *kobj, struct attribute *attr,
> @@ -823,7 +827,7 @@ static ssize_t show_hw_stats(struct kobject *kobj, struct attribute *attr,
> ret = update_hw_stats(dev, stats, hsa->port_num, hsa->index);
> if (ret)
> goto unlock;
> - ret = print_hw_stat(stats, hsa->index, buf);
> + ret = print_hw_stat(dev, hsa->port_num, stats, hsa->index, buf);
> unlock:
> mutex_unlock(&stats->lock);
>
> diff --git a/include/rdma/rdma_counter.h b/include/rdma/rdma_counter.h
> index 4bc62909a638..5ad86ae67cc5 100644
> --- a/include/rdma/rdma_counter.h
> +++ b/include/rdma/rdma_counter.h
> @@ -27,6 +27,7 @@ struct rdma_counter_mode {
>
> struct rdma_port_counter {
> struct rdma_counter_mode mode;
> + struct rdma_hw_stats *hstats;
> struct mutex lock;
> };
>
> @@ -41,13 +42,13 @@ struct rdma_counter {
> u8 port;
> };
>
> -void rdma_counter_init(struct ib_device *dev);
> +int rdma_counter_init(struct ib_device *dev);
> void rdma_counter_cleanup(struct ib_device *dev);
> int rdma_counter_set_auto_mode(struct ib_device *dev, u8 port,
> bool on, enum rdma_nl_counter_mask mask);
> int rdma_counter_bind_qp_auto(struct ib_qp *qp, u8 port);
> int rdma_counter_unbind_qp(struct ib_qp *qp, bool force);
> -
> int rdma_counter_query_stats(struct rdma_counter *counter);
> +u64 rdma_counter_get_hwstat_value(struct ib_device *dev, u8 port, u32 index);
>
> #endif /* _RDMA_COUNTER_H_ */
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH rdma-next v2 11/17] RDMA/netlink: Implement counter dumpit calback
From: Leon Romanovsky @ 2019-05-29 11:31 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, RDMA mailing list, Majd Dibbiny, Mark Zhang,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190522172137.GD15023@ziepe.ca>
On Wed, May 22, 2019 at 02:21:37PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 29, 2019 at 11:34:47AM +0300, Leon Romanovsky wrote:
> > From: Mark Zhang <markz@mellanox.com>
> >
> > This patch adds the ability to return all available counters
> > together with their properties and hwstats.
> >
> > Signed-off-by: Mark Zhang <markz@mellanox.com>
> > Reviewed-by: Majd Dibbiny <majd@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > drivers/infiniband/core/counters.c | 28 +++++
> > drivers/infiniband/core/device.c | 2 +
> > drivers/infiniband/core/nldev.c | 173 +++++++++++++++++++++++++++++
> > include/rdma/ib_verbs.h | 10 ++
> > include/rdma/rdma_counter.h | 3 +
> > include/uapi/rdma/rdma_netlink.h | 10 +-
> > 6 files changed, 225 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> > index 665e0d43c21b..36cd9eca1e46 100644
> > +++ b/drivers/infiniband/core/counters.c
> > @@ -62,6 +62,9 @@ static struct rdma_counter *rdma_counter_alloc(struct ib_device *dev, u8 port,
> > {
> > struct rdma_counter *counter;
> >
> > + if (!dev->ops.counter_alloc_stats)
> > + return NULL;
> > +
>
> Seems weird to add this now, why was it Ok to have counters prior to
> this patch?
Prior to this patch, "sysfs" counters and "netlink" counters were
independent from user perspective.
Thanks
>
> Jason
^ permalink raw reply
* Re: [PATCH rdma-next v2 17/17] RDMA/nldev: Allow get default counter statistics through RDMA netlink
From: Leon Romanovsky @ 2019-05-29 11:54 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, RDMA mailing list, Majd Dibbiny, Mark Zhang,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190522173011.GG15023@ziepe.ca>
On Wed, May 22, 2019 at 02:30:11PM -0300, Jason Gunthorpe wrote:
> On Mon, Apr 29, 2019 at 11:34:53AM +0300, Leon Romanovsky wrote:
> > From: Mark Zhang <markz@mellanox.com>
> >
> > This patch adds the ability to return the hwstats of per-port default
> > counters (which can also be queried through sysfs nodes).
> >
> > Signed-off-by: Mark Zhang <markz@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > drivers/infiniband/core/nldev.c | 101 +++++++++++++++++++++++++++++++-
> > 1 file changed, 99 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/infiniband/core/nldev.c b/drivers/infiniband/core/nldev.c
> > index 53c1d2d82a06..cb2dd38f49f1 100644
> > +++ b/drivers/infiniband/core/nldev.c
> > @@ -1709,6 +1709,98 @@ static int nldev_stat_del_doit(struct sk_buff *skb, struct nlmsghdr *nlh,
> > return ret;
> > }
> >
> > +static int nldev_res_get_default_counter_doit(struct sk_buff *skb,
> > + struct nlmsghdr *nlh,
> > + struct netlink_ext_ack *extack,
> > + struct nlattr *tb[])
> > +{
> > + struct rdma_hw_stats *stats;
> > + struct nlattr *table_attr;
> > + struct ib_device *device;
> > + int ret, num_cnts, i;
> > + struct sk_buff *msg;
> > + u32 index, port;
> > + u64 v;
> > +
> > + if (!tb[RDMA_NLDEV_ATTR_DEV_INDEX] || !tb[RDMA_NLDEV_ATTR_PORT_INDEX])
> > + return -EINVAL;
> > +
> > + index = nla_get_u32(tb[RDMA_NLDEV_ATTR_DEV_INDEX]);
> > + device = ib_device_get_by_index(sock_net(skb->sk), index);
> > + if (!device)
> > + return -EINVAL;
> > +
> > + if (!device->ops.alloc_hw_stats || !device->ops.get_hw_stats) {
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > +
> > + port = nla_get_u32(tb[RDMA_NLDEV_ATTR_PORT_INDEX]);
> > + if (!rdma_is_port_valid(device, port)) {
> > + ret = -EINVAL;
> > + goto err;
> > + }
> > +
> > + msg = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> > + if (!msg) {
> > + ret = -ENOMEM;
> > + goto err;
> > + }
> > +
> > + nlh = nlmsg_put(msg, NETLINK_CB(skb).portid, nlh->nlmsg_seq,
> > + RDMA_NL_GET_TYPE(RDMA_NL_NLDEV,
> > + RDMA_NLDEV_CMD_STAT_GET),
> > + 0, 0);
> > +
> > + if (fill_nldev_handle(msg, device) ||
> > + nla_put_u32(msg, RDMA_NLDEV_ATTR_PORT_INDEX, port)) {
> > + ret = -EMSGSIZE;
> > + goto err_msg;
> > + }
> > +
> > + stats = device->ops.alloc_hw_stats(device, port);
> > + if (!stats) {
> > + ret = -ENOMEM;
> > + goto err_msg;
> > + }
>
> Why do we need yet another one of these to be allocated?
I would say that it is bug.
>
> > + num_cnts = device->ops.get_hw_stats(device, stats, port, 0);
>
> Is '0' right here?
I think that "index" (third parameter) is not used at all.
>
> Jason
^ permalink raw reply
* Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
From: Catalin Marinas @ 2019-05-29 12:12 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Andrey Konovalov, Kees Cook, Evgenii Stepanov, Linux ARM,
Linux Memory Management List, LKML, amd-gfx, dri-devel,
linux-rdma, linux-media, kvm, open list:KERNEL SELFTEST FRAMEWORK,
Vincenzo Frascino, Will Deacon, Mark Rutland, Andrew Morton,
Greg Kroah-Hartman, Yishai Hadas, Felix Kuehling
In-Reply-To: <20190529061126.GA18124@infradead.org>
On Tue, May 28, 2019 at 11:11:26PM -0700, Christoph Hellwig wrote:
> On Tue, May 28, 2019 at 04:14:45PM +0200, Andrey Konovalov wrote:
> > Thanks for a lot of valuable input! I've read through all the replies
> > and got somewhat lost. What are the changes I need to do to this
> > series?
> >
> > 1. Should I move untagging for memory syscalls back to the generic
> > code so other arches would make use of it as well, or should I keep
> > the arm64 specific memory syscalls wrappers and address the comments
> > on that patch?
>
> It absolutely needs to move to common code. Having arch code leads
> to pointless (often unintentional) semantic difference between
> architectures, and lots of boilerplate code.
That's fine by me as long as we agree on the semantics (which shouldn't
be hard; Khalid already following up). We should probably also move the
proposed ABI document [1] into a common place (or part of since we'll
have arm64-specifics like prctl() calls to explicitly opt in to memory
tagging).
[1] https://lore.kernel.org/lkml/20190318163533.26838-1-vincenzo.frascino@arm.com/T/#u
--
Catalin
^ permalink raw reply
* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Dave Martin @ 2019-05-29 12:42 UTC (permalink / raw)
To: Catalin Marinas
Cc: Mark Rutland, kvm, Szabolcs Nagy, Will Deacon, dri-devel,
linux-mm, linux-kselftest, Vincenzo Frascino, Jacob Bramley,
Leon Romanovsky, linux-rdma, amd-gfx, Dmitry Vyukov,
Ramana Radhakrishnan, Evgeniy Stepanov, linux-media, Kees Cook,
Ruben Ayrapetyan, Andrey Konovalov, Kevin Brodsky,
Alex Williamson, Yishai
In-Reply-To: <20190528163400.GE32006@arrakis.emea.arm.com>
On Tue, May 28, 2019 at 05:34:00PM +0100, Catalin Marinas wrote:
> On Tue, May 28, 2019 at 04:56:45PM +0100, Dave P Martin wrote:
> > On Tue, May 28, 2019 at 04:40:58PM +0100, Catalin Marinas wrote:
> >
> > [...]
> >
> > > My thoughts on allowing tags (quick look):
> > >
> > > brk - no
> >
> > [...]
> >
> > > mlock, mlock2, munlock - yes
> > > mmap - no (we may change this with MTE but not for TBI)
> >
> > [...]
> >
> > > mprotect - yes
> >
> > I haven't following this discussion closely... what's the rationale for
> > the inconsistencies here (feel free to refer me back to the discussion
> > if it's elsewhere).
>
> _My_ rationale (feel free to disagree) is that mmap() by default would
> not return a tagged address (ignoring MTE for now). If it gets passed a
> tagged address or a "tagged NULL" (for lack of a better name) we don't
> have clear semantics of whether the returned address should be tagged in
> this ABI relaxation. I'd rather reserve this specific behaviour if we
> overload the non-zero tag meaning of mmap() for MTE. Similar reasoning
> for mremap(), at least on the new_address argument (not entirely sure
> about old_address).
>
> munmap() should probably follow the mmap() rules.
>
> As for brk(), I don't see why the user would need to pass a tagged
> address, we can't associate any meaning to this tag.
>
> For the rest, since it's likely such addresses would have been tagged by
> malloc() in user space, we should allow tagged pointers.
Those arguments seem reasonable. We should try to capture this
somewhere when documenting the ABI.
To be clear, I'm not sure that we should guarantee anywhere that a
tagged pointer is rejected: rather the behaviour should probably be
left unspecified. Then we can tidy it up incrementally.
(The behaviour is unspecified today, in any case.)
Cheers
---Dave
^ permalink raw reply
* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Catalin Marinas @ 2019-05-29 13:23 UTC (permalink / raw)
To: Dave Martin
Cc: Mark Rutland, kvm, Szabolcs Nagy, Will Deacon, dri-devel,
linux-mm, linux-kselftest, Vincenzo Frascino, Jacob Bramley,
Leon Romanovsky, linux-rdma, amd-gfx, Dmitry Vyukov,
Ramana Radhakrishnan, Evgeniy Stepanov, linux-media, Kees Cook,
Ruben Ayrapetyan, Andrey Konovalov, Kevin Brodsky,
Alex Williamson, Yishai
In-Reply-To: <20190529124224.GE28398@e103592.cambridge.arm.com>
On Wed, May 29, 2019 at 01:42:25PM +0100, Dave P Martin wrote:
> On Tue, May 28, 2019 at 05:34:00PM +0100, Catalin Marinas wrote:
> > On Tue, May 28, 2019 at 04:56:45PM +0100, Dave P Martin wrote:
> > > On Tue, May 28, 2019 at 04:40:58PM +0100, Catalin Marinas wrote:
> > >
> > > [...]
> > >
> > > > My thoughts on allowing tags (quick look):
> > > >
> > > > brk - no
> > >
> > > [...]
> > >
> > > > mlock, mlock2, munlock - yes
> > > > mmap - no (we may change this with MTE but not for TBI)
> > >
> > > [...]
> > >
> > > > mprotect - yes
> > >
> > > I haven't following this discussion closely... what's the rationale for
> > > the inconsistencies here (feel free to refer me back to the discussion
> > > if it's elsewhere).
> >
> > _My_ rationale (feel free to disagree) is that mmap() by default would
> > not return a tagged address (ignoring MTE for now). If it gets passed a
> > tagged address or a "tagged NULL" (for lack of a better name) we don't
> > have clear semantics of whether the returned address should be tagged in
> > this ABI relaxation. I'd rather reserve this specific behaviour if we
> > overload the non-zero tag meaning of mmap() for MTE. Similar reasoning
> > for mremap(), at least on the new_address argument (not entirely sure
> > about old_address).
> >
> > munmap() should probably follow the mmap() rules.
> >
> > As for brk(), I don't see why the user would need to pass a tagged
> > address, we can't associate any meaning to this tag.
> >
> > For the rest, since it's likely such addresses would have been tagged by
> > malloc() in user space, we should allow tagged pointers.
>
> Those arguments seem reasonable. We should try to capture this
> somewhere when documenting the ABI.
>
> To be clear, I'm not sure that we should guarantee anywhere that a
> tagged pointer is rejected: rather the behaviour should probably be
> left unspecified. Then we can tidy it up incrementally.
>
> (The behaviour is unspecified today, in any case.)
What is specified (or rather de-facto ABI) today is that passing a user
address above TASK_SIZE (e.g. non-zero top byte) would fail in most
cases. If we relax this with the TBI we may end up with some de-facto
ABI before we actually get MTE hardware. Tightening it afterwards may be
slightly more problematic, although MTE needs to be an explicit opt-in.
IOW, I wouldn't want to unnecessarily relax the ABI if we don't need to.
--
Catalin
^ permalink raw reply
* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Catalin Marinas @ 2019-05-29 14:20 UTC (permalink / raw)
To: Khalid Aziz
Cc: Andrew Murray, Andrey Konovalov, Mark Rutland, kvm, Szabolcs Nagy,
Will Deacon, dri-devel, linux-mm, linux-kselftest, Felix Kuehling,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Dmitry Vyukov, Dave Martin, Evgeniy Stepanov,
linux-media, Kevin Brodsky, Kees Cook,
Ruben Ayrapetyan <Ruben.Ayrapetyan@
In-Reply-To: <11193998209cc6ff34e7d704f081206b8787b174.camel@oracle.com>
Hi Khalid,
On Tue, May 28, 2019 at 05:33:04PM -0600, Khalid Aziz wrote:
> On Tue, 2019-05-28 at 16:40 +0100, Catalin Marinas wrote:
> > I think another aspect is how we define the ABI. Is allowing tags to
> > mlock() for example something specific to arm64 or would sparc ADI
> > need the same? In the absence of other architectures defining such
> > ABI, my preference would be to keep the wrappers in the arch code.
> >
> > Assuming sparc won't implement untagged_addr(), we can place the
> > macros back in the generic code but, as per the review here, we need
> > to be more restrictive on where we allow tagged addresses. For
> > example, if mmap() gets a tagged address with MAP_FIXED, is it
> > expected to return the tag?
>
> I would recommend against any ABI differences between ARM64 MTE/TBI and
> sparc ADI unless it simply can not be helped. My understanding of
> MTE/TBI is limited, so I will explain how sparc ADI works. On sparc, a
> tagged address has no meaning until following steps happen:
Before we go into the MTE/ADI similarities or differences, just to
clarify that TBI is something that we supported from the start of the
arm64 kernel port. TBI (top byte ignore) allows a user pointer to have
non-zero top byte and dereference it without causing a fault (the
hardware masks it out). The user/kernel ABI does not allow such tagged
pointers into the kernel, nor would the kernel return any such tagged
addresses.
With MTE (memory tagging extensions), the top-byte meaning is changed
from no longer being ignored to actually being checked against a tag in
the physical RAM (we call it allocation tag).
> 1. set the user mode PSTATE.mcde bit. This acts as the master switch to
> enable ADI for a process.
>
> 2. set TTE.mcd bit on TLB entries that match the address range ADI is
> being enabled on.
Something close enough for MTE, with the difference that enabling it is
not a PSTATE bit but rather a system control bit (SCTLR_EL1 register),
so only the kernel can turn it on/off for the user.
> 3. Store version tag for the range of addresses userspace wants ADI
> enabled on using "stxa" instruction. These tags are stored in physical
> memory by the memory controller.
Do you have an "ldxa" instruction to load the tags from physical memory?
> Steps 1 and 2 are accomplished by userspace by calling mprotect() with
> PROT_ADI. Tags are set by storing tags in a loop, for example:
>
> version = 10;
> tmp_addr = shmaddr;
> end = shmaddr + BUFFER_SIZE;
> while (tmp_addr < end) {
> asm volatile(
> "stxa %1, [%0]0x90\n\t"
> :
> : "r" (tmp_addr), "r" (version));
> tmp_addr += adi_blksz;
> }
On arm64, a sequence similar to the above would live in the libc. So a
malloc() call will tag the memory and return the tagged address to the
user.
We were not planning for a PROT_ADI/MTE but rather have MTE enabled for
all user memory ranges. We may revisit this before we upstream the MTE
support (probably some marginal benefit for the hardware not fetching
the tags from memory if we don't need to, e.g. code sections).
Given that we already have the TBI feature and with MTE enabled the top
byte is no longer ignored, we are planning for an explicit opt-in by the
user via prctl() to enable MTE.
> With these semantics, giving mmap() or shamat() a tagged address is
> meaningless since no tags have been stored at the addresses mmap() will
> allocate and one can not store tags before memory range has been
> allocated. If we choose to allow tagged addresses to come into mmap()
> and shmat(), sparc code can strip the tags unconditionally and that may
> help simplify ABI and/or code.
We could say that with TBI (pre-MTE support), the top byte is actually
ignored on mmap(). Now, if you pass a MAP_FIXED with a tagged address,
should the user expect the same tagged address back or stripping the tag
is acceptable? If we want to keep the current mmap() semantics, I'd say
the same tag is returned. However, with MTE this also implies that the
memory was coloured.
> > My thoughts on allowing tags (quick look):
> >
> > brk - no
> > get_mempolicy - yes
> > madvise - yes
> > mbind - yes
> > mincore - yes
> > mlock, mlock2, munlock - yes
> > mmap - no (we may change this with MTE but not for TBI)
> > mmap_pgoff - not used on arm64
> > mprotect - yes
> > mremap - yes for old_address, no for new_address (on par with mmap)
> > msync - yes
> > munmap - probably no (mmap does not return tagged ptrs)
> > remap_file_pages - no (also deprecated syscall)
> > shmat, shmdt - shall we allow tagged addresses on shared memory?
> >
> > The above is only about the TBI ABI while ignoring hardware MTE. For
> > the latter, we may want to change the mmap() to allow pre-colouring
> > on page fault which means that munmap()/mprotect() should also
> > support tagged pointers. Possibly mremap() as well but we need to
> > decide whether it should allow re-colouring the page (probably no,
> > in which case old_address and new_address should have the same tag).
> > For some of these we'll end up with arm64 specific wrappers again,
> > unless sparc ADI adopts exactly the same ABI restrictions.
>
> Let us keep any restrictions common across ARM64 and sparc. pre-
> coloring on sparc in the kernel would mean kernel will have to execute
> stxa instructions in a loop for each page being faulted in.
Since the user can probe the pre-existing colour in a faulted-in page
(either with some 'ldxa' instruction or by performing a tag-checked
access), the kernel should always pre-colour (even if colour 0) any
allocated page. There might not be an obvious security risk but I feel
uneasy about letting colours leak between address spaces (different user
processes or between kernel and user).
Since we already need such loop in the kernel, we might as well allow
user space to require a certain colour. This comes in handy for large
malloc() and another advantage is that the C library won't be stuck
trying to paint the whole range (think GB).
> Not that big a deal but doesn't that assume the entire page has the
> same tag which is dedcued from the upper bits of address? Shouldn't we
> support tags at the same granularity level as what the hardware
> supports?
That's mostly about large malloc() optimisation via mmap(), the latter
working on page granularity already. There is another use-case for
pre-coloured thread stacks, also allocated via anonymous mmap().
> We went through this discussion for sparc and decision was to support
> tags at the same granularity as hardware. That means we can not deduce
> tags from the first address that pioints into an mmap or shmat region.
> Those tags and the upper bytes of colored address could change for
> every cacheline sized block (64-bytes on sparc M7).
It's 16-byte for arm64, so smaller than the cacheline.
> We can try to store tags for an entire region in vma but that is
> expensive, plus on sparc tags are set in userspace with no
> participation from kernel and now we need a way for userspace to
> communicate the tags to kernel.
We can't support finer granularity through the mmap() syscall and, as
you said, the vma is not the right thing to store the individual tags.
With the above extension to mmap(), we'd have to store a colour per vma
and prevent merging if different colours (we could as well use the
pkeys mechanism we already have in the kernel but use a colour per vma
instead of a key).
Of course, the user is allowed to change the in-memory colours at a
finer granularity and the kernel will preserve them during swapping
out/in, page migration etc. The above mmap() proposal is just for the
first fault-in of a page in a given range/vma.
> From sparc point of view, making kernel responsible for assigning tags
> to a page on page fault is full of pitfalls.
This could be just some arm64-specific but if you plan to deploy it more
generically for sparc (at the C library level), you may find this
useful.
--
Catalin
^ permalink raw reply
* Re: [PATCH v15 01/17] uaccess: add untagged_addr definition for other arches
From: Khalid Aziz @ 2019-05-29 14:49 UTC (permalink / raw)
To: Andrey Konovalov, linux-arm-kernel, linux-mm, linux-kernel,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm, linux-kselftest
Cc: Mark Rutland, Szabolcs Nagy, Catalin Marinas, Will Deacon,
Kostya Serebryany, Felix Kuehling, Vincenzo Frascino,
Jacob Bramley, Leon Romanovsky, Dave Martin, Evgeniy Stepanov,
Kevin Brodsky, Kees Cook, Ruben Ayrapetyan, Lee Smith,
Alex Williamson, Mauro Carvalho Chehab, Dmitry Vyukov,
Greg Kroah-Hartman, Yishai Hadas, Jens Wiklander,
Ramana Radhakrishnan
In-Reply-To: <67ae3bd92e590d42af22ef2de0ad37b730a13837.1557160186.git.andreyknvl@google.com>
On Mon, 2019-05-06 at 18:30 +0200, Andrey Konovalov wrote:
> To allow arm64 syscalls to accept tagged pointers from userspace, we
> must
> untag them when they are passed to the kernel. Since untagging is
> done in
> generic parts of the kernel, the untagged_addr macro needs to be
> defined
> for all architectures.
>
> Define it as a noop for architectures other than arm64.
>
> Acked-by: Catalin Marinas <catalin.marinas@arm.com>
> Signed-off-by: Andrey Konovalov <andreyknvl@google.com>
> ---
> include/linux/mm.h | 4 ++++
> 1 file changed, 4 insertions(+)
As discussed in the other thread Chris started, there is a generic need
to untag addresses in kernel and this patch gets us ready for that.
Reviewed-by: Khalid Aziz <khalid.aziz@oracle.com>
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6b10c21630f5..44041df804a6 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -99,6 +99,10 @@ extern int mmap_rnd_compat_bits __read_mostly;
> #include <asm/pgtable.h>
> #include <asm/processor.h>
>
> +#ifndef untagged_addr
> +#define untagged_addr(addr) (addr)
> +#endif
> +
> #ifndef __pa_symbol
> #define __pa_symbol(x) __pa(RELOC_HIDE((unsigned long)(x), 0))
> #endif
^ permalink raw reply
* [PATCH][next] IB/rdmavt: Use struct_size() helper
From: Gustavo A. R. Silva @ 2019-05-29 15:12 UTC (permalink / raw)
To: Dennis Dalessandro, Mike Marciniszyn, Doug Ledford,
Jason Gunthorpe
Cc: linux-rdma, linux-kernel, Gustavo A. R. Silva
Make use of the struct_size() helper instead of an open-coded version
in order to avoid any potential type mistakes, in particular in the
context in which this code is being used.
So, replace the following form:
sizeof(struct rvt_sge) * init_attr->cap.max_send_sge + sizeof(struct rvt_swqe)
with:
struct_size(swq, sg_list, init_attr->cap.max_send_sge)
and so on...
Also, notice that variable size is unnecessary, hence it is removed.
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/infiniband/sw/rdmavt/qp.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/infiniband/sw/rdmavt/qp.c b/drivers/infiniband/sw/rdmavt/qp.c
index 31a2e65e4906..a60f5faea198 100644
--- a/drivers/infiniband/sw/rdmavt/qp.c
+++ b/drivers/infiniband/sw/rdmavt/qp.c
@@ -988,9 +988,7 @@ struct ib_qp *rvt_create_qp(struct ib_pd *ibpd,
case IB_QPT_UC:
case IB_QPT_RC:
case IB_QPT_UD:
- sz = sizeof(struct rvt_sge) *
- init_attr->cap.max_send_sge +
- sizeof(struct rvt_swqe);
+ sz = struct_size(swq, sg_list, init_attr->cap.max_send_sge);
swq = vzalloc_node(array_size(sz, sqsize), rdi->dparms.node);
if (!swq)
return ERR_PTR(-ENOMEM);
--
2.21.0
^ permalink raw reply related
* [PATCH][next] IB/qib: Use struct_size() helper
From: Gustavo A. R. Silva @ 2019-05-29 15:13 UTC (permalink / raw)
To: Dennis Dalessandro, Mike Marciniszyn, Doug Ledford,
Jason Gunthorpe
Cc: linux-rdma, linux-kernel, Gustavo A. R. Silva
Make use of the struct_size() helper instead of an open-coded version
in order to avoid any potential type mistakes, in particular in the
context in which this code is being used.
So, replace the following form:
sizeof(*pkt) + sizeof(pkt->addr[0])*n
with:
struct_size(pkt, addr, n)
Also, notice that variable size is unnecessary, hence it is removed.
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/infiniband/hw/qib/qib_user_sdma.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/infiniband/hw/qib/qib_user_sdma.c b/drivers/infiniband/hw/qib/qib_user_sdma.c
index 0c204776263f..97649f64e09e 100644
--- a/drivers/infiniband/hw/qib/qib_user_sdma.c
+++ b/drivers/infiniband/hw/qib/qib_user_sdma.c
@@ -904,10 +904,11 @@ static int qib_user_sdma_queue_pkts(const struct qib_devdata *dd,
}
if (frag_size) {
- int pktsize, tidsmsize, n;
+ int tidsmsize, n;
+ size_t pktsize;
n = npages*((2*PAGE_SIZE/frag_size)+1);
- pktsize = sizeof(*pkt) + sizeof(pkt->addr[0])*n;
+ pktsize = struct_size(pkt, addr, n);
/*
* Determine if this is tid-sdma or just sdma.
--
2.21.0
^ permalink raw reply related
* [PATCH][next] IB/hfi1: Use struct_size() helper
From: Gustavo A. R. Silva @ 2019-05-29 15:15 UTC (permalink / raw)
To: Mike Marciniszyn, Dennis Dalessandro, Doug Ledford,
Jason Gunthorpe
Cc: linux-rdma, linux-kernel, Gustavo A. R. Silva
Make use of the struct_size() helper instead of an open-coded version
in order to avoid any potential type mistakes, in particular in the
context in which this code is being used.
So, replace the following form:
sizeof(struct opa_port_status_rsp) + num_vls * sizeof(struct _vls_pctrs)
with:
struct_size(rsp, vls, num_vls)
and so on...
Also, notice that variable size is unnecessary, hence it is removed.
This code was detected with the help of Coccinelle.
Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
---
drivers/infiniband/hw/hfi1/mad.c | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/infiniband/hw/hfi1/mad.c b/drivers/infiniband/hw/hfi1/mad.c
index 4228393e6c4c..184dba3c2828 100644
--- a/drivers/infiniband/hw/hfi1/mad.c
+++ b/drivers/infiniband/hw/hfi1/mad.c
@@ -2744,8 +2744,7 @@ static int pma_get_opa_portstatus(struct opa_pma_mad *pmp,
u16 link_width;
u16 link_speed;
- response_data_size = sizeof(struct opa_port_status_rsp) +
- num_vls * sizeof(struct _vls_pctrs);
+ response_data_size = struct_size(rsp, vls, num_vls);
if (response_data_size > sizeof(pmp->data)) {
pmp->mad_hdr.status |= OPA_PM_STATUS_REQUEST_TOO_LARGE;
return reply((struct ib_mad_hdr *)pmp);
@@ -3014,8 +3013,7 @@ static int pma_get_opa_datacounters(struct opa_pma_mad *pmp,
}
/* Sanity check */
- response_data_size = sizeof(struct opa_port_data_counters_msg) +
- num_vls * sizeof(struct _vls_dctrs);
+ response_data_size = struct_size(req, port[0].vls, num_vls);
if (response_data_size > sizeof(pmp->data)) {
pmp->mad_hdr.status |= IB_SMP_INVALID_FIELD;
@@ -3232,8 +3230,7 @@ static int pma_get_opa_porterrors(struct opa_pma_mad *pmp,
return reply((struct ib_mad_hdr *)pmp);
}
- response_data_size = sizeof(struct opa_port_error_counters64_msg) +
- num_vls * sizeof(struct _vls_ectrs);
+ response_data_size = struct_size(req, port[0].vls, num_vls);
if (response_data_size > sizeof(pmp->data)) {
pmp->mad_hdr.status |= IB_SMP_INVALID_FIELD;
--
2.21.0
^ permalink raw reply related
* Re: [PATCH v15 05/17] arms64: untag user pointers passed to memory syscalls
From: Dave Martin @ 2019-05-29 15:18 UTC (permalink / raw)
To: Catalin Marinas
Cc: Mark Rutland, kvm, Christian Koenig, Szabolcs Nagy, Will Deacon,
dri-devel, linux-mm, Lee Smith, linux-kselftest,
Vincenzo Frascino, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, linux-arm-kernel, Evgeniy Stepanov, linux-media,
Kees Cook, Ruben Ayrapetyan, Andrey Konovalov, Kevin Brodsky,
Alex Williamson <alex.willi>
In-Reply-To: <20190529132341.27t3knoxpb7t7y3g@mbp>
On Wed, May 29, 2019 at 02:23:42PM +0100, Catalin Marinas wrote:
> On Wed, May 29, 2019 at 01:42:25PM +0100, Dave P Martin wrote:
> > On Tue, May 28, 2019 at 05:34:00PM +0100, Catalin Marinas wrote:
> > > On Tue, May 28, 2019 at 04:56:45PM +0100, Dave P Martin wrote:
> > > > On Tue, May 28, 2019 at 04:40:58PM +0100, Catalin Marinas wrote:
> > > >
> > > > [...]
> > > >
> > > > > My thoughts on allowing tags (quick look):
> > > > >
> > > > > brk - no
> > > >
> > > > [...]
> > > >
> > > > > mlock, mlock2, munlock - yes
> > > > > mmap - no (we may change this with MTE but not for TBI)
> > > >
> > > > [...]
> > > >
> > > > > mprotect - yes
> > > >
> > > > I haven't following this discussion closely... what's the rationale for
> > > > the inconsistencies here (feel free to refer me back to the discussion
> > > > if it's elsewhere).
> > >
> > > _My_ rationale (feel free to disagree) is that mmap() by default would
> > > not return a tagged address (ignoring MTE for now). If it gets passed a
> > > tagged address or a "tagged NULL" (for lack of a better name) we don't
> > > have clear semantics of whether the returned address should be tagged in
> > > this ABI relaxation. I'd rather reserve this specific behaviour if we
> > > overload the non-zero tag meaning of mmap() for MTE. Similar reasoning
> > > for mremap(), at least on the new_address argument (not entirely sure
> > > about old_address).
> > >
> > > munmap() should probably follow the mmap() rules.
> > >
> > > As for brk(), I don't see why the user would need to pass a tagged
> > > address, we can't associate any meaning to this tag.
> > >
> > > For the rest, since it's likely such addresses would have been tagged by
> > > malloc() in user space, we should allow tagged pointers.
> >
> > Those arguments seem reasonable. We should try to capture this
> > somewhere when documenting the ABI.
> >
> > To be clear, I'm not sure that we should guarantee anywhere that a
> > tagged pointer is rejected: rather the behaviour should probably be
> > left unspecified. Then we can tidy it up incrementally.
> >
> > (The behaviour is unspecified today, in any case.)
>
> What is specified (or rather de-facto ABI) today is that passing a user
> address above TASK_SIZE (e.g. non-zero top byte) would fail in most
> cases. If we relax this with the TBI we may end up with some de-facto
I may be being too picky, but "would fail in most cases" sounds like
"unspecified" ?
> ABI before we actually get MTE hardware. Tightening it afterwards may be
> slightly more problematic, although MTE needs to be an explicit opt-in.
>
> IOW, I wouldn't want to unnecessarily relax the ABI if we don't need to.
So long we don't block foreseeable future developments unnecessarily
either -- I agree there's a balance to be struck.
I guess this can be reviewed when we have nailed down the details a bit
further.
Cheers
---Dave
^ permalink raw reply
* Re: [PATCH rdma-next v2 13/17] RDMA/core: Get sum value of all counters when perform a sysfs stat read
From: Jason Gunthorpe @ 2019-05-29 15:41 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Doug Ledford, RDMA mailing list, Majd Dibbiny, Mark Zhang,
Saeed Mahameed, linux-netdev
In-Reply-To: <20190529111544.GV4633@mtr-leonro.mtl.com>
On Wed, May 29, 2019 at 02:15:44PM +0300, Leon Romanovsky wrote:
> On Wed, May 22, 2019 at 02:10:42PM -0300, Jason Gunthorpe wrote:
> > On Mon, Apr 29, 2019 at 11:34:49AM +0300, Leon Romanovsky wrote:
> > > From: Mark Zhang <markz@mellanox.com>
> > >
> > > Since a QP can only be bound to one counter, then if it is bound to a
> > > separate counter, for backward compatibility purpose, the statistic
> > > value must be:
> > > * stat of default counter
> > > + stat of all running allocated counters
> > > + stat of all deallocated counters (history stats)
> > >
> > > Signed-off-by: Mark Zhang <markz@mellanox.com>
> > > Reviewed-by: Majd Dibbiny <majd@mellanox.com>
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > drivers/infiniband/core/counters.c | 99 +++++++++++++++++++++++++++++-
> > > drivers/infiniband/core/device.c | 8 ++-
> > > drivers/infiniband/core/sysfs.c | 10 ++-
> > > include/rdma/rdma_counter.h | 5 +-
> > > 4 files changed, 113 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/core/counters.c b/drivers/infiniband/core/counters.c
> > > index 36cd9eca1e46..f598b1cdb241 100644
> > > +++ b/drivers/infiniband/core/counters.c
> > > @@ -146,6 +146,20 @@ static int __rdma_counter_bind_qp(struct rdma_counter *counter,
> > > return ret;
> > > }
> > >
> > > +static void counter_history_stat_update(const struct rdma_counter *counter)
> > > +{
> > > + struct ib_device *dev = counter->device;
> > > + struct rdma_port_counter *port_counter;
> > > + int i;
> > > +
> > > + port_counter = &dev->port_data[counter->port].port_counter;
> > > + if (!port_counter->hstats)
> > > + return;
> > > +
> > > + for (i = 0; i < counter->stats->num_counters; i++)
> > > + port_counter->hstats->value[i] += counter->stats->value[i];
> > > +}
> > > +
> > > static int __rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> > > {
> > > struct rdma_counter *counter = qp->counter;
> > > @@ -285,8 +299,10 @@ int rdma_counter_unbind_qp(struct ib_qp *qp, bool force)
> > > return ret;
> > >
> > > rdma_restrack_put(&counter->res);
> > > - if (atomic_dec_and_test(&counter->usecnt))
> > > + if (atomic_dec_and_test(&counter->usecnt)) {
> > > + counter_history_stat_update(counter);
> > > rdma_counter_dealloc(counter);
> > > + }
> > >
> > > return 0;
> > > }
> > > @@ -307,21 +323,98 @@ int rdma_counter_query_stats(struct rdma_counter *counter)
> > > return ret;
> > > }
> > >
> > > -void rdma_counter_init(struct ib_device *dev)
> > > +static u64 get_running_counters_hwstat_sum(struct ib_device *dev,
> > > + u8 port, u32 index)
> > > +{
> > > + struct rdma_restrack_entry *res;
> > > + struct rdma_restrack_root *rt;
> > > + struct rdma_counter *counter;
> > > + unsigned long id = 0;
> > > + u64 sum = 0;
> > > +
> > > + rt = &dev->res[RDMA_RESTRACK_COUNTER];
> > > + xa_lock(&rt->xa);
> > > + xa_for_each(&rt->xa, id, res) {
> > > + if (!rdma_restrack_get(res))
> > > + continue;
> >
> > Why do we need to get refcounts if we are holding the xa_lock?
>
> Don't we need to protect an entry itself from disappearing?
xa_lock prevents xa_erase and xa_erase should be done before any
parallel kfree.
Jason
^ 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