On Mon 2009-05-04 13:29:22, Michael Tokarev wrote:
> Is there any reason why 32-bit uswsusp &Friends does not work
> on 64bits kernel?
>
> For one, 32bits s2disk produces the following when trying to
> suspend:
>
> ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(400c330d){t:'3';sz:12} arg(ff853554) on /dev/snapshot
> ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(4004330a){t:'3';sz:4} arg(00000805) on /dev/snapshot
>
> this is coming from:
>
> error = ioctl(dev, SNAPSHOT_SET_SWAP_AREA, &swap);
> if (error && !offset)
> error = ioctl(dev, SNAPSHOT_SET_SWAP_FILE, blkdev);
>
> but I guess (just guess!) that other SNAPSHOT_* operations will
> also fail the same way.
>
> Is there a reason why those are not in compat_ioctl?
Lazyness...?
Patch would be welcome. On 4GB machine, running 64bit kernel (but
staying with 32bit userland) makes sense...
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek wrote:
> On Mon 2009-05-04 13:29:22, Michael Tokarev wrote:
>> Is there any reason why 32-bit uswsusp &Friends does not work
>> on 64bits kernel?
>>
>> For one, 32bits s2disk produces the following when trying to
>> suspend:
>>
>> ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(400c330d){t:'3';sz:12} arg(ff853554) on /dev/snapshot
>> ioctl32(s2disk:4134): Unknown cmd fd(4) cmd(4004330a){t:'3';sz:4} arg(00000805) on /dev/snapshot
>>
>> this is coming from:
>>
>> error = ioctl(dev, SNAPSHOT_SET_SWAP_AREA, &swap);
>> if (error && !offset)
>> error = ioctl(dev, SNAPSHOT_SET_SWAP_FILE, blkdev);
>>
>> but I guess (just guess!) that other SNAPSHOT_* operations will
>> also fail the same way.
>>
>> Is there a reason why those are not in compat_ioctl?
>
> Lazyness...?
>
> Patch would be welcome.
Pavel, you really should take a look at the original thread.
As I mentioned in my first email, I'm not the right person to
do the patch. But regardless, I spent about a day understanding
this stuff (or trying to, anyway) - 100% useless day - and when
I thought I have a patch someone else spoken up and said this
way (compat_ioctl) is the wrong approach now. And sent another,
also trivial patch, adding compat calls right to the proper
place in kernel/power.c. Which (the patch) has been ignored
too.
So "welcome" is a wrong word here, and I'm sorry about that.
> On 4GB machine, running 64bit kernel (but
> staying with 32bit userland) makes sense...
This is exactly what I'm running here by the way.
But regardless, uswsusp should be fixed too before it will
be useful for that. And fixing uswsusp is not trivial,
unlike kernel side. But having in mind amount of useless
jumping needed just for the trivial kernel part, for a
20-minute patch, -- there's not much hope really. (I
wanted to fix it too, initially, -- not any more).
/mjt
On Sunday 12 July 2009, Michael Tokarev wrote:
> As I mentioned in my first email, I'm not the right person to
> do the patch. But regardless, I spent about a day understanding
> this stuff (or trying to, anyway) - 100% useless day - and when
> I thought I have a patch someone else spoken up and said this
> way (compat_ioctl) is the wrong approach now. And sent another,
> also trivial patch, adding compat calls right to the proper
> place in kernel/power.c. Which (the patch) has been ignored
> too.
I never got a reply from you saying whether or not the patch
I sent actually worked. If you or someone else can confirm it,
I'll resend with the fixes you mentioned.
Arnd <><
Arnd Bergmann wrote:
> On Sunday 12 July 2009, Michael Tokarev wrote:
>> As I mentioned in my first email, I'm not the right person to
>> do the patch. But regardless, I spent about a day understanding
>> this stuff (or trying to, anyway) - 100% useless day - and when
>> I thought I have a patch someone else spoken up and said this
>> way (compat_ioctl) is the wrong approach now. And sent another,
>> also trivial patch, adding compat calls right to the proper
>> place in kernel/power.c. Which (the patch) has been ignored
>> too.
>
> I never got a reply from you saying whether or not the patch
> I sent actually worked. If you or someone else can confirm it,
> I'll resend with the fixes you mentioned.
In order to (try to) check if it works or not, another userspace
component has to be fixed to support 32/64 bit mode. It's uswsusp,
which currently assumes swap space structures are all 32bits. So
it isn't possible to immediately check if it works or not -- just
ioctl(s) aren't enough. Complete fix (kernel+user space) requires
both, fixing all remaining (yet unknown) issues in old and new
code on the way.
For now, I think it's best to let Pavel or Rafael to decide what
to do with all this.
Thanks!
/mjt
On Mon 2009-07-13 10:51:15, Michael Tokarev wrote:
> Arnd Bergmann wrote:
>> On Sunday 12 July 2009, Michael Tokarev wrote:
>>> As I mentioned in my first email, I'm not the right person to
>>> do the patch. But regardless, I spent about a day understanding
>>> this stuff (or trying to, anyway) - 100% useless day - and when
>>> I thought I have a patch someone else spoken up and said this
>>> way (compat_ioctl) is the wrong approach now. And sent another,
>>> also trivial patch, adding compat calls right to the proper
>>> place in kernel/power.c. Which (the patch) has been ignored
>>> too.
>>
>> I never got a reply from you saying whether or not the patch
>> I sent actually worked. If you or someone else can confirm it,
>> I'll resend with the fixes you mentioned.
>
> In order to (try to) check if it works or not, another userspace
> component has to be fixed to support 32/64 bit mode. It's uswsusp,
> which currently assumes swap space structures are all 32bits. So
> it isn't possible to immediately check if it works or not -- just
> ioctl(s) aren't enough. Complete fix (kernel+user space) requires
> both, fixing all remaining (yet unknown) issues in old and new
> code on the way.
Well, there seems to be single structure in s2disk; that does not seem
that hard to fix.
> For now, I think it's best to let Pavel or Rafael to decide what
> to do with all this.
I don't currently have easy access to 64bit machine, so I guess it is
up to someone else.
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek wrote:
[]
>> In order to (try to) check if it works or not, another userspace
>> component has to be fixed to support 32/64 bit mode. It's uswsusp,
>> which currently assumes swap space structures are all 32bits. So
>> it isn't possible to immediately check if it works or not -- just
>> ioctl(s) aren't enough. Complete fix (kernel+user space) requires
>> both, fixing all remaining (yet unknown) issues in old and new
>> code on the way.
>
> Well, there seems to be single structure in s2disk; that does not seem
> that hard to fix.
Which one do you mean?
Also, do you want to make it 32/64 bit clean in userspace too, so that
an image produced by 32-bit s2disk can be read by 64bit resume and
vise versa? Sure it's good thing to have, to avoid possible issues
with 32bit initramfs on 64bit system for example...
>> For now, I think it's best to let Pavel or Rafael to decide what
>> to do with all this.
>
> I don't currently have easy access to 64bit machine, so I guess it is
> up to someone else.
[]
Ok. I applied Arnd's patch again (with two fixes -- adding
#include <linux/compat.h> and s/compat_ulong/compat_ulong_t/ and
tried suspend/resume cycle with unmodified uswsusp-0.8. Suspend
worked (seemengly - it reported about 4G pages written, on a
machine with 4G memory) but resume failed after reading all the
pages and submitting them to /dev/snapshot.
I've no time right now to debug it further (and again, yet again:
I don't know the code, neither kernel nor userspace part).
So I'd merge this change for now and deal with possible bugs later.
At least there's no failed ioctl()s on /dev/snapshot anymore,
neither at suspend nor at resume time.
/mjt
On Tue 2009-07-14 10:57:06, Michael Tokarev wrote:
> Pavel Machek wrote:
> []
>>> In order to (try to) check if it works or not, another userspace
>>> component has to be fixed to support 32/64 bit mode. It's uswsusp,
>>> which currently assumes swap space structures are all 32bits. So
>>> it isn't possible to immediately check if it works or not -- just
>>> ioctl(s) aren't enough. Complete fix (kernel+user space) requires
>>> both, fixing all remaining (yet unknown) issues in old and new
>>> code on the way.
>>
>> Well, there seems to be single structure in s2disk; that does not seem
>> that hard to fix.
>
> Which one do you mean?
struct swsusp_info should be the one...
> Also, do you want to make it 32/64 bit clean in userspace too, so that
> an image produced by 32-bit s2disk can be read by 64bit resume and
> vise versa? Sure it's good thing to have, to avoid possible issues
> with 32bit initramfs on 64bit system for example...
It would be cool, but...
>>> For now, I think it's best to let Pavel or Rafael to decide what
>>> to do with all this.
>>
>> I don't currently have easy access to 64bit machine, so I guess it is
>> up to someone else.
> []
>
> Ok. I applied Arnd's patch again (with two fixes -- adding
> #include <linux/compat.h> and s/compat_ulong/compat_ulong_t/ and
> tried suspend/resume cycle with unmodified uswsusp-0.8. Suspend
> worked (seemengly - it reported about 4G pages written, on a
> machine with 4G memory) but resume failed after reading all the
> pages and submitting them to /dev/snapshot.
>
> I've no time right now to debug it further (and again, yet again:
> I don't know the code, neither kernel nor userspace part).
>
> So I'd merge this change for now and deal with possible bugs later.
>
> At least there's no failed ioctl()s on /dev/snapshot anymore,
> neither at suspend nor at resume time.
ACK.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html