Should I wait until atomic_read_ctrl patch is landed and update my patch?
On Fri, Sep 18, 2015 at 1:56 PM, Peter Zijlstra wrote:
> On Fri, Sep 18, 2015 at 01:50:01PM +0200, Dmitry Vyukov wrote:
>> > +#ifndef atomic_read_ctrl
>> > +static inline int atomic_read_ctrl(atomic_t *v)
>> > +{
>> > +
On Fri, Sep 18, 2015 at 10:57:32AM +0200, Peter Zijlstra wrote:
> On Fri, Sep 18, 2015 at 10:51:56AM +0200, Peter Zijlstra wrote:
> > That would upgrade the atomic_read() path to a full READ_ONCE_CTRL(),
> > and thereby avoid any of the kmem_cache_free() stores from leaking out.
>
> That said, on
On Fri, Sep 18, 2015 at 03:46:30PM +0200, Peter Zijlstra wrote:
> On Fri, Sep 18, 2015 at 03:28:44PM +0200, Oleg Nesterov wrote:
> > On 09/18, Peter Zijlstra wrote:
> > >
> > > On Thu, Sep 17, 2015 at 08:09:19PM +0200, Oleg Nesterov wrote:
> > >
> > > > I need to recheck, but afaics this is not pos
On Fri, Sep 18, 2015 at 09:15:34AM -0700, Eric Dumazet wrote:
> On Fri, 2015-09-18 at 13:22 +0200, Peter Zijlstra wrote:
> > +static inline int atomic_read_ctrl(atomic_t *v)
>
> const atomic_t *v
Good point, lemme also fix up ATOMIC_LONG_READ_OP().
--
To unsubscribe from this list: send the line
On Fri, 2015-09-18 at 13:22 +0200, Peter Zijlstra wrote:
...
>
> +#ifndef atomic_read_ctrl
> +static inline int atomic_read_ctrl(atomic_t *v)
const atomic_t *v
> +{
> + int val = atomic_read(v);
> + smp_read_barrier_depends(); /* Enforce control dependency. */
> + return val;
> +}
On Fri, Sep 18, 2015 at 10:28:20AM +0100, Will Deacon wrote:
> On Fri, Sep 18, 2015 at 10:06:46AM +0100, Dmitry Vyukov wrote:
> > On Fri, Sep 18, 2015 at 10:51 AM, Peter Zijlstra
> > wrote:
> > > As an alternative patch, could we not do:
> > >
> > > void put_pid(struct pid *pid)
> > > {
> > >
I can't resist. You certainly don't need this spam, but
On 09/18, Peter Zijlstra wrote:
>
> On Fri, Sep 18, 2015 at 03:28:44PM +0200, Oleg Nesterov wrote:
> >
> > And again, I simply can't understand if this code
> >
> > if (READ_ONCE_CTRL(Y))
> > BUG_ON(X == 0);
> >
> > to me it d
On 09/18, Peter Zijlstra wrote:
>
> On Fri, Sep 18, 2015 at 03:28:44PM +0200, Oleg Nesterov wrote:
> > On 09/18, Peter Zijlstra wrote:
> > >
> > > ns = pid->numbers[pid->level].ns;
> > > if ((atomic_read(&pid->count) == 1) ||
> > >atomic_dec_and_test(&pid->count)) {
> > >
> > > +
On 09/18, Dmitry Vyukov wrote:
>
> On Fri, Sep 18, 2015 at 3:44 PM, Oleg Nesterov wrote:
> > So I assume that if we have
> >
> > int X = 0;
> > atomic_t Y = ATOMIC_INIT(0);
> >
> > void w(void)
> > {
> > X = 1;
> > atomic_inc_return(&
On Fri, Sep 18, 2015 at 03:44:53PM +0200, Oleg Nesterov wrote:
> On 09/18, Peter Zijlstra wrote:
> > +static inline int atomic_read_ctrl(atomic_t *v)
> > +{
> > + int val = atomic_read(v);
> > + smp_read_barrier_depends(); /* Enforce control dependency. */
> > + return val;
> > +}
>
> Help.
On Fri, Sep 18, 2015 at 3:44 PM, Oleg Nesterov wrote:
> On 09/18, Peter Zijlstra wrote:
>>
>> Provide atomic_read_ctrl() to mirror READ_ONCE_CTRL(), such that we can
>> more conveniently use atomics in control dependencies.
>>
>> Since we can assume atomic_read() implies a READ_ONCE(), we must onl
On Fri, Sep 18, 2015 at 03:28:44PM +0200, Oleg Nesterov wrote:
> On 09/18, Peter Zijlstra wrote:
> >
> > On Thu, Sep 17, 2015 at 08:09:19PM +0200, Oleg Nesterov wrote:
> >
> > > I need to recheck, but afaics this is not possible. This optimization
> > > is fine, but probably needs a comment.
> >
>
On 09/18, Peter Zijlstra wrote:
>
> Provide atomic_read_ctrl() to mirror READ_ONCE_CTRL(), such that we can
> more conveniently use atomics in control dependencies.
>
> Since we can assume atomic_read() implies a READ_ONCE(), we must only
> emit an extra smp_read_barrier_depends() in order to upgra
Damn, sorry for noise,
On 09/18, Oleg Nesterov wrote:
>
> Now. In theory this this code is wrong:
>
> if (Y) {
> BUG_ON(X == 0);
> }
Of course without READ_ONCE() or barrier() in between this code
is buggy in any case. But I hope you understand what I tried to
say...
Ol
On 09/18, Peter Zijlstra wrote:
>
> On Thu, Sep 17, 2015 at 08:09:19PM +0200, Oleg Nesterov wrote:
>
> > I need to recheck, but afaics this is not possible. This optimization
> > is fine, but probably needs a comment.
>
> For sure, this code doesn't make any sense to me.
So yes, after a sleep I am
On Fri, Sep 18, 2015 at 01:44:42PM +0100, Will Deacon wrote:
> On Fri, Sep 18, 2015 at 01:19:20PM +0100, Peter Zijlstra wrote:
> > On Fri, Sep 18, 2015 at 01:56:37PM +0200, Peter Zijlstra wrote:
> > > +#ifndef atomic64_read_ctrl
> > > +static inline int atomic64_read_ctrl(atomic64_t *v)
> > > +{
>
On Fri, Sep 18, 2015 at 01:19:20PM +0100, Peter Zijlstra wrote:
> On Fri, Sep 18, 2015 at 01:56:37PM +0200, Peter Zijlstra wrote:
> > +#ifndef atomic64_read_ctrl
> > +static inline int atomic64_read_ctrl(atomic64_t *v)
> > +{
> > + int val = atomic64_read(v);
>
> Duh
>
> long long...
...
On Fri, Sep 18, 2015 at 01:31:27PM +0100, James Hogan wrote:
> Hi Peter,
>
> On Fri, Sep 18, 2015 at 11:27:32AM +0200, Peter Zijlstra wrote:
> > On Fri, Sep 18, 2015 at 10:57:32AM +0200, Peter Zijlstra wrote:
> > > diff --git a/arch/metag/include/asm/atomic_lnkget.h
> > > b/arch/metag/include/asm
Hi Peter,
On Fri, Sep 18, 2015 at 11:27:32AM +0200, Peter Zijlstra wrote:
> On Fri, Sep 18, 2015 at 10:57:32AM +0200, Peter Zijlstra wrote:
> > diff --git a/arch/metag/include/asm/atomic_lnkget.h
> > b/arch/metag/include/asm/atomic_lnkget.h
> > index 21c4c268b86c..1bd21c933435 100644
> > --- a/ar
On Fri, Sep 18, 2015 at 01:56:37PM +0200, Peter Zijlstra wrote:
> +#ifndef atomic64_read_ctrl
> +static inline int atomic64_read_ctrl(atomic64_t *v)
> +{
> + int val = atomic64_read(v);
Duh
long long...
> + smp_read_barrier_depends(); /* Enforce control dependency. */
> + ret
On Fri, Sep 18, 2015 at 01:50:01PM +0200, Dmitry Vyukov wrote:
> > +#ifndef atomic_read_ctrl
> > +static inline int atomic_read_ctrl(atomic_t *v)
> > +{
> > + int val = atomic_read(v);
> > + smp_read_barrier_depends(); /* Enforce control dependency. */
> > + return val;
> > +}
> >
On Fri, Sep 18, 2015 at 1:22 PM, Peter Zijlstra wrote:
> On Fri, Sep 18, 2015 at 10:28:20AM +0100, Will Deacon wrote:
>> On Fri, Sep 18, 2015 at 10:06:46AM +0100, Dmitry Vyukov wrote:
>
>> > Can we have something along the lines of:
>> >
>> > #define atomic_read_ctrl(v) READ_ONCE_CTRL(&(v)->counte
On Fri, Sep 18, 2015 at 12:22:52PM +0100, Peter Zijlstra wrote:
> On Fri, Sep 18, 2015 at 10:28:20AM +0100, Will Deacon wrote:
> > On Fri, Sep 18, 2015 at 10:06:46AM +0100, Dmitry Vyukov wrote:
>
> > > Can we have something along the lines of:
> > >
> > > #define atomic_read_ctrl(v) READ_ONCE_CTR
On Fri, Sep 18, 2015 at 10:28:20AM +0100, Will Deacon wrote:
> On Fri, Sep 18, 2015 at 10:06:46AM +0100, Dmitry Vyukov wrote:
> > Can we have something along the lines of:
> >
> > #define atomic_read_ctrl(v) READ_ONCE_CTRL(&(v)->counter)
>
> Funnily enough, I had this exact same discussion off-l
On Fri, Sep 18, 2015 at 10:28:20AM +0100, Will Deacon wrote:
> > #define atomic_read_ctrl(v) READ_ONCE_CTRL(&(v)->counter)
>
> Funnily enough, I had this exact same discussion off-list yesterday
> afternoon, since I wrote some code relying on a ctrl dependency from
> an atomic_read to an atomic_xc
On Fri, Sep 18, 2015 at 10:57:32AM +0200, Peter Zijlstra wrote:
> diff --git a/arch/metag/include/asm/atomic_lnkget.h
> b/arch/metag/include/asm/atomic_lnkget.h
> index 21c4c268b86c..1bd21c933435 100644
> --- a/arch/metag/include/asm/atomic_lnkget.h
> +++ b/arch/metag/include/asm/atomic_lnkget.h
>
On Fri, Sep 18, 2015 at 10:06:46AM +0100, Dmitry Vyukov wrote:
> On Fri, Sep 18, 2015 at 10:51 AM, Peter Zijlstra wrote:
> > As an alternative patch, could we not do:
> >
> > void put_pid(struct pid *pid)
> > {
> > struct pid_namespace *ns;
> >
> > if (!pid)
> >
On Fri, Sep 18, 2015 at 10:51 AM, Peter Zijlstra wrote:
> On Thu, Sep 17, 2015 at 08:09:19PM +0200, Oleg Nesterov wrote:
>> On 09/17, Dmitry Vyukov wrote:
>> >
>> > I can update the patch description, but let me explain it here first.
>>
>> Yes thanks.
>>
>> > Here is the essence of what happens:
On Fri, Sep 18, 2015 at 10:51:56AM +0200, Peter Zijlstra wrote:
> That would upgrade the atomic_read() path to a full READ_ONCE_CTRL(),
> and thereby avoid any of the kmem_cache_free() stores from leaking out.
That said, on my TODO list is an item to review all atomic_{read,set}()
implementation t
On Thu, Sep 17, 2015 at 08:09:19PM +0200, Oleg Nesterov wrote:
> On 09/17, Dmitry Vyukov wrote:
> >
> > I can update the patch description, but let me explain it here first.
>
> Yes thanks.
>
> > Here is the essence of what happens:
>
> Aha, so you really meant that 2 put_pid's can race with eac
On Thu, Sep 17, 2015 at 8:09 PM, Oleg Nesterov wrote:
> On 09/17, Dmitry Vyukov wrote:
>>
>> I can update the patch description, but let me explain it here first.
>
> Yes thanks.
>
>> Here is the essence of what happens:
>
> Aha, so you really meant that 2 put_pid's can race with each other,
>
>>
On 09/17, Dmitry Vyukov wrote:
>
> I can update the patch description, but let me explain it here first.
Yes thanks.
> Here is the essence of what happens:
Aha, so you really meant that 2 put_pid's can race with each other,
> // thread 1
> 1: pid->foo = 1; // foo is the first word of pid object
On Thu, Sep 17, 2015 at 7:57 PM, Dmitry Vyukov wrote:
> I can update the patch description, but let me explain it here first.
>
> Here is the essence of what happens:
>
> // thread 1
> 1: pid->foo = 1; // foo is the first word of pid object
> // then it does put_pid
> 2: atomic_dec_and_test(&pid->
I can update the patch description, but let me explain it here first.
Here is the essence of what happens:
// thread 1
1: pid->foo = 1; // foo is the first word of pid object
// then it does put_pid
2: atomic_dec_and_test(&pid->count) // decrements count to 1 and
returns false so the function ret
On Thu, Sep 17, 2015 at 06:41:46PM +0200, Dmitry Vyukov wrote:
> What happens here exactly matches what is described in CONTROL
> DEPENDENCIES section of Documentation/memory-barriers.txt. So all the
> bad things described there are possible here. The document explicitly
> requires usage of rmb/acq
On 09/17, Dmitry Vyukov wrote:
>
> What happens here exactly matches what is described in CONTROL
> DEPENDENCIES section of Documentation/memory-barriers.txt. So all the
> bad things described there are possible here.
And I still can't understand how these bad things connect to put_pid().
Probably
What happens here exactly matches what is described in CONTROL
DEPENDENCIES section of Documentation/memory-barriers.txt. So all the
bad things described there are possible here. The document explicitly
requires usage of rmb/acquire/READ_ONCE_CTRL in such cases. I don't
know what to add to that.
R
Honestly, I can not see how this can happen. So I do not really
understand the problem and the fix.
And if this can happen I can't understand how this patch can help.
What about "ns = pid->numbers[pid->level].ns" ? It can be reordered
with atomic_read_acquire().
I leave this to other reviewers, b
put_pid checks whether the current thread has the only reference
to the pid with atomic_read() which does not have any memory
barriers, and if so proceeds directly to kmem_cache_free().
As the result memory accesses to the object in kmem_cache_free()
or user accesses to the object after reallocatio
39 matches
Mail list logo
