Messages in this thread | | | Date | Tue, 20 Sep 2022 13:21:33 +0300 | Subject | Re: [PATCH] mm: Make failslab writable again | From | Alexander Atanasov <> |
| |
On 20.09.22 12:29, Vlastimil Babka wrote: > On 9/20/22 11:17, Alexander Atanasov wrote: >> Hello, >> >> On 20.09.22 11:42, Vlastimil Babka wrote: >>>> +static ssize_t failslab_store(struct kmem_cache *s, const char *buf, >>>> + size_t length) >>>> +{ >>>> + if (s->refcount > 1) >>>> + return -EINVAL; >>>> + >>>> + s->flags &= ~SLAB_FAILSLAB; >>>> + if (buf[0] == '1') >>>> + s->flags |= SLAB_FAILSLAB; >>> >>> Could we at least use a temporary variable to set up the final value and >>> then do a WRITE_ONCE() to s->flags, so the compiler is not allowed to do >>> some funky stuff? Assuming this is really the only place where we modify >>> s->flags during runtime, so we can't miss other updates due to RMW. >> >> Since it is set or clear - instead of temporary variable and potentially two >> writes and RMW issues i would suggest this: >> + if (buf[0] == '1') >> + s->flags |= SLAB_FAILSLAB; >> + else >> + s->flags &= ~SLAB_FAILSLAB; > > This way also has RMW issues, and also the compiler is allowed to > temporarily modify s->flags any way it likes; with WRITE_ONCE() it can't.
Okay, so the safest way is this?
if (buf[0] == '1') WRITE_ONCE(s->flags, READ_ONCE(s->flags) | SLAB_FAILSLAB); else WRITE_ONCE(s->flags, READ_ONCE(s->flags) & ~SLAB_FAILSLAB);
It got me thinking how many places would break if the compiler starts to temporariliy modify the flags - i hope it never does.
-- Regards, Alexander Atanasov
| |