* [PATCH v3 hmm 00/12] mm/hmm: Various revisions from a locking/code review
From: Jason Gunthorpe @ 2019-06-14 0:44 UTC (permalink / raw)
To: Jerome Glisse, Ralph Campbell, John Hubbard, Felix.Kuehling
Cc: Andrea Arcangeli, linux-rdma, amd-gfx, linux-mm, Jason Gunthorpe,
dri-devel, Ben Skeggs
From: Jason Gunthorpe <jgg@mellanox.com>
This patch series arised out of discussions with Jerome when looking at the
ODP changes, particularly informed by use after free races we have already
found and fixed in the ODP code (thanks to syzkaller) working with mmu
notifiers, and the discussion with Ralph on how to resolve the lifetime model.
Overall this brings in a simplified locking scheme and easy to explain
lifetime model:
If a hmm_range is valid, then the hmm is valid, if a hmm is valid then the mm
is allocated memory.
If the mm needs to still be alive (ie to lock the mmap_sem, find a vma, etc)
then the mmget must be obtained via mmget_not_zero().
The use unlocked reads on 'hmm->dead' are also eliminated in favour of using
standard mmget() locking to prevent the mm from being released. Many of the
debugging checks of !range->hmm and !hmm->mm are dropped in favour of poison -
which is much clearer as to the lifetime intent.
The trailing patches are just some random cleanups I noticed when reviewing
this code.
I would like to run some testing with the ODP patch, but haven't
yet. Otherwise I think this is reviewed enough, and if there is nothing more
say I hope to apply it next week.
I plan to continue to work on the idea with CH to move more of this mirror
code into mmu notifiers and other places, but this will take some time and
research.
Thanks to everyone who took time to look at this!
Jason Gunthorpe (12):
mm/hmm: fix use after free with struct hmm in the mmu notifiers
mm/hmm: Use hmm_mirror not mm as an argument for hmm_range_register
mm/hmm: Hold a mmgrab from hmm to mm
mm/hmm: Simplify hmm_get_or_create and make it reliable
mm/hmm: Remove duplicate condition test before wait_event_timeout
mm/hmm: Hold on to the mmget for the lifetime of the range
mm/hmm: Use lockdep instead of comments
mm/hmm: Remove racy protection against double-unregistration
mm/hmm: Poison hmm_range during unregister
mm/hmm: Do not use list*_rcu() for hmm->ranges
mm/hmm: Remove confusing comment and logic from hmm_release
mm/hmm: Fix error flows in hmm_invalidate_range_start
drivers/gpu/drm/nouveau/nouveau_svm.c | 2 +-
include/linux/hmm.h | 52 +----
kernel/fork.c | 1 -
mm/hmm.c | 286 ++++++++++++--------------
4 files changed, 140 insertions(+), 201 deletions(-)
--
2.21.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Ira Weiny @ 2019-06-14 0:00 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Matthew Wilcox, Dave Chinner, Jan Kara, Dan Williams,
Theodore Ts'o, Jeff Layton, linux-xfs, Andrew Morton,
John Hubbard, Jérôme Glisse, linux-fsdevel,
linux-kernel, linux-nvdimm, linux-ext4, linux-mm, linux-rdma
In-Reply-To: <20190613234530.GK22901@ziepe.ca>
On Thu, Jun 13, 2019 at 08:45:30PM -0300, Jason Gunthorpe wrote:
> On Thu, Jun 13, 2019 at 02:13:21PM -0700, Ira Weiny wrote:
> > On Thu, Jun 13, 2019 at 08:27:55AM -0700, Matthew Wilcox wrote:
> > > On Thu, Jun 13, 2019 at 10:25:55AM +1000, Dave Chinner wrote:
> > > > e.g. Process A has an exclusive layout lease on file F. It does an
> > > > IO to file F. The filesystem IO path checks that Process A owns the
> > > > lease on the file and so skips straight through layout breaking
> > > > because it owns the lease and is allowed to modify the layout. It
> > > > then takes the inode metadata locks to allocate new space and write
> > > > new data.
> > > >
> > > > Process B now tries to write to file F. The FS checks whether
> > > > Process B owns a layout lease on file F. It doesn't, so then it
> > > > tries to break the layout lease so the IO can proceed. The layout
> > > > breaking code sees that process A has an exclusive layout lease
> > > > granted, and so returns -ETXTBSY to process B - it is not allowed to
> > > > break the lease and so the IO fails with -ETXTBSY.
> > >
> > > This description doesn't match the behaviour that RDMA wants either.
> > > Even if Process A has a lease on the file, an IO from Process A which
> > > results in blocks being freed from the file is going to result in the
> > > RDMA device being able to write to blocks which are now freed (and
> > > potentially reallocated to another file).
> >
> > I don't understand why this would not work for RDMA? As long as the layout
> > does not change the page pins can remain in place.
>
> Because process A had a layout lease (and presumably a MR) and the
> layout was still modified in way that invalidates the RDMA MR.
Oh sorry I miss read the above... (got Process A and B mixed up...)
Right, but Process A still can't free those blocks because the gup pin exists
on them... So yea it can't _just_ be a layout lease which controls this on the
"file fd".
Ira
^ permalink raw reply
* Re: [PATCH v2] RDMA/cma: Make CM response timeout and # CM retries configurable
From: Jason Gunthorpe @ 2019-06-13 23:46 UTC (permalink / raw)
To: Håkon Bugge
Cc: Doug Ledford, Leon Romanovsky, Parav Pandit, Steve Wise,
OFED mailing list, linux-kernel
In-Reply-To: <1D8E6B14-3336-42B3-B572-596DD2183D89@oracle.com>
On Thu, Jun 13, 2019 at 07:39:24PM +0200, Håkon Bugge wrote:
>
>
> > On 13 Jun 2019, at 19:23, Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Thu, Jun 13, 2019 at 06:58:30PM +0200, Håkon Bugge wrote:
> >
> >> If you refer to the backlog parameter in rdma_listen(), I cannot see
> >> it being used at all for IB.
> >>
> >> For CX-3, which is paravirtualized wrt. MAD packets, it is the proxy
> >> UD receive queue length for the PF driver that can be construed as a
> >> backlog.
> >
> > No, in IB you can drop UD packets if your RQ is full - so the proxy RQ
> > is really part of the overall RQ on QP1.
> >
> > The backlog starts once packets are taken off the RQ and begin the
> > connection accept processing.
>
> Do think we say the same thing. If, incoming REQ processing is
> severly delayed, the backlog is #entries in the QP1 receive queue in
> the PF. I can call rdma_listen() with a backlog of a zillion, but it
> will not help.
backlog and queue depth are different things, we shouldn't confuse
them together..
Jason
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Jason Gunthorpe @ 2019-06-13 23:45 UTC (permalink / raw)
To: Ira Weiny
Cc: Matthew Wilcox, Dave Chinner, Jan Kara, Dan Williams,
Theodore Ts'o, Jeff Layton, linux-xfs, Andrew Morton,
John Hubbard, Jérôme Glisse, linux-fsdevel,
linux-kernel, linux-nvdimm, linux-ext4, linux-mm, linux-rdma
In-Reply-To: <20190613211321.GC32404@iweiny-DESK2.sc.intel.com>
On Thu, Jun 13, 2019 at 02:13:21PM -0700, Ira Weiny wrote:
> On Thu, Jun 13, 2019 at 08:27:55AM -0700, Matthew Wilcox wrote:
> > On Thu, Jun 13, 2019 at 10:25:55AM +1000, Dave Chinner wrote:
> > > e.g. Process A has an exclusive layout lease on file F. It does an
> > > IO to file F. The filesystem IO path checks that Process A owns the
> > > lease on the file and so skips straight through layout breaking
> > > because it owns the lease and is allowed to modify the layout. It
> > > then takes the inode metadata locks to allocate new space and write
> > > new data.
> > >
> > > Process B now tries to write to file F. The FS checks whether
> > > Process B owns a layout lease on file F. It doesn't, so then it
> > > tries to break the layout lease so the IO can proceed. The layout
> > > breaking code sees that process A has an exclusive layout lease
> > > granted, and so returns -ETXTBSY to process B - it is not allowed to
> > > break the lease and so the IO fails with -ETXTBSY.
> >
> > This description doesn't match the behaviour that RDMA wants either.
> > Even if Process A has a lease on the file, an IO from Process A which
> > results in blocks being freed from the file is going to result in the
> > RDMA device being able to write to blocks which are now freed (and
> > potentially reallocated to another file).
>
> I don't understand why this would not work for RDMA? As long as the layout
> does not change the page pins can remain in place.
Because process A had a layout lease (and presumably a MR) and the
layout was still modified in way that invalidates the RDMA MR.
Jason
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Ira Weiny @ 2019-06-13 21:13 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Dave Chinner, Jan Kara, Dan Williams, Theodore Ts'o,
Jeff Layton, linux-xfs, Andrew Morton, John Hubbard,
Jérôme Glisse, linux-fsdevel, linux-kernel,
linux-nvdimm, linux-ext4, linux-mm, Jason Gunthorpe, linux-rdma
In-Reply-To: <20190613152755.GI32656@bombadil.infradead.org>
On Thu, Jun 13, 2019 at 08:27:55AM -0700, Matthew Wilcox wrote:
> On Thu, Jun 13, 2019 at 10:25:55AM +1000, Dave Chinner wrote:
> > e.g. Process A has an exclusive layout lease on file F. It does an
> > IO to file F. The filesystem IO path checks that Process A owns the
> > lease on the file and so skips straight through layout breaking
> > because it owns the lease and is allowed to modify the layout. It
> > then takes the inode metadata locks to allocate new space and write
> > new data.
> >
> > Process B now tries to write to file F. The FS checks whether
> > Process B owns a layout lease on file F. It doesn't, so then it
> > tries to break the layout lease so the IO can proceed. The layout
> > breaking code sees that process A has an exclusive layout lease
> > granted, and so returns -ETXTBSY to process B - it is not allowed to
> > break the lease and so the IO fails with -ETXTBSY.
>
> This description doesn't match the behaviour that RDMA wants either.
> Even if Process A has a lease on the file, an IO from Process A which
> results in blocks being freed from the file is going to result in the
> RDMA device being able to write to blocks which are now freed (and
> potentially reallocated to another file).
I don't understand why this would not work for RDMA? As long as the layout
does not change the page pins can remain in place.
Ira
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Ira Weiny @ 2019-06-13 20:34 UTC (permalink / raw)
To: Dave Chinner
Cc: Jason Gunthorpe, Theodore Ts'o,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard, Jeff Layton,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Jérôme Glisse, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
Jan Kara, linux-ext4-u79uwXL29TY76Z2rM5mHXA, Andrew Morton
In-Reply-To: <20190613005552.GI14363-pA1nmv6sEBkOM8BvhN4Z8vybgvtCy99p@public.gmane.org>
On Thu, Jun 13, 2019 at 10:55:52AM +1000, Dave Chinner wrote:
> On Wed, Jun 12, 2019 at 04:30:24PM -0700, Ira Weiny wrote:
> > On Wed, Jun 12, 2019 at 05:37:53AM -0700, Matthew Wilcox wrote:
> > > On Sat, Jun 08, 2019 at 10:10:36AM +1000, Dave Chinner wrote:
> > > > On Fri, Jun 07, 2019 at 11:25:35AM -0700, Ira Weiny wrote:
> > > > > Are you suggesting that we have something like this from user space?
> > > > >
> > > > > fcntl(fd, F_SETLEASE, F_LAYOUT | F_UNBREAKABLE);
> > > >
> > > > Rather than "unbreakable", perhaps a clearer description of the
> > > > policy it entails is "exclusive"?
> > > >
> > > > i.e. what we are talking about here is an exclusive lease that
> > > > prevents other processes from changing the layout. i.e. the
> > > > mechanism used to guarantee a lease is exclusive is that the layout
> > > > becomes "unbreakable" at the filesystem level, but the policy we are
> > > > actually presenting to uses is "exclusive access"...
> > >
> > > That's rather different from the normal meaning of 'exclusive' in the
> > > context of locks, which is "only one user can have access to this at
> > > a time". As I understand it, this is rather more like a 'shared' or
> > > 'read' lock. The filesystem would be the one which wants an exclusive
> > > lock, so it can modify the mapping of logical to physical blocks.
> > >
> > > The complication being that by default the filesystem has an exclusive
> > > lock on the mapping, and what we're trying to add is the ability for
> > > readers to ask the filesystem to give up its exclusive lock.
> >
> > This is an interesting view...
> >
> > And after some more thought, exclusive does not seem like a good name for this
> > because technically F_WRLCK _is_ an exclusive lease...
> >
> > In addition, the user does not need to take the "exclusive" write lease to be
> > notified of (broken by) an unexpected truncate. A "read" lease is broken by
> > truncate. (And "write" leases really don't do anything different WRT the
> > interaction of the FS and the user app. Write leases control "exclusive"
> > access between other file descriptors.)
>
> I've been assuming that there is only one type of layout lease -
> there is no use case I've heard of for read/write layout leases, and
> like you say there is zero difference in behaviour at the filesystem
> level - they all have to be broken to allow a non-lease truncate to
> proceed.
>
> IMO, taking a "read lease" to be able to modify and write to the
> underlying mapping of a file makes absolutely no sense at all.
> IOWs, we're talking exaclty about a revokable layout lease vs an
> exclusive layout lease here, and so read/write really doesn't match
> the policy or semantics we are trying to provide.
I humbly disagree, at least depending on how you look at it... :-D
The patches as they stand expect the user to take a "read" layout lease which
indicates they are currently using "reading" the layout as is. They are not
changing ("writing" to) the layout. They then pin pages which locks parts of
the layout and therefore they expect no "writers" to change the layout.
The "write" layout lease breaks the "read" layout lease indicating that the
layout is being written to. Should the layout be pinned in such a way that the
layout can't be changed the "layout writer" (truncate) fails.
In fact, this is what NFS does right now. The lease it puts on the file is of
"read" type.
nfs4layouts.c:
static int
nfsd4_layout_setlease(struct nfs4_layout_stateid *ls)
{
...
fl->fl_flags = FL_LAYOUT;
fl->fl_type = F_RDLCK;
...
}
I was not changing that much from the NFS patter which meant the break lease
code worked.
Jans proposal is solid but it means that there is no breaking of the lease. I
tried to add an "exclusive" flag to the "write" lease but the __break_lease()
code gets weird. I'm not saying it is not possible. Just that I have not
seen a good way to do it.
>
> > Another thing to consider is that this patch set _allows_ a truncate/hole punch
> > to proceed _if_ the pages being affected are not actually pinned. So the
> > unbreakable/exclusive nature of the lease is not absolute.
>
> If you're talking about the process that owns the layout lease
> running the truncate, then that is fine.
>
> However, if you are talking about a process that does not own the
> layout lease being allowed to truncate a file without first breaking
> the layout lease, then that is fundamentally broken.
In both cases (local or remote process) the lease is broken prior to the
attempt to truncate.
>
> i.e. If you don't own a layout lease, the layout leases must be
> broken before the truncate can proceed.
Agreed.
>
> If it's an exclusive lease,
> then you cannot break the lease and the truncate *must fail before
> it is started*. i.e. the layout lease state must be correctly
> resolved before we start an operation that may modify a file layout.
>
> Determining if we can actually do the truncate based on page state
> occurs /after/ the lease says the truncate can proceed....
That makes a lot of sense and that is the way the patch currently works.
I need to think on this some more. Keeping the lease may not be critical. As
discussed with Jan; dealing with close() is best dealt with by tracking the
actual pins on the file. If that works then we could potentially keep the
lease semantics closer to what you and I are talking about here.
Ira
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
>
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Ira Weiny @ 2019-06-13 20:34 UTC (permalink / raw)
To: Dave Chinner
Cc: Jason Gunthorpe, Theodore Ts'o,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard, Jeff Layton,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Matthew Wilcox,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Jérôme Glisse, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
Jan Kara, linux-ext4-u79uwXL29TY76Z2rM5mHXA, Andrew Morton
In-Reply-To: <20190613002555.GH14363-pA1nmv6sEBkOM8BvhN4Z8vybgvtCy99p@public.gmane.org>
On Thu, Jun 13, 2019 at 10:25:55AM +1000, Dave Chinner wrote:
> On Wed, Jun 12, 2019 at 05:37:53AM -0700, Matthew Wilcox wrote:
> > On Sat, Jun 08, 2019 at 10:10:36AM +1000, Dave Chinner wrote:
> > > On Fri, Jun 07, 2019 at 11:25:35AM -0700, Ira Weiny wrote:
> > > > Are you suggesting that we have something like this from user space?
> > > >
> > > > fcntl(fd, F_SETLEASE, F_LAYOUT | F_UNBREAKABLE);
> > >
> > > Rather than "unbreakable", perhaps a clearer description of the
> > > policy it entails is "exclusive"?
> > >
> > > i.e. what we are talking about here is an exclusive lease that
> > > prevents other processes from changing the layout. i.e. the
> > > mechanism used to guarantee a lease is exclusive is that the layout
> > > becomes "unbreakable" at the filesystem level, but the policy we are
> > > actually presenting to uses is "exclusive access"...
> >
> > That's rather different from the normal meaning of 'exclusive' in the
> > context of locks, which is "only one user can have access to this at
> > a time".
>
>
> Layout leases are not locks, they are a user access policy object.
> It is the process/fd which holds the lease and it's the process/fd
> that is granted exclusive access. This is exactly the same semantic
> as O_EXCL provides for granting exclusive access to a block device
> via open(), yes?
>
> > As I understand it, this is rather more like a 'shared' or
> > 'read' lock. The filesystem would be the one which wants an exclusive
> > lock, so it can modify the mapping of logical to physical blocks.
>
> ISTM that you're conflating internal filesystem implementation with
> application visible semantics. Yes, the filesystem uses internal
> locks to serialise the modification of the things the lease manages
> access too, but that has nothing to do with the access policy the
> lease provides to users.
>
> e.g. Process A has an exclusive layout lease on file F. It does an
> IO to file F. The filesystem IO path checks that Process A owns the
> lease on the file and so skips straight through layout breaking
> because it owns the lease and is allowed to modify the layout. It
> then takes the inode metadata locks to allocate new space and write
> new data.
>
> Process B now tries to write to file F. The FS checks whether
> Process B owns a layout lease on file F. It doesn't, so then it
> tries to break the layout lease so the IO can proceed. The layout
> breaking code sees that process A has an exclusive layout lease
> granted, and so returns -ETXTBSY to process B - it is not allowed to
> break the lease and so the IO fails with -ETXTBSY.
>
> i.e. the exclusive layout lease prevents other processes from
> performing operations that may need to modify the layout from
> performing those operations. It does not "lock" the file/inode in
> any way, it just changes how the layout lease breaking behaves.
Question: Do we expect Process A to get notified that Process B was attempting
to change the layout?
This changes the exclusivity semantics. While Process A has an exclusive lease
it could release it if notified to allow process B temporary exclusivity.
Question 2: Do we expect other process' (say Process C) to also be able to map
and pin the file? I believe users will need this and for layout purposes it is
ok to do so. But this means that Process A does not have "exclusive" access to
the lease.
So given Process C has also placed a layout lease on the file. Indicating
that it does not want the layout to change. Both A and C need to be "broken"
by Process B to change the layout. If there is no Process B; A and C can run
just fine with a "locked" layout.
Ira
>
> Further, the "exclusiveness" of a layout lease is completely
> irrelevant to the filesystem that is indicating that an operation
> that may need to modify the layout is about to be performed. All the
> filesystem has to do is handle failures to break the lease
> appropriately. Yes, XFS serialises the layout lease validation
> against other IO to the same file via it's IO locks, but that's an
> internal data IO coherency requirement, not anything to do with
> layout lease management.
>
> Note that I talk about /writes/ here. This is interchangable with
> any other operation that may need to modify the extent layout of the
> file, be it truncate, fallocate, etc: the attempt to break the
> layout lease by a non-owner should fail if the lease is "exclusive"
> to the owner.
>
> > The complication being that by default the filesystem has an exclusive
> > lock on the mapping, and what we're trying to add is the ability for
> > readers to ask the filesystem to give up its exclusive lock.
>
> The filesystem doesn't even lock the "mapping" until after the
> layout lease has been validated or broken.
>
> Cheers,
>
> Dave.
> --
> Dave Chinner
> david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org
>
^ permalink raw reply
* Re: [PATCH v2] RDMA/cma: Make CM response timeout and # CM retries configurable
From: Doug Ledford @ 2019-06-13 20:25 UTC (permalink / raw)
To: Håkon Bugge
Cc: Jason Gunthorpe, Leon Romanovsky, Parav Pandit, Steve Wise,
OFED mailing list, linux-kernel
In-Reply-To: <67B4F337-4C3A-4193-B1EF-42FD4765CBB7@oracle.com>
[-- Attachment #1: Type: text/plain, Size: 4820 bytes --]
On Thu, 2019-06-13 at 18:58 +0200, Håkon Bugge wrote:
> > On 13 Jun 2019, at 16:25, Doug Ledford <dledford@redhat.com> wrote:
> >
> > On Tue, 2019-02-26 at 08:57 +0100, Håkon Bugge wrote:
> > > During certain workloads, the default CM response timeout is too
> > > short, leading to excessive retries. Hence, make it configurable
> > > through sysctl. While at it, also make number of CM retries
> > > configurable.
> > >
> > > The defaults are not changed.
> > >
> > > Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
> > > ---
> > > v1 -> v2:
> > > * Added unregister_net_sysctl_table() in cma_cleanup()
> > > ---
> > > drivers/infiniband/core/cma.c | 52
> > > ++++++++++++++++++++++++++++++---
> > > --
> > > 1 file changed, 45 insertions(+), 7 deletions(-)
> >
> > This has been sitting on patchworks since forever. Presumably
> > because
> > Jason and I neither one felt like we really wanted it, but also
> > couldn't justify flat refusing it.
>
> I thought the agreement was to use NL and iproute2. But I haven't had
> the capacity.
To be fair, the email thread was gone from my linux-rdma folder. So, I
just had to review the entry in patchworks, and there was no captured
discussion there. So, if the agreement was made, it must have been
face to face some time and if I was involed, I had certainly forgotten
by now. But I still needed to clean up patchworks, hence my email ;-).
> > Well, I've made up my mind, so
> > unless Jason wants to argue the other side, I'm rejecting this
> > patch.
> > Here's why. The whole concept of a timeout is to help recovery in
> > a
> > situation that overloads one end of the connection. There is a
> > relationship between the max queue backlog on the one host and the
> > timeout on the other host.
>
> If you refer to the backlog parameter in rdma_listen(), I cannot see
> it being used at all for IB.
No, not exactly. I was more referring to heavy load causing an
overflow in the mad packet receive processing. We have
IB_MAD_QP_RECV_SIZE set to 512 by default, but it can be changed at
module load time of the ib_core module and that represents the maximum
number of backlogged mad packets we can have waiting to be processed
before we just drop them on the floor. There can be other places to
drop them too, but this is the one I was referring to.
> For CX-3, which is paravirtualized wrt. MAD packets, it is the proxy
> UD receive queue length for the PF driver that can be construed as a
> backlog. Remember that any MAD packet being sent from a VF or the PF
> itself, is sent to a proxy UD QP in the PF. Those packets are then
> multiplexed out on the real QP0/1. Incoming MAD packets are
> demultiplexed and sent once more to the proxy QP in the VF.
>
> > Generally, in order for a request to get
> > dropped and us to need to retransmit, the queue must already have a
> > full backlog. So, how long does it take a heavily loaded system to
> > process a full backlog? That, plus a fuzz for a margin of error,
> > should be our timeout. We shouldn't be asking users to configure
> > it.
>
> Customer configures #VMs and different workload may lead to way
> different number of CM connections. The proxying of MAD packet
> through the PF driver has a finite packet rate. With 64 VMs, 10.000
> QPs on each, all going down due to a switch failing or similar, you
> have 640.000 DREQs to be sent, and with the finite packet rate of MA
> packets through the PF, this takes more than the current CM timeout.
> And then you re-transmit and increase the burden of the PF proxying.
>
> So, we can change the default to cope with this. But, a MAD packet is
> unreliable, we may have transient loss. In this case, we want a short
> timeout.
>
> > However, if users change the default backlog queue on their
> > systems,
> > *then* it would make sense to have the users also change the
> > timeout
> > here, but I think guidance would be helpful.
> >
> > So, to revive this patch, what I'd like to see is some attempt to
> > actually quantify a reasonable timeout for the default backlog
> > depth,
> > then the patch should actually change the default to that
> > reasonable
> > timeout, and then put in the ability to adjust the timeout with
> > some
> > sort of doc guidance on how to calculate a reasonable timeout based
> > on
> > configured backlog depth.
>
> I can agree to this :-)
>
>
> Thxs, Håkon
>
> > --
> > Doug Ledford <dledford@redhat.com>
> > GPG KeyID: B826A3330E572FDD
> > Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
> > 2FDD
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* RE: [PATCH 10/13] megaraid_sas: set virt_boundary_mask in the scsi host
From: Kashyap Desai @ 2019-06-13 20:04 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Sebastian Ott, Sagi Grimberg, Max Gurtovoy,
Bart Van Assche, Ulf Hansson, Alan Stern, Oliver Neukum,
linux-block, linux-rdma, linux-mmc, linux-nvme, linux-scsi,
PDL,MEGARAIDLINUX, PDL-MPT-FUSIONLINUX, linux-hyperv, linux-usb,
usb-storage, linux-kernel
In-Reply-To: <20190613084458.GB13221@lst.de>
>
> So before I respin this series, can you help with a way to figure out
for
> mpt3sas and megaraid if a given controller supports NVMe devices at all,
so
> that we don't have to set the virt boundary if not?
In MegaRaid we have below enum - VENTURA_SERIES and AERO_SERIES
supports NVME
enum MR_ADAPTER_TYPE {
MFI_SERIES = 1,
THUNDERBOLT_SERIES = 2,
INVADER_SERIES = 3,
VENTURA_SERIES = 4,
AERO_SERIES = 5,
};
In mpt3sas driver we have below method - If IOC FACT reports NVME Device
support in Protocol Flags, we can consider it as HBA with NVME drive
support.
ioc->facts.ProtocolFlags & MPI2_IOCFACTS_PROTOCOL_NVME_DEVICES
Kashyap
^ permalink raw reply
* RE: [PATCH 10/13] megaraid_sas: set virt_boundary_mask in the scsi host
From: Kashyap Desai @ 2019-06-13 19:58 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Sebastian Ott, Sagi Grimberg, Max Gurtovoy,
Bart Van Assche, Ulf Hansson, Alan Stern, Oliver Neukum,
linux-block, linux-rdma, linux-mmc, linux-nvme, linux-scsi,
PDL,MEGARAIDLINUX, PDL-MPT-FUSIONLINUX, linux-hyperv, linux-usb,
usb-storage, linux-kernel
In-Reply-To: <20190608081400.GA19573@lst.de>
>
> On Thu, Jun 06, 2019 at 09:07:27PM +0530, Kashyap Desai wrote:
> > Hi Christoph, Changes for <megaraid_sas> and <mpt3sas> looks good. We
> > want to confirm few sanity before ACK. BTW, what benefit we will see
> > moving virt_boundry setting to SCSI mid layer ? Is it just modular
> > approach OR any functional fix ?
>
> The big difference is that virt_boundary now also changes the
> max_segment_size, and this ensures that this limit is also communicated
to
> the DMA mapping layer.
Is there any changes in API blk_queue_virt_boundary? I could not find
relevant code which account for this. Can you help ?
Which git repo shall I use for testing ? That way I can confirm, I didn't
miss relevant changes.
>From your above explanation, it means (after this patch) max segment size
of the MR controller will be set to 4K.
Earlier it is possible to receive single SGE of 64K datalength (Since max
seg size was 64K), but now the same buffer will reach the driver having 16
SGEs (Each SGE will contain 4K length).
Right ?
Kashyap
^ permalink raw reply
* Re: [PATCH mlx5-next 00/16] Mellanox, mlx5 next updates 10-06-2019
From: Saeed Mahameed @ 2019-06-13 18:02 UTC (permalink / raw)
To: Leon Romanovsky; +Cc: netdev@vger.kernel.org, linux-rdma@vger.kernel.org
In-Reply-To: <20190610233733.12155-1-saeedm@mellanox.com>
On Mon, 2019-06-10 at 23:38 +0000, Saeed Mahameed wrote:
> Hi All,
>
> This series is aimed mlx5-next branch, it includes a couple of low
> level
> updates for mlx5_core driver, needed for both rdma and net-next
> trees.
>
> 1) Bodong refactors query esw functions so he could use it to support
> querying max VFs from device.
>
> 2) Vu, handles VF representors creation from VF creation handler
> context.
>
> 3) Daniel, increased the fw initialization wait timeout for large
> sriov
> configuration.
>
> 4) Yuval, refactors IRQ vectors management and separates them from
> EQs,
> so IRQs can be shared between different EQs to save system resources,
> especially on VMs and VF functions.
>
> 5) Ariel, exploits Yuval's work and uses only one IRQ for the 4 async
> EQs we have per function (So we can save 3 IRQ vectors per function).
>
> Thanks,
> Saeed.
>
Applied to mlx5-next,
Thanks!
^ permalink raw reply
* Re: [PATCH v2 hmm 00/11] Various revisions from a locking/code review
From: Jason Gunthorpe @ 2019-06-13 17:50 UTC (permalink / raw)
To: Yang, Philip
Cc: linux-rdma@vger.kernel.org, Kuehling, Felix,
amd-gfx@lists.freedesktop.org, linux-mm@kvack.org,
dri-devel@lists.freedesktop.org, Deucher, Alexander
In-Reply-To: <69bb7fe9-98e7-8a49-3e0b-f639010b8991@amd.com>
On Wed, Jun 12, 2019 at 09:49:12PM +0000, Yang, Philip wrote:
> Rebase to https://github.com/jgunthorpe/linux.git hmm branch, need some
> changes because of interface hmm_range_register change. Then run a quick
> amdgpu_test. Test is finished, result is ok.
Great! Thanks
I'll add your Tested-by to the series
> But there is below kernel BUG message, seems hmm_free_rcu calls
> down_write.....
>
> [ 1171.919921] BUG: sleeping function called from invalid context at
> /home/yangp/git/compute_staging/kernel/kernel/locking/rwsem.c:65
> [ 1171.919933] in_atomic(): 1, irqs_disabled(): 0, pid: 53, name:
> kworker/1:1
> [ 1171.919938] 2 locks held by kworker/1:1/53:
> [ 1171.919940] #0: 000000001c7c19d4 ((wq_completion)rcu_gp){+.+.}, at:
> process_one_work+0x20e/0x630
> [ 1171.919951] #1: 00000000923f2cfa
> ((work_completion)(&sdp->work)){+.+.}, at: process_one_work+0x20e/0x630
> [ 1171.919959] CPU: 1 PID: 53 Comm: kworker/1:1 Tainted: G W
> 5.2.0-rc1-kfd-yangp #196
> [ 1171.919961] Hardware name: ASUS All Series/Z97-PRO(Wi-Fi ac)/USB 3.1,
> BIOS 9001 03/07/2016
> [ 1171.919965] Workqueue: rcu_gp srcu_invoke_callbacks
> [ 1171.919968] Call Trace:
> [ 1171.919974] dump_stack+0x67/0x9b
> [ 1171.919980] ___might_sleep+0x149/0x230
> [ 1171.919985] down_write+0x1c/0x70
> [ 1171.919989] hmm_free_rcu+0x24/0x80
> [ 1171.919993] srcu_invoke_callbacks+0xc9/0x150
> [ 1171.920000] process_one_work+0x28e/0x630
> [ 1171.920008] worker_thread+0x39/0x3f0
> [ 1171.920014] ? process_one_work+0x630/0x630
> [ 1171.920017] kthread+0x11c/0x140
> [ 1171.920021] ? kthread_park+0x90/0x90
> [ 1171.920026] ret_from_fork+0x24/0x30
Thank you Phillip, it seems the prior tests were not done with
lockdep..
Sigh, I will keep this with the gross pagetable_lock then. I updated
the patches on the git with this modification. I think we have covered
all the bases so I will send another V of the series to the list and
if no more comments then it will move ahead to hmm.git. Thanks to all.
diff --git a/mm/hmm.c b/mm/hmm.c
index 136c812faa2790..4c64d4c32f4825 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -49,16 +49,15 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
lockdep_assert_held_exclusive(&mm->mmap_sem);
+ /* Abuse the page_table_lock to also protect mm->hmm. */
+ spin_lock(&mm->page_table_lock);
if (mm->hmm) {
- if (kref_get_unless_zero(&mm->hmm->kref))
+ if (kref_get_unless_zero(&mm->hmm->kref)) {
+ spin_unlock(&mm->page_table_lock);
return mm->hmm;
- /*
- * The hmm is being freed by some other CPU and is pending a
- * RCU grace period, but this CPU can NULL now it since we
- * have the mmap_sem.
- */
- mm->hmm = NULL;
+ }
}
+ spin_unlock(&mm->page_table_lock);
hmm = kmalloc(sizeof(*hmm), GFP_KERNEL);
if (!hmm)
@@ -81,7 +80,14 @@ static struct hmm *hmm_get_or_create(struct mm_struct *mm)
}
mmgrab(hmm->mm);
+
+ /*
+ * We hold the exclusive mmap_sem here so we know that mm->hmm is
+ * still NULL or 0 kref, and is safe to update.
+ */
+ spin_lock(&mm->page_table_lock);
mm->hmm = hmm;
+ spin_unlock(&mm->page_table_lock);
return hmm;
}
@@ -89,10 +95,14 @@ static void hmm_free_rcu(struct rcu_head *rcu)
{
struct hmm *hmm = container_of(rcu, struct hmm, rcu);
- down_write(&hmm->mm->mmap_sem);
+ /*
+ * The mm->hmm pointer is kept valid while notifier ops can be running
+ * so they don't have to deal with a NULL mm->hmm value
+ */
+ spin_lock(&hmm->mm->page_table_lock);
if (hmm->mm->hmm == hmm)
hmm->mm->hmm = NULL;
- up_write(&hmm->mm->mmap_sem);
+ spin_unlock(&hmm->mm->page_table_lock);
mmdrop(hmm->mm);
kfree(hmm);
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related
* Re: [PATCH v2] RDMA/cma: Make CM response timeout and # CM retries configurable
From: Håkon Bugge @ 2019-06-13 17:39 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Doug Ledford, Leon Romanovsky, Parav Pandit, Steve Wise,
OFED mailing list, linux-kernel
In-Reply-To: <20190613172355.GF22901@ziepe.ca>
> On 13 Jun 2019, at 19:23, Jason Gunthorpe <jgg@ziepe.ca> wrote:
>
> On Thu, Jun 13, 2019 at 06:58:30PM +0200, Håkon Bugge wrote:
>
>> If you refer to the backlog parameter in rdma_listen(), I cannot see
>> it being used at all for IB.
>>
>> For CX-3, which is paravirtualized wrt. MAD packets, it is the proxy
>> UD receive queue length for the PF driver that can be construed as a
>> backlog.
>
> No, in IB you can drop UD packets if your RQ is full - so the proxy RQ
> is really part of the overall RQ on QP1.
>
> The backlog starts once packets are taken off the RQ and begin the
> connection accept processing.
Do think we say the same thing. If, incoming REQ processing is severly delayed, the backlog is #entries in the QP1 receive queue in the PF. I can call rdma_listen() with a backlog of a zillion, but it will not help.
>> Customer configures #VMs and different workload may lead to way
>> different number of CM connections. The proxying of MAD packet
>> through the PF driver has a finite packet rate. With 64 VMs, 10.000
>> QPs on each, all going down due to a switch failing or similar, you
>> have 640.000 DREQs to be sent, and with the finite packet rate of MA
>> packets through the PF, this takes more than the current CM
>> timeout. And then you re-transmit and increase the burden of the PF
>> proxying.
>
> I feel like the performance of all this proxying is too low to support
> such a large work load :(
That is what I am aiming at, for example to spread the completion_vector(s) for said QPs ;-)
-h
>
> Can it be improved?
>
> Jason
^ permalink raw reply
* Re: [PATCH v2] RDMA/cma: Make CM response timeout and # CM retries configurable
From: Jason Gunthorpe @ 2019-06-13 17:23 UTC (permalink / raw)
To: Håkon Bugge
Cc: Doug Ledford, Leon Romanovsky, Parav Pandit, Steve Wise,
OFED mailing list, linux-kernel
In-Reply-To: <67B4F337-4C3A-4193-B1EF-42FD4765CBB7@oracle.com>
On Thu, Jun 13, 2019 at 06:58:30PM +0200, Håkon Bugge wrote:
> If you refer to the backlog parameter in rdma_listen(), I cannot see
> it being used at all for IB.
>
> For CX-3, which is paravirtualized wrt. MAD packets, it is the proxy
> UD receive queue length for the PF driver that can be construed as a
> backlog.
No, in IB you can drop UD packets if your RQ is full - so the proxy RQ
is really part of the overall RQ on QP1.
The backlog starts once packets are taken off the RQ and begin the
connection accept processing.
> Customer configures #VMs and different workload may lead to way
> different number of CM connections. The proxying of MAD packet
> through the PF driver has a finite packet rate. With 64 VMs, 10.000
> QPs on each, all going down due to a switch failing or similar, you
> have 640.000 DREQs to be sent, and with the finite packet rate of MA
> packets through the PF, this takes more than the current CM
> timeout. And then you re-transmit and increase the burden of the PF
> proxying.
I feel like the performance of all this proxying is too low to support
such a large work load :(
Can it be improved?
Jason
^ permalink raw reply
* Re: [PATCH v2] RDMA/cma: Make CM response timeout and # CM retries configurable
From: Håkon Bugge @ 2019-06-13 16:58 UTC (permalink / raw)
To: Doug Ledford
Cc: Jason Gunthorpe, Leon Romanovsky, Parav Pandit, Steve Wise,
OFED mailing list, linux-kernel
In-Reply-To: <174ccd37a9ffa05d0c7c03fe80ff7170a9270824.camel@redhat.com>
> On 13 Jun 2019, at 16:25, Doug Ledford <dledford@redhat.com> wrote:
>
> On Tue, 2019-02-26 at 08:57 +0100, Håkon Bugge wrote:
>> During certain workloads, the default CM response timeout is too
>> short, leading to excessive retries. Hence, make it configurable
>> through sysctl. While at it, also make number of CM retries
>> configurable.
>>
>> The defaults are not changed.
>>
>> Signed-off-by: Håkon Bugge <haakon.bugge@oracle.com>
>> ---
>> v1 -> v2:
>> * Added unregister_net_sysctl_table() in cma_cleanup()
>> ---
>> drivers/infiniband/core/cma.c | 52 ++++++++++++++++++++++++++++++---
>> --
>> 1 file changed, 45 insertions(+), 7 deletions(-)
>
> This has been sitting on patchworks since forever. Presumably because
> Jason and I neither one felt like we really wanted it, but also
> couldn't justify flat refusing it.
I thought the agreement was to use NL and iproute2. But I haven't had the capacity.
> Well, I've made up my mind, so
> unless Jason wants to argue the other side, I'm rejecting this patch.
> Here's why. The whole concept of a timeout is to help recovery in a
> situation that overloads one end of the connection. There is a
> relationship between the max queue backlog on the one host and the
> timeout on the other host.
If you refer to the backlog parameter in rdma_listen(), I cannot see it being used at all for IB.
For CX-3, which is paravirtualized wrt. MAD packets, it is the proxy UD receive queue length for the PF driver that can be construed as a backlog. Remember that any MAD packet being sent from a VF or the PF itself, is sent to a proxy UD QP in the PF. Those packets are then multiplexed out on the real QP0/1. Incoming MAD packets are demultiplexed and sent once more to the proxy QP in the VF.
> Generally, in order for a request to get
> dropped and us to need to retransmit, the queue must already have a
> full backlog. So, how long does it take a heavily loaded system to
> process a full backlog? That, plus a fuzz for a margin of error,
> should be our timeout. We shouldn't be asking users to configure it.
Customer configures #VMs and different workload may lead to way different number of CM connections. The proxying of MAD packet through the PF driver has a finite packet rate. With 64 VMs, 10.000 QPs on each, all going down due to a switch failing or similar, you have 640.000 DREQs to be sent, and with the finite packet rate of MA packets through the PF, this takes more than the current CM timeout. And then you re-transmit and increase the burden of the PF proxying.
So, we can change the default to cope with this. But, a MAD packet is unreliable, we may have transient loss. In this case, we want a short timeout.
> However, if users change the default backlog queue on their systems,
> *then* it would make sense to have the users also change the timeout
> here, but I think guidance would be helpful.
>
> So, to revive this patch, what I'd like to see is some attempt to
> actually quantify a reasonable timeout for the default backlog depth,
> then the patch should actually change the default to that reasonable
> timeout, and then put in the ability to adjust the timeout with some
> sort of doc guidance on how to calculate a reasonable timeout based on
> configured backlog depth.
I can agree to this :-)
Thxs, Håkon
>
> --
> Doug Ledford <dledford@redhat.com>
> GPG KeyID: B826A3330E572FDD
> Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
> 2FDD
^ permalink raw reply
* Re: [PATCH net-next v2 2/2] ipoib: show VF broadcast address
From: Michal Kubecek @ 2019-06-13 16:39 UTC (permalink / raw)
To: Denis Kirjanov; +Cc: davem, dledford, netdev, linux-rdma
In-Reply-To: <20190613142003.129391-4-dkirjanov@suse.com>
On Thu, Jun 13, 2019 at 04:20:03PM +0200, Denis Kirjanov wrote:
> in IPoIB case we can't see a VF broadcast address for but
> can see for PF
>
> Before:
> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
> state UP mode DEFAULT group default qlen 256
> link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
> vf 0 MAC 14:80:00:00:66:fe, spoof checking off, link-state disable,
> trust off, query_rss off
> ...
>
> After:
> 11: ib1: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 2044 qdisc pfifo_fast
> state UP mode DEFAULT group default qlen 256
> link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff
> vf 0 link/infiniband
> 80:00:00:66:fe:80:00:00:00:00:00:00:24:8a:07:03:00:a4:3e:7c brd
> 00:ff:ff:ff:ff:12:40:1b:ff:ff:00:00:00:00:00:00:ff:ff:ff:ff, spoof
> checking off, link-state disable, trust off, query_rss off
>
> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
> ---
> include/uapi/linux/if_link.h | 5 +++++
> net/core/rtnetlink.c | 6 ++++++
> 2 files changed, 11 insertions(+)
>
> diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
> index 5b225ff63b48..1f36dd3a45d6 100644
> --- a/include/uapi/linux/if_link.h
> +++ b/include/uapi/linux/if_link.h
> @@ -681,6 +681,7 @@ enum {
> enum {
> IFLA_VF_UNSPEC,
> IFLA_VF_MAC, /* Hardware queue specific attributes */
> + IFLA_VF_BROADCAST,
> IFLA_VF_VLAN, /* VLAN ID and QoS */
> IFLA_VF_TX_RATE, /* Max TX Bandwidth Allocation */
> IFLA_VF_SPOOFCHK, /* Spoof Checking on/off switch */
Oops, I forgot to mention one important point when reviewing v1: the new
attribute type must be added at the end (just before __IFLA_VF_MAX) so
that you do not change value of existing IFLA_VF_* constants (this would
break compatibility).
> @@ -704,6 +705,10 @@ struct ifla_vf_mac {
> __u8 mac[32]; /* MAX_ADDR_LEN */
> };
>
> +struct ifla_vf_broadcast {
> + __u8 broadcast[32];
> +};
> +
> struct ifla_vf_vlan {
> __u32 vf;
> __u32 vlan; /* 0 - 4095, 0 disables VLAN filter */
My first idea was that to question the need of a wrapping structure as
we couldn't modify that structure in the future anyway so that there
does not seem to be any gain against simply passing the address as a
binary with attribute length equal to address length (like we do with
IFLA_ADDRESS and IFLA_BROADCAST).
But then I checked other IFLA_VF_* attributes and I'm confused. The
structure seems to be
IFLA_VF_INFO_LIST
IFLA_VF_INFO
IFLA_VF_MAC
IFLA_VF_VLAN
...
IFLA_VF_INFO
IFLA_VF_MAC
IFLA_VF_VLAN
...
...
Each IFLA_VF_INFO corresponds to one virtual function but its number is
not determined by an attribute within this nest. Instead, each of the
neste IFLA_VF_* attributes is a structure containing "__u32 vf" and it's
only matter of convention that within one IFLA_VF_INFO nest, all data
belongs to the same VF, neither do_setlink() nor do_setvfinfo() check
it.
I guess you should either follow this weird pattern or introduce proper
IFLA_VF_ID to be used for IFLA_VF_BROADCAST and all future IFLA_VF_*
attributes. However, each new attribute makes IFLA_VF_INFO bigger and
lowers the number of VFs that can be stored in an IFLA_VF_INFO_LIST nest
without exceeding the hard limit of 65535 bytes so that we cannot afford
to add too many.
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index cec60583931f..88304212f127 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
...
> @@ -1753,6 +1758,7 @@ static const struct nla_policy ifla_info_policy[IFLA_INFO_MAX+1] = {
>
> static const struct nla_policy ifla_vf_policy[IFLA_VF_MAX+1] = {
> [IFLA_VF_MAC] = { .len = sizeof(struct ifla_vf_mac) },
> + [IFLA_VF_BROADCAST] = {. len = sizeof(struct ifla_vf_broadcast) },
> [IFLA_VF_VLAN] = { .len = sizeof(struct ifla_vf_vlan) },
> [IFLA_VF_VLAN_LIST] = { .type = NLA_NESTED },
> [IFLA_VF_TX_RATE] = { .len = sizeof(struct ifla_vf_tx_rate) },
As you do not implement setting the broadcast address (is that possible
at all?), NLA_REJECT would be more appropriate so that the request isn't
silently ignored.
Michal
^ permalink raw reply
* Re: [PATCH v2] RDMA/cma: Make CM response timeout and # CM retries configurable
From: Doug Ledford @ 2019-06-13 16:32 UTC (permalink / raw)
To: Bart Van Assche, Håkon Bugge, Jason Gunthorpe,
Leon Romanovsky, Parav Pandit, Steve Wise
Cc: linux-rdma, linux-kernel
In-Reply-To: <2c114313-01fe-6d4d-5134-592d1a7b829b@acm.org>
[-- Attachment #1: Type: text/plain, Size: 1577 bytes --]
On Thu, 2019-06-13 at 08:28 -0700, Bart Van Assche wrote:
> On 6/13/19 7:25 AM, Doug Ledford wrote:
> > So, to revive this patch, what I'd like to see is some attempt to
> > actually quantify a reasonable timeout for the default backlog
> > depth,
> > then the patch should actually change the default to that
> > reasonable
> > timeout, and then put in the ability to adjust the timeout with
> > some
> > sort of doc guidance on how to calculate a reasonable timeout based
> > on
> > configured backlog depth.
>
> How about following the approach of the SRP initiator driver? It
> derives
> the CM timeout from the subnet manager timeout. The assumption
> behind
> this is that in large networks the subnet manager timeout has to be
> set
> higher than its default to make communication work. See also
> srp_get_subnet_timeout().
Theoretically, the subnet manager needs a longer timeout in a bigger
network because it's handling more data as a single point of lookup for
the entire subnet. Individual machines, on the other hand, have the
same backlog size (by default) regardless of the size of the network,
and there is no guarantee that if the admin increased the subnet
manager timeout, that they also increased the backlog queue depth size.
So, while I like things that auto-tune like you are suggesting, the
problem is that the one item does not directly correlate with the
other.
--
Doug Ledford <dledford@redhat.com>
GPG KeyID: B826A3330E572FDD
Key fingerprint = AE6B 1BDA 122B 23B4 265B 1274 B826 A333 0E57
2FDD
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
From: Vincenzo Frascino @ 2019-06-13 16:15 UTC (permalink / raw)
To: Catalin Marinas
Cc: Mark Rutland, kvm, Christian Koenig, Szabolcs Nagy, Will Deacon,
dri-devel, Kostya Serebryany, Khalid Aziz, Lee Smith,
linux-kselftest, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Christoph Hellwig, Jason Gunthorpe, Dmitry Vyukov,
Dave Martin, Evgeniy Stepanov, linux-media, Kees Cook,
Ruben Ayrapetyan, Andrey Konovalov, Kevin Brodsky,
Alex Williamson <alex.williams>
In-Reply-To: <20190613155754.GX28951@C02TF0J2HF1T.local>
On 13/06/2019 16:57, Catalin Marinas wrote:
> On Thu, Jun 13, 2019 at 04:45:54PM +0100, Vincenzo Frascino wrote:
>> On 13/06/2019 16:35, Catalin Marinas wrote:
>>> On Thu, Jun 13, 2019 at 12:16:59PM +0100, Dave P Martin wrote:
>>>> On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote:
>>>>> +
>>>>> +/*
>>>>> + * Control the relaxed ABI allowing tagged user addresses into the kernel.
>>>>> + */
>>>>> +static unsigned int tagged_addr_prctl_allowed = 1;
>>>>> +
>>>>> +long set_tagged_addr_ctrl(unsigned long arg)
>>>>> +{
>>>>> + if (!tagged_addr_prctl_allowed)
>>>>> + return -EINVAL;
>>>>
>>>> So, tagging can actually be locked on by having a process enable it and
>>>> then some possibly unrelated process clearing tagged_addr_prctl_allowed.
>>>> That feels a bit weird.
>>>
>>> The problem is that if you disable the ABI globally, lots of
>>> applications would crash. This sysctl is meant as a way to disable the
>>> opt-in to the TBI ABI. Another option would be a kernel command line
>>> option (I'm not keen on a Kconfig option).
>>
>> Why you are not keen on a Kconfig option?
>
> Because I don't want to rebuild the kernel/reboot just to be able to
> test how user space handles the ABI opt-in. I'm ok with a Kconfig option
> to disable this globally in addition to a run-time option (if actually
> needed, I'm not sure).
>
There might be scenarios (i.e. embedded) in which this is not needed, hence
having a config option (maybe Y by default) that removes from the kernel the
whole feature would be good, obviously in conjunction with the run-time option.
Based on my previous review, if we move out the code from process.c in its own
independent file when the Kconfig option is turned off we could remove the
entire object from the kernel (this would remove the sysctl and let still the
prctl return -EINVAL).
These changes though could be done successively with a separate patch set, if
the Kconfig is meant to be Y by default.
--
Regards,
Vincenzo
^ permalink raw reply
* Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
From: Catalin Marinas @ 2019-06-13 15:57 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: Dave Martin, Andrey Konovalov, linux-arm-kernel, linux-mm,
linux-kernel, amd-gfx, dri-devel, linux-rdma, linux-media, kvm,
linux-kselftest, Mark Rutland, Szabolcs Nagy, Will Deacon,
Kostya Serebryany, Khalid Aziz, Felix Kuehling, Jacob Bramley,
Leon Romanovsky, Christoph Hellwig, Jason Gunthorpe,
Evgeniy Stepanov, Ke
In-Reply-To: <99cc257d-5e99-922a-fbe7-3bbaf3621e38@arm.com>
On Thu, Jun 13, 2019 at 04:45:54PM +0100, Vincenzo Frascino wrote:
> On 13/06/2019 16:35, Catalin Marinas wrote:
> > On Thu, Jun 13, 2019 at 12:16:59PM +0100, Dave P Martin wrote:
> >> On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote:
> >>> +
> >>> +/*
> >>> + * Control the relaxed ABI allowing tagged user addresses into the kernel.
> >>> + */
> >>> +static unsigned int tagged_addr_prctl_allowed = 1;
> >>> +
> >>> +long set_tagged_addr_ctrl(unsigned long arg)
> >>> +{
> >>> + if (!tagged_addr_prctl_allowed)
> >>> + return -EINVAL;
> >>
> >> So, tagging can actually be locked on by having a process enable it and
> >> then some possibly unrelated process clearing tagged_addr_prctl_allowed.
> >> That feels a bit weird.
> >
> > The problem is that if you disable the ABI globally, lots of
> > applications would crash. This sysctl is meant as a way to disable the
> > opt-in to the TBI ABI. Another option would be a kernel command line
> > option (I'm not keen on a Kconfig option).
>
> Why you are not keen on a Kconfig option?
Because I don't want to rebuild the kernel/reboot just to be able to
test how user space handles the ABI opt-in. I'm ok with a Kconfig option
to disable this globally in addition to a run-time option (if actually
needed, I'm not sure).
--
Catalin
^ permalink raw reply
* Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
From: Vincenzo Frascino @ 2019-06-13 15:45 UTC (permalink / raw)
To: Catalin Marinas, Dave Martin
Cc: Mark Rutland, kvm, Christian Koenig, Szabolcs Nagy, Will Deacon,
dri-devel, Kostya Serebryany, Khalid Aziz, Lee Smith,
linux-kselftest, Jacob Bramley, Leon Romanovsky, linux-rdma,
amd-gfx, Christoph Hellwig, Jason Gunthorpe, Dmitry Vyukov,
Evgeniy Stepanov, linux-media, Kees Cook, Ruben Ayrapetyan,
Andrey Konovalov, Kevin Brodsky, Alex Williamson, Mauro
In-Reply-To: <20190613153505.GU28951@C02TF0J2HF1T.local>
On 13/06/2019 16:35, Catalin Marinas wrote:
> On Thu, Jun 13, 2019 at 12:16:59PM +0100, Dave P Martin wrote:
>> On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote:
>>> From: Catalin Marinas <catalin.marinas@arm.com>
>>>
>>> It is not desirable to relax the ABI to allow tagged user addresses into
>>> the kernel indiscriminately. This patch introduces a prctl() interface
>>> for enabling or disabling the tagged ABI with a global sysctl control
>>> for preventing applications from enabling the relaxed ABI (meant for
>>> testing user-space prctl() return error checking without reconfiguring
>>> the kernel). The ABI properties are inherited by threads of the same
>>> application and fork()'ed children but cleared on execve().
>>>
>>> The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle
>>> MTE-specific settings like imprecise vs precise exceptions.
>>>
>>> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
>>> ---
>>> arch/arm64/include/asm/processor.h | 6 +++
>>> arch/arm64/include/asm/thread_info.h | 1 +
>>> arch/arm64/include/asm/uaccess.h | 3 +-
>>> arch/arm64/kernel/process.c | 67 ++++++++++++++++++++++++++++
>>> include/uapi/linux/prctl.h | 5 +++
>>> kernel/sys.c | 16 +++++++
>>> 6 files changed, 97 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
>>> index fcd0e691b1ea..fee457456aa8 100644
>>> --- a/arch/arm64/include/asm/processor.h
>>> +++ b/arch/arm64/include/asm/processor.h
>>> @@ -307,6 +307,12 @@ extern void __init minsigstksz_setup(void);
>>> /* PR_PAC_RESET_KEYS prctl */
>>> #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg)
>>>
>>> +/* PR_TAGGED_ADDR prctl */
>>
>> (A couple of comments I missed in my last reply:)
>>
>> Name mismatch?
>
> Yeah, it went through several names but it seems that I didn't update
> all places.
>
>>> +long set_tagged_addr_ctrl(unsigned long arg);
>>> +long get_tagged_addr_ctrl(void);
>>> +#define SET_TAGGED_ADDR_CTRL(arg) set_tagged_addr_ctrl(arg)
>>> +#define GET_TAGGED_ADDR_CTRL() get_tagged_addr_ctrl()
>>> +
>>
>> [...]
>>
>>> diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
>>> index 3767fb21a5b8..69d0be1fc708 100644
>>> --- a/arch/arm64/kernel/process.c
>>> +++ b/arch/arm64/kernel/process.c
>>> @@ -30,6 +30,7 @@
>>> #include <linux/kernel.h>
>>> #include <linux/mm.h>
>>> #include <linux/stddef.h>
>>> +#include <linux/sysctl.h>
>>> #include <linux/unistd.h>
>>> #include <linux/user.h>
>>> #include <linux/delay.h>
>>> @@ -323,6 +324,7 @@ void flush_thread(void)
>>> fpsimd_flush_thread();
>>> tls_thread_flush();
>>> flush_ptrace_hw_breakpoint(current);
>>> + clear_thread_flag(TIF_TAGGED_ADDR);
>>> }
>>>
>>> void release_thread(struct task_struct *dead_task)
>>> @@ -552,3 +554,68 @@ void arch_setup_new_exec(void)
>>>
>>> ptrauth_thread_init_user(current);
>>> }
>>> +
>>> +/*
>>> + * Control the relaxed ABI allowing tagged user addresses into the kernel.
>>> + */
>>> +static unsigned int tagged_addr_prctl_allowed = 1;
>>> +
>>> +long set_tagged_addr_ctrl(unsigned long arg)
>>> +{
>>> + if (!tagged_addr_prctl_allowed)
>>> + return -EINVAL;
>>
>> So, tagging can actually be locked on by having a process enable it and
>> then some possibly unrelated process clearing tagged_addr_prctl_allowed.
>> That feels a bit weird.
>
> The problem is that if you disable the ABI globally, lots of
> applications would crash. This sysctl is meant as a way to disable the
> opt-in to the TBI ABI. Another option would be a kernel command line
> option (I'm not keen on a Kconfig option).
>
Why you are not keen on a Kconfig option?
>> Do we want to allow a process that has tagging on to be able to turn
>> it off at all? Possibly things like CRIU might want to do that.
>
> I left it in for symmetry but I don't expect it to be used. A potential
> use-case is doing it per subsequent threads in an application.
>
>>> + if (is_compat_task())
>>> + return -EINVAL;
>>> + if (arg & ~PR_TAGGED_ADDR_ENABLE)
>>> + return -EINVAL;
>>
>> How do we expect this argument to be extended in the future?
>
> Yes, for MTE. That's why I wouldn't allow random bits here.
>
>> I'm wondering whether this is really a bitmask or an enum, or a mixture
>> of the two. Maybe it doesn't matter.
>
> User may want to set PR_TAGGED_ADDR_ENABLE | PR_MTE_PRECISE in a single
> call.
>
>>> + if (arg & PR_TAGGED_ADDR_ENABLE)
>>> + set_thread_flag(TIF_TAGGED_ADDR);
>>> + else
>>> + clear_thread_flag(TIF_TAGGED_ADDR);
>>
>> I think update_thread_flag() could be used here.
>
> Yes. I forgot you added this.
>
--
Regards,
Vincenzo
^ permalink raw reply
* Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
From: Catalin Marinas @ 2019-06-13 15:35 UTC (permalink / raw)
To: Dave Martin
Cc: Andrey Konovalov, linux-arm-kernel, linux-mm, linux-kernel,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm, linux-kselftest,
Mark Rutland, Szabolcs Nagy, Will Deacon, Kostya Serebryany,
Khalid Aziz, Felix Kuehling, Vincenzo Frascino, Jacob Bramley,
Leon Romanovsky, Christoph Hellwig, Jason Gunthorpe,
Evgeniy Stepanov
In-Reply-To: <20190613111659.GX28398@e103592.cambridge.arm.com>
On Thu, Jun 13, 2019 at 12:16:59PM +0100, Dave P Martin wrote:
> On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote:
> > From: Catalin Marinas <catalin.marinas@arm.com>
> >
> > It is not desirable to relax the ABI to allow tagged user addresses into
> > the kernel indiscriminately. This patch introduces a prctl() interface
> > for enabling or disabling the tagged ABI with a global sysctl control
> > for preventing applications from enabling the relaxed ABI (meant for
> > testing user-space prctl() return error checking without reconfiguring
> > the kernel). The ABI properties are inherited by threads of the same
> > application and fork()'ed children but cleared on execve().
> >
> > The PR_SET_TAGGED_ADDR_CTRL will be expanded in the future to handle
> > MTE-specific settings like imprecise vs precise exceptions.
> >
> > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> > ---
> > arch/arm64/include/asm/processor.h | 6 +++
> > arch/arm64/include/asm/thread_info.h | 1 +
> > arch/arm64/include/asm/uaccess.h | 3 +-
> > arch/arm64/kernel/process.c | 67 ++++++++++++++++++++++++++++
> > include/uapi/linux/prctl.h | 5 +++
> > kernel/sys.c | 16 +++++++
> > 6 files changed, 97 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h
> > index fcd0e691b1ea..fee457456aa8 100644
> > --- a/arch/arm64/include/asm/processor.h
> > +++ b/arch/arm64/include/asm/processor.h
> > @@ -307,6 +307,12 @@ extern void __init minsigstksz_setup(void);
> > /* PR_PAC_RESET_KEYS prctl */
> > #define PAC_RESET_KEYS(tsk, arg) ptrauth_prctl_reset_keys(tsk, arg)
> >
> > +/* PR_TAGGED_ADDR prctl */
>
> (A couple of comments I missed in my last reply:)
>
> Name mismatch?
Yeah, it went through several names but it seems that I didn't update
all places.
> > +long set_tagged_addr_ctrl(unsigned long arg);
> > +long get_tagged_addr_ctrl(void);
> > +#define SET_TAGGED_ADDR_CTRL(arg) set_tagged_addr_ctrl(arg)
> > +#define GET_TAGGED_ADDR_CTRL() get_tagged_addr_ctrl()
> > +
>
> [...]
>
> > diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c
> > index 3767fb21a5b8..69d0be1fc708 100644
> > --- a/arch/arm64/kernel/process.c
> > +++ b/arch/arm64/kernel/process.c
> > @@ -30,6 +30,7 @@
> > #include <linux/kernel.h>
> > #include <linux/mm.h>
> > #include <linux/stddef.h>
> > +#include <linux/sysctl.h>
> > #include <linux/unistd.h>
> > #include <linux/user.h>
> > #include <linux/delay.h>
> > @@ -323,6 +324,7 @@ void flush_thread(void)
> > fpsimd_flush_thread();
> > tls_thread_flush();
> > flush_ptrace_hw_breakpoint(current);
> > + clear_thread_flag(TIF_TAGGED_ADDR);
> > }
> >
> > void release_thread(struct task_struct *dead_task)
> > @@ -552,3 +554,68 @@ void arch_setup_new_exec(void)
> >
> > ptrauth_thread_init_user(current);
> > }
> > +
> > +/*
> > + * Control the relaxed ABI allowing tagged user addresses into the kernel.
> > + */
> > +static unsigned int tagged_addr_prctl_allowed = 1;
> > +
> > +long set_tagged_addr_ctrl(unsigned long arg)
> > +{
> > + if (!tagged_addr_prctl_allowed)
> > + return -EINVAL;
>
> So, tagging can actually be locked on by having a process enable it and
> then some possibly unrelated process clearing tagged_addr_prctl_allowed.
> That feels a bit weird.
The problem is that if you disable the ABI globally, lots of
applications would crash. This sysctl is meant as a way to disable the
opt-in to the TBI ABI. Another option would be a kernel command line
option (I'm not keen on a Kconfig option).
> Do we want to allow a process that has tagging on to be able to turn
> it off at all? Possibly things like CRIU might want to do that.
I left it in for symmetry but I don't expect it to be used. A potential
use-case is doing it per subsequent threads in an application.
> > + if (is_compat_task())
> > + return -EINVAL;
> > + if (arg & ~PR_TAGGED_ADDR_ENABLE)
> > + return -EINVAL;
>
> How do we expect this argument to be extended in the future?
Yes, for MTE. That's why I wouldn't allow random bits here.
> I'm wondering whether this is really a bitmask or an enum, or a mixture
> of the two. Maybe it doesn't matter.
User may want to set PR_TAGGED_ADDR_ENABLE | PR_MTE_PRECISE in a single
call.
> > + if (arg & PR_TAGGED_ADDR_ENABLE)
> > + set_thread_flag(TIF_TAGGED_ADDR);
> > + else
> > + clear_thread_flag(TIF_TAGGED_ADDR);
>
> I think update_thread_flag() could be used here.
Yes. I forgot you added this.
--
Catalin
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Jason Gunthorpe @ 2019-06-13 15:29 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Dave Chinner, Ira Weiny, Jan Kara, Dan Williams,
Theodore Ts'o, Jeff Layton, linux-xfs, Andrew Morton,
John Hubbard, Jérôme Glisse, linux-fsdevel,
linux-kernel, linux-nvdimm, linux-ext4, linux-mm, linux-rdma
In-Reply-To: <20190613032320.GG32656@bombadil.infradead.org>
On Wed, Jun 12, 2019 at 08:23:20PM -0700, Matthew Wilcox wrote:
> On Thu, Jun 13, 2019 at 10:25:55AM +1000, Dave Chinner wrote:
> > On Wed, Jun 12, 2019 at 05:37:53AM -0700, Matthew Wilcox wrote:
> > > That's rather different from the normal meaning of 'exclusive' in the
> > > context of locks, which is "only one user can have access to this at
> > > a time".
> >
> > Layout leases are not locks, they are a user access policy object.
> > It is the process/fd which holds the lease and it's the process/fd
> > that is granted exclusive access. This is exactly the same semantic
> > as O_EXCL provides for granting exclusive access to a block device
> > via open(), yes?
>
> This isn't my understanding of how RDMA wants this to work, so we should
> probably clear that up before we get too far down deciding what name to
> give it.
>
> For the RDMA usage case, it is entirely possible that both process A
> and process B which don't know about each other want to perform RDMA to
> file F. So there will be two layout leases active on this file at the
> same time. It's fine for IOs to simultaneously be active to both leases.
> But if the filesystem wants to move blocks around, it has to break
> both leases.
>
> If Process C tries to do a write to file F without a lease, there's no
> problem, unless a side-effect of the write would be to change the block
> mapping, in which case either the leases must break first, or the write
> must be denied.
>
> Jason, please correct me if I've misunderstood the RDMA needs here.
Yes, I think you've captured it
Based on Dave's remarks how frequently a filesystem needs to break the
lease will determine if it is usuable to be combined with RDMA or
not...
Jason
^ permalink raw reply
* Re: [PATCH v2] RDMA/cma: Make CM response timeout and # CM retries configurable
From: Bart Van Assche @ 2019-06-13 15:28 UTC (permalink / raw)
To: Doug Ledford, Håkon Bugge, Jason Gunthorpe, Leon Romanovsky,
Parav Pandit, Steve Wise
Cc: linux-rdma, linux-kernel
In-Reply-To: <174ccd37a9ffa05d0c7c03fe80ff7170a9270824.camel@redhat.com>
On 6/13/19 7:25 AM, Doug Ledford wrote:
> So, to revive this patch, what I'd like to see is some attempt to
> actually quantify a reasonable timeout for the default backlog depth,
> then the patch should actually change the default to that reasonable
> timeout, and then put in the ability to adjust the timeout with some
> sort of doc guidance on how to calculate a reasonable timeout based on
> configured backlog depth.
How about following the approach of the SRP initiator driver? It derives
the CM timeout from the subnet manager timeout. The assumption behind
this is that in large networks the subnet manager timeout has to be set
higher than its default to make communication work. See also
srp_get_subnet_timeout().
Bart.
^ permalink raw reply
* Re: [PATCH RFC 00/10] RDMA/FS DAX truncate proposal
From: Matthew Wilcox @ 2019-06-13 15:27 UTC (permalink / raw)
To: Dave Chinner
Cc: Jason Gunthorpe, Jan Kara, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw,
linux-rdma-u79uwXL29TY76Z2rM5mHXA, John Hubbard, Jeff Layton,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-xfs-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg,
Jérôme Glisse, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA,
Theodore Ts'o, linux-ext4-u79uwXL29TY76Z2rM5mHXA,
Andrew Morton
In-Reply-To: <20190613002555.GH14363-pA1nmv6sEBkOM8BvhN4Z8vybgvtCy99p@public.gmane.org>
On Thu, Jun 13, 2019 at 10:25:55AM +1000, Dave Chinner wrote:
> e.g. Process A has an exclusive layout lease on file F. It does an
> IO to file F. The filesystem IO path checks that Process A owns the
> lease on the file and so skips straight through layout breaking
> because it owns the lease and is allowed to modify the layout. It
> then takes the inode metadata locks to allocate new space and write
> new data.
>
> Process B now tries to write to file F. The FS checks whether
> Process B owns a layout lease on file F. It doesn't, so then it
> tries to break the layout lease so the IO can proceed. The layout
> breaking code sees that process A has an exclusive layout lease
> granted, and so returns -ETXTBSY to process B - it is not allowed to
> break the lease and so the IO fails with -ETXTBSY.
This description doesn't match the behaviour that RDMA wants either.
Even if Process A has a lease on the file, an IO from Process A which
results in blocks being freed from the file is going to result in the
RDMA device being able to write to blocks which are now freed (and
potentially reallocated to another file).
^ permalink raw reply
* Re: [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI
From: Catalin Marinas @ 2019-06-13 15:26 UTC (permalink / raw)
To: Dave Martin
Cc: Andrey Konovalov, linux-arm-kernel, linux-mm, linux-kernel,
amd-gfx, dri-devel, linux-rdma, linux-media, kvm, linux-kselftest,
Mark Rutland, Szabolcs Nagy, Will Deacon, Kostya Serebryany,
Khalid Aziz, Felix Kuehling, Vincenzo Frascino, Jacob Bramley,
Leon Romanovsky, Christoph Hellwig, Jason Gunthorpe,
Evgeniy Stepanov
In-Reply-To: <20190613110235.GW28398@e103592.cambridge.arm.com>
Hi Dave,
On Thu, Jun 13, 2019 at 12:02:35PM +0100, Dave P Martin wrote:
> On Wed, Jun 12, 2019 at 01:43:20PM +0200, Andrey Konovalov wrote:
> > +/*
> > + * Global sysctl to disable the tagged user addresses support. This control
> > + * only prevents the tagged address ABI enabling via prctl() and does not
> > + * disable it for tasks that already opted in to the relaxed ABI.
> > + */
> > +static int zero;
> > +static int one = 1;
>
> !!!
>
> And these can't even be const without a cast. Yuk.
>
> (Not your fault though, but it would be nice to have a proc_dobool() to
> avoid this.)
I had the same reaction. Maybe for another patch sanitising this pattern
across the kernel.
> > --- a/include/uapi/linux/prctl.h
> > +++ b/include/uapi/linux/prctl.h
> > @@ -229,4 +229,9 @@ struct prctl_mm_map {
> > # define PR_PAC_APDBKEY (1UL << 3)
> > # define PR_PAC_APGAKEY (1UL << 4)
> >
> > +/* Tagged user address controls for arm64 */
> > +#define PR_SET_TAGGED_ADDR_CTRL 55
> > +#define PR_GET_TAGGED_ADDR_CTRL 56
> > +# define PR_TAGGED_ADDR_ENABLE (1UL << 0)
> > +
>
> Do we expect this prctl to be applicable to other arches, or is it
> strictly arm64-specific?
I kept it generic, at least the tagged address part. The MTE bits later
on would be arm64-specific.
> > @@ -2492,6 +2498,16 @@ SYSCALL_DEFINE5(prctl, int, option, unsigned long, arg2, unsigned long, arg3,
> > return -EINVAL;
> > error = PAC_RESET_KEYS(me, arg2);
> > break;
> > + case PR_SET_TAGGED_ADDR_CTRL:
> > + if (arg3 || arg4 || arg5)
>
> <bikeshed>
>
> How do you anticipate these arguments being used in the future?
I don't expect them to be used at all. But since I'm not sure, I'd force
them as zero for now rather than ignored. The GET is supposed to return
the SET arg2, hence I'd rather not used the other arguments.
> For the SVE prctls I took the view that "get" could only ever mean one
> thing, and "put" already had a flags argument with spare bits for future
> expansion anyway, so forcing the extra arguments to zero would be
> unnecessary.
>
> Opinions seem to differ on whether requiring surplus arguments to be 0
> is beneficial for hygiene, but the glibc prototype for prctl() is
>
> int prctl (int __option, ...);
>
> so it seemed annoying to have to pass extra arguments to it just for the
> sake of it. IMHO this also makes the code at the call site less
> readable, since it's not immediately apparent that all those 0s are
> meaningless.
It's fine by me to ignore the other arguments. I just followed the
pattern of some existing prctl options. I don't have a strong opinion
either way.
> > + return -EINVAL;
> > + error = SET_TAGGED_ADDR_CTRL(arg2);
> > + break;
> > + case PR_GET_TAGGED_ADDR_CTRL:
> > + if (arg2 || arg3 || arg4 || arg5)
> > + return -EINVAL;
> > + error = GET_TAGGED_ADDR_CTRL();
>
> Having a "get" prctl is probably a good idea, but is there a clear
> usecase for it?
Not sure, maybe some other library (e.g. a JIT compiler) would like to
check whether tagged addresses have been enabled during application
start and decide to generate tagged pointers for itself. It seemed
pretty harmless, unless we add more complex things to the prctl() that
cannot be returned in one request).
--
Catalin
^ 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