* [RFC PATCH] locks:Remove spinlock in unshare_files
@ 2020-03-13 3:10 ling.ma.program
2020-03-16 13:25 ` Ling Ma
0 siblings, 1 reply; 4+ messages in thread
From: ling.ma.program @ 2020-03-13 3:10 UTC (permalink / raw)
To: linux-kernel; +Cc: ling.ml
From: Ma Ling <ling.ml@antfin.com>
Processor support atomic operation for long/int/short/char type,
we use the feature to avoid spinlock, which cost hundreds cycles.
Appreciate your comments
Ling
Signed-off-by: Ma Ling <ling.ml@antfin.com>
---
kernel/fork.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 60a1295..fe54600 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -3041,9 +3041,7 @@ int unshare_files(struct files_struct **displaced)
return error;
}
*displaced = task->files;
- task_lock(task);
- task->files = copy;
- task_unlock(task);
+ WRITE_ONCE(task->files, copy);
return 0;
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] locks:Remove spinlock in unshare_files
2020-03-13 3:10 [RFC PATCH] locks:Remove spinlock in unshare_files ling.ma.program
@ 2020-03-16 13:25 ` Ling Ma
2020-03-16 13:39 ` Peter Zijlstra
0 siblings, 1 reply; 4+ messages in thread
From: Ling Ma @ 2020-03-16 13:25 UTC (permalink / raw)
To: linux-kernel, Peter Zijlstra, Ingo Molnar; +Cc: ling.ma
Any comments ?
Thanks
Ling
<ling.ma.program@gmail.com> 于2020年3月13日周五 上午11:09写道:
>
> From: Ma Ling <ling.ml@antfin.com>
>
> Processor support atomic operation for long/int/short/char type,
> we use the feature to avoid spinlock, which cost hundreds cycles.
>
> Appreciate your comments
> Ling
>
> Signed-off-by: Ma Ling <ling.ml@antfin.com>
> ---
> kernel/fork.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 60a1295..fe54600 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -3041,9 +3041,7 @@ int unshare_files(struct files_struct **displaced)
> return error;
> }
> *displaced = task->files;
> - task_lock(task);
> - task->files = copy;
> - task_unlock(task);
> + WRITE_ONCE(task->files, copy);
> return 0;
> }
>
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] locks:Remove spinlock in unshare_files
2020-03-16 13:25 ` Ling Ma
@ 2020-03-16 13:39 ` Peter Zijlstra
2020-03-16 18:35 ` Al Viro
0 siblings, 1 reply; 4+ messages in thread
From: Peter Zijlstra @ 2020-03-16 13:39 UTC (permalink / raw)
To: Ling Ma; +Cc: linux-kernel, Ingo Molnar, ling.ma, viro
On Mon, Mar 16, 2020 at 09:25:42PM +0800, Ling Ma wrote:
> Any comments ?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?
Also, it probably helps to Cc the right people.
> <ling.ma.program@gmail.com> 于2020年3月13日周五 上午11:09写道:
> >
> > From: Ma Ling <ling.ml@antfin.com>
> >
> > Processor support atomic operation for long/int/short/char type,
> > we use the feature to avoid spinlock, which cost hundreds cycles.
> >
> > Appreciate your comments
> > Ling
> >
> > Signed-off-by: Ma Ling <ling.ml@antfin.com>
> > ---
> > kernel/fork.c | 4 +---
> > 1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 60a1295..fe54600 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -3041,9 +3041,7 @@ int unshare_files(struct files_struct **displaced)
> > return error;
> > }
> > *displaced = task->files;
> > - task_lock(task);
> > - task->files = copy;
> > - task_unlock(task);
> > + WRITE_ONCE(task->files, copy);
> > return 0;
> > }
AFAICT this is completely and utterly buggered.
IFF task->files was lockless, like say RCU, then you'd still need
smp_store_release(). But if we look at fs/file.c then everything uses
task_lock() and removing it like the above is actively broken.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] locks:Remove spinlock in unshare_files
2020-03-16 13:39 ` Peter Zijlstra
@ 2020-03-16 18:35 ` Al Viro
0 siblings, 0 replies; 4+ messages in thread
From: Al Viro @ 2020-03-16 18:35 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Ling Ma, linux-kernel, Ingo Molnar, ling.ma
On Mon, Mar 16, 2020 at 02:39:16PM +0100, Peter Zijlstra wrote:
> > > diff --git a/kernel/fork.c b/kernel/fork.c
> > > index 60a1295..fe54600 100644
> > > --- a/kernel/fork.c
> > > +++ b/kernel/fork.c
> > > @@ -3041,9 +3041,7 @@ int unshare_files(struct files_struct **displaced)
> > > return error;
> > > }
> > > *displaced = task->files;
> > > - task_lock(task);
> > > - task->files = copy;
> > > - task_unlock(task);
> > > + WRITE_ONCE(task->files, copy);
> > > return 0;
> > > }
>
> AFAICT this is completely and utterly buggered.
>
> IFF task->files was lockless, like say RCU, then you'd still need
> smp_store_release(). But if we look at fs/file.c then everything uses
> task_lock() and removing it like the above is actively broken.
The problem is not fs/file.c; it's the code that does (read-only)
access to *other* threads' ->files. procfs, SAK, some cgroup
shite (pardon the redundancy)... All of those rely upon task_lock.
FWIW, having just grepped around, I'm worried about the crap io_uring
is pulling off - interplay with unshare(2) could be unpleasant.
In any case - task_lock in the code that assigns to ->files (and it's
not just unshare_files()) serves to protect the 3rd-party readers
(including get_files_struct()) from having the fucker taken apart
under them. It's not just freeing the thing - it's the entire
close_files().
And no, we do *NOT* want to convert everything to get_files_struct() +
being clever in it. I would rather have get_files_struct() taken
out and shot, TBH - the only real reason it hadn't been killed years
ago is the loop in proc_readfd_common()...
I'd prefer to have 3rd-party readers indicate their interest
in a way that would be distinguishable from normal references,
with close_files() waiting until all of those are gone. One way
to do that would be
* secondary counter in files_struct
* rcu-delayed freeing of actual structure (not a problem)
* rcu_read_lock in 3rd-party readers (among other things
it means that proc_readfd_common() would need to be rearchitected
a bit)
* close_files() starting with subtraction of large constant
from the secondary counter and then spinning until it gets to
-<large constant>
* 3rd-party readers (under rcu_read_lock()) fetching task->files,
bumping the secondary counter unless it's negative, doing their thing,
then decrementing the counter.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-03-16 18:37 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-13 3:10 [RFC PATCH] locks:Remove spinlock in unshare_files ling.ma.program
2020-03-16 13:25 ` Ling Ma
2020-03-16 13:39 ` Peter Zijlstra
2020-03-16 18:35 ` Al Viro
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).