Re: [PATCH 2/2] uswsusp: build only when configured

[Domenico Andreoli <domenico.andreoli@linux.com>]

On Fri, May 01, 2020 at 04:54:13PM +0200, Rafael J. Wysocki wrote:
> On Fri, May 1, 2020 at 9:10 AM Domenico Andreoli
> <domenico.andreoli@linux.com> wrote:
> >
> > On Wed, Apr 29, 2020 at 01:20:53PM +0200, Rafael J. Wysocki wrote:
> > > On Mon, Apr 27, 2020 at 11:48 AM Domenico Andreoli <domenico.andreoli@linux.com> wrote:
> > > >
> > > > On Sun, Apr 26, 2020 at 06:16:29PM +0200, Rafael J. Wysocki wrote:
> > > > >
> >
> > [...]
> >
> > > > >
> > > > > It is possible in theory that two processes write "disk" to /sys/power/state
> > > > > concurrently.
> > > > >
> > > > > Is there enough mutual exclusion in place to handle this gracefully after the
> > > > > above change?
> > > >
> > > > No, indeed.
> > > >
> > > > It looks like hibernate.c needs the mutual exclusion and user.c could
> > > > just use it. Should I move snapshot_device_available to hibernate.c
> > > > and rename it hibernate_available?
> > >
> > > There is hibernation_available() already.
> > >
> > > Maybe switch over to the refcount_t API, call the variable
> > > hibernate_refcount and use refcount_add_not_zero() on it for the
> > > mutual exclusion.
> >
> > I'm doing as you ask but I'm not understanding what we actually gain
> > from using the refcount_t API.
> >
> > I'm reading about relaxation of memory ordering and there is no mention
> > on what this implies for the add_not_zero operation that we use.
> 
> The details probably don't matter, but what we use here effectively is
> a refcount which is not allowed to grow above 1,
> 
> That's why it'd be reasonable to explicitly define it as a refcount,
> now that there is a suitable API.

The logic above looks fine to me and AFICT I implemented it in
https://lore.kernel.org/linux-pm/20200501152304.523890160@gmail.com/.

What I noticed only after I posted the patch, it triggers a warning
(the ">>>>>>" traces are only in my local code):

| May  3 15:06:10 dumbo kernel: [  318.272438] >>>>>>>>>> release refcount-pre 3221225472
| May  3 15:06:10 dumbo kernel: [  318.272441] ------------[ cut here ]------------
| May  3 15:06:10 dumbo kernel: [  318.272442] refcount_t: saturated; leaking memory.
| ...
| May  3 15:06:10 dumbo kernel: [  318.272531] Call Trace:
| May  3 15:06:10 dumbo kernel: [  318.272537]  hibernate_release+0x52/0x64
| May  3 15:06:10 dumbo kernel: [  318.272540]  snapshot_release+0x47/0x70
| May  3 15:06:10 dumbo kernel: [  318.272545]  __fput+0xe1/0x250
| May  3 15:06:10 dumbo kernel: [  318.272547]  task_work_run+0x76/0xb0
| May  3 15:06:10 dumbo kernel: [  318.272551]  exit_to_usermode_loop+0xeb/0xf0
| May  3 15:06:10 dumbo kernel: [  318.272554]  do_syscall_64+0x162/0x180
| May  3 15:06:10 dumbo kernel: [  318.272558]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
| May  3 15:06:10 dumbo kernel: [  318.272560] RIP: 0033:0x7fc0c064eb54
| ...
| May  3 15:06:10 dumbo kernel: [  318.272570] ---[ end trace 9b4a89152f05edb2 ]---
| May  3 15:06:10 dumbo kernel: [  318.272571] >>>>>>>>>> release refcount-port 3221225472

If I switch back to atomic_t, I get the expected values (my traces of
two hibernation cycles):

| [   42.836678] >>>>>>>>>> acquire refcount-pre 1
| [   42.836683] >>>>>>>>>> acquire refcount-post 0
| [   47.313636] >>>>>>>>>> release refcount-pre 0
| [   47.313638] >>>>>>>>>> release refcount-post 1
| [   58.069508] >>>>>>>>>> acquire refcount-pre 1
| [   58.069513] >>>>>>>>>> acquire refcount-post 0
| [   63.661207] >>>>>>>>>> release refcount-pre 0
| [   63.661209] >>>>>>>>>> release refcount-post 1

I'm still trying to understand why this difference between refcount_t
and atomic_t in our context. I must be missing something big.

Thanks,
Dom

-- 
rsa4096: 3B10 0CA1 8674 ACBA B4FE  FCD2 CE5B CF17 9960 DE13
ed25519: FFB4 0CC3 7F2E 091D F7DA  356E CC79 2832 ED38 CB05