2020-10-30 12:42:14

by Alejandro Colomar

[permalink] [raw]
Subject: [PATCH 1/2] futex.2: srcfix

Signed-off-by: Alejandro Colomar <[email protected]>
---
man2/futex.2 | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/man2/futex.2 b/man2/futex.2
index f82602c11..837adbd25 100644
--- a/man2/futex.2
+++ b/man2/futex.2
@@ -25,8 +25,8 @@ futex \- fast user-space locking
.SH SYNOPSIS
.nf
.PP
-.B "#include <linux/futex.h>"
-.B "#include <sys/time.h>"
+.B #include <linux/futex.h>
+.B #include <sys/time.h>
.PP
.BI "int futex(int *" uaddr ", int " futex_op ", int " val ,
.BI " const struct timespec *" timeout , \
--
2.28.0


2020-10-30 12:44:44

by Alejandro Colomar

[permalink] [raw]
Subject: [PATCH 2/2] futex.2: Use appropriate types

The Linux kernel uses the following:

kernel/futex.c:3778:
SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
struct __kernel_timespec __user *, utime, u32 __user *, uaddr2,
u32, val3)

Since there is no glibc wrapper, use the same types the kernel uses.

Signed-off-by: Alejandro Colomar <[email protected]>
---
man2/futex.2 | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)

diff --git a/man2/futex.2 b/man2/futex.2
index 837adbd25..73de71623 100644
--- a/man2/futex.2
+++ b/man2/futex.2
@@ -26,12 +26,13 @@ futex \- fast user-space locking
.nf
.PP
.B #include <linux/futex.h>
+.B #include <stdint.h>
.B #include <sys/time.h>
.PP
-.BI "int futex(int *" uaddr ", int " futex_op ", int " val ,
+.BI "long futex(uint32_t *" uaddr ", int " futex_op ", uint32_t " val ,
.BI " const struct timespec *" timeout , \
" \fR /* or: \fBuint32_t \fIval2\fP */"
-.BI " int *" uaddr2 ", int " val3 );
+.BI " uint32_t *" uaddr2 ", uint32_t " val3 );
.fi
.PP
.IR Note :
@@ -581,8 +582,8 @@ any of the two supplied futex words:
.IP
.in +4n
.EX
-int oldval = *(int *) uaddr2;
-*(int *) uaddr2 = oldval \fIop\fP \fIoparg\fP;
+uint32_t oldval = *(uint32_t *) uaddr2;
+*(uint32_t *) uaddr2 = oldval \fIop\fP \fIoparg\fP;
futex(uaddr, FUTEX_WAKE, val, 0, 0, 0);
if (oldval \fIcmp\fP \fIcmparg\fP)
futex(uaddr2, FUTEX_WAKE, val2, 0, 0, 0);
@@ -1765,11 +1766,11 @@ Child (18535) 4
#define errExit(msg) do { perror(msg); exit(EXIT_FAILURE); \e
} while (0)

-static int *futex1, *futex2, *iaddr;
+static uint32_t *futex1, *futex2, *iaddr;

static int
-futex(int *uaddr, int futex_op, int val,
- const struct timespec *timeout, int *uaddr2, int val3)
+futex(uint32_t *uaddr, int futex_op, uint32_t val,
+ const struct timespec *timeout, uint32_t *uaddr2, uint32_t val3)
{
return syscall(SYS_futex, uaddr, futex_op, val,
timeout, uaddr2, val3);
@@ -1779,9 +1780,9 @@ futex(int *uaddr, int futex_op, int val,
become 1, and then set the value to 0. */

static void
-fwait(int *futexp)
+fwait(uint32_t *futexp)
{
- int s;
+ long s;

/* atomic_compare_exchange_strong(ptr, oldval, newval)
atomically performs the equivalent of:
@@ -1794,7 +1795,7 @@ fwait(int *futexp)
while (1) {

/* Is the futex available? */
- const int one = 1;
+ const uint32_t one = 1;
if (atomic_compare_exchange_strong(futexp, &one, 0))
break; /* Yes */

@@ -1811,13 +1812,13 @@ fwait(int *futexp)
so that if the peer is blocked in fpost(), it can proceed. */

static void
-fpost(int *futexp)
+fpost(uint32_t *futexp)
{
- int s;
+ long s;

/* atomic_compare_exchange_strong() was described in comments above */

- const int zero = 0;
+ const uint32_t zero = 0;
if (atomic_compare_exchange_strong(futexp, &zero, 1)) {
s = futex(futexp, FUTEX_WAKE, 1, NULL, NULL, 0);
if (s == \-1)
--
2.28.0

2020-10-30 13:48:54

by Alejandro Colomar

[permalink] [raw]
Subject: Re: [PATCH 2/2] futex.2: Use appropriate types

BTW, apparently the kernel doesn't use 'const' for 'utime'
('timeout' in the manual page),
but effectively, it doesn't modify it, AFAICS.

Should the kernel use 'const'?
Is there a reason for the kernel not using 'const'?
Should we do anything about it in the manual page?

Thanks,

Alex

On 2020-10-30 13:39, Alejandro Colomar wrote:
> The Linux kernel uses the following:
>
> kernel/futex.c:3778:
> SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
> struct __kernel_timespec __user *, utime, u32 __user *, uaddr2,
> u32, val3)
>
> Since there is no glibc wrapper, use the same types the kernel uses.
>
> Signed-off-by: Alejandro Colomar <[email protected]>
> ---
> man2/futex.2 | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/man2/futex.2 b/man2/futex.2
> index 837adbd25..73de71623 100644
> --- a/man2/futex.2
> +++ b/man2/futex.2
> @@ -26,12 +26,13 @@ futex \- fast user-space locking
> .nf
> .PP
> .B #include <linux/futex.h>
> +.B #include <stdint.h>
> .B #include <sys/time.h>
> .PP
> -.BI "int futex(int *" uaddr ", int " futex_op ", int " val ,
> +.BI "long futex(uint32_t *" uaddr ", int " futex_op ", uint32_t " val ,
> .BI " const struct timespec *" timeout , \
> " \fR /* or: \fBuint32_t \fIval2\fP */"
> -.BI " int *" uaddr2 ", int " val3 );
> +.BI " uint32_t *" uaddr2 ", uint32_t " val3 );
> .fi
> .PP
> .IR Note :
> @@ -581,8 +582,8 @@ any of the two supplied futex words:
> .IP
> .in +4n
> .EX
> -int oldval = *(int *) uaddr2;
> -*(int *) uaddr2 = oldval \fIop\fP \fIoparg\fP;
> +uint32_t oldval = *(uint32_t *) uaddr2;
> +*(uint32_t *) uaddr2 = oldval \fIop\fP \fIoparg\fP;
> futex(uaddr, FUTEX_WAKE, val, 0, 0, 0);
> if (oldval \fIcmp\fP \fIcmparg\fP)
> futex(uaddr2, FUTEX_WAKE, val2, 0, 0, 0);
> @@ -1765,11 +1766,11 @@ Child (18535) 4
> #define errExit(msg) do { perror(msg); exit(EXIT_FAILURE); \e
> } while (0)
>
> -static int *futex1, *futex2, *iaddr;
> +static uint32_t *futex1, *futex2, *iaddr;
>
> static int
> -futex(int *uaddr, int futex_op, int val,
> - const struct timespec *timeout, int *uaddr2, int val3)
> +futex(uint32_t *uaddr, int futex_op, uint32_t val,
> + const struct timespec *timeout, uint32_t *uaddr2, uint32_t val3)
> {
> return syscall(SYS_futex, uaddr, futex_op, val,
> timeout, uaddr2, val3);
> @@ -1779,9 +1780,9 @@ futex(int *uaddr, int futex_op, int val,
> become 1, and then set the value to 0. */
>
> static void
> -fwait(int *futexp)
> +fwait(uint32_t *futexp)
> {
> - int s;
> + long s;
>
> /* atomic_compare_exchange_strong(ptr, oldval, newval)
> atomically performs the equivalent of:
> @@ -1794,7 +1795,7 @@ fwait(int *futexp)
> while (1) {
>
> /* Is the futex available? */
> - const int one = 1;
> + const uint32_t one = 1;
> if (atomic_compare_exchange_strong(futexp, &one, 0))
> break; /* Yes */
>
> @@ -1811,13 +1812,13 @@ fwait(int *futexp)
> so that if the peer is blocked in fpost(), it can proceed. */
>
> static void
> -fpost(int *futexp)
> +fpost(uint32_t *futexp)
> {
> - int s;
> + long s;
>
> /* atomic_compare_exchange_strong() was described in comments above */
>
> - const int zero = 0;
> + const uint32_t zero = 0;
> if (atomic_compare_exchange_strong(futexp, &zero, 1)) {
> s = futex(futexp, FUTEX_WAKE, 1, NULL, NULL, 0);
> if (s == \-1)
>

Subject: Re: [PATCH 1/2] futex.2: srcfix

On 10/30/20 1:39 PM, Alejandro Colomar wrote:
> Signed-off-by: Alejandro Colomar <[email protected]>

Hi Alex,

I've applied this patch, but would prefer to avoid such
patches in the future. Nothing is actually broken in the
old version, so I tend to regard such patches as unnecessary
chur,.

Thanks,

Michael

> ---
> man2/futex.2 | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/man2/futex.2 b/man2/futex.2
> index f82602c11..837adbd25 100644
> --- a/man2/futex.2
> +++ b/man2/futex.2
> @@ -25,8 +25,8 @@ futex \- fast user-space locking
> .SH SYNOPSIS
> .nf
> .PP
> -.B "#include <linux/futex.h>"
> -.B "#include <sys/time.h>"
> +.B #include <linux/futex.h>
> +.B #include <sys/time.h>
> .PP
> .BI "int futex(int *" uaddr ", int " futex_op ", int " val ,
> .BI " const struct timespec *" timeout , \
>


--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Subject: Re: [PATCH 2/2] futex.2: Use appropriate types

On 10/30/20 1:39 PM, Alejandro Colomar wrote:
> The Linux kernel uses the following:
>
> kernel/futex.c:3778:
> SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
> struct __kernel_timespec __user *, utime, u32 __user *, uaddr2,
> u32, val3)
>
> Since there is no glibc wrapper, use the same types the kernel uses.

Thanks. Patch applied.

Cheers,

Michael

> Signed-off-by: Alejandro Colomar <[email protected]>
> ---
> man2/futex.2 | 27 ++++++++++++++-------------
> 1 file changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/man2/futex.2 b/man2/futex.2
> index 837adbd25..73de71623 100644
> --- a/man2/futex.2
> +++ b/man2/futex.2
> @@ -26,12 +26,13 @@ futex \- fast user-space locking
> .nf
> .PP
> .B #include <linux/futex.h>
> +.B #include <stdint.h>
> .B #include <sys/time.h>
> .PP
> -.BI "int futex(int *" uaddr ", int " futex_op ", int " val ,
> +.BI "long futex(uint32_t *" uaddr ", int " futex_op ", uint32_t " val ,
> .BI " const struct timespec *" timeout , \
> " \fR /* or: \fBuint32_t \fIval2\fP */"
> -.BI " int *" uaddr2 ", int " val3 );
> +.BI " uint32_t *" uaddr2 ", uint32_t " val3 );
> .fi
> .PP
> .IR Note :
> @@ -581,8 +582,8 @@ any of the two supplied futex words:
> .IP
> .in +4n
> .EX
> -int oldval = *(int *) uaddr2;
> -*(int *) uaddr2 = oldval \fIop\fP \fIoparg\fP;
> +uint32_t oldval = *(uint32_t *) uaddr2;
> +*(uint32_t *) uaddr2 = oldval \fIop\fP \fIoparg\fP;
> futex(uaddr, FUTEX_WAKE, val, 0, 0, 0);
> if (oldval \fIcmp\fP \fIcmparg\fP)
> futex(uaddr2, FUTEX_WAKE, val2, 0, 0, 0);
> @@ -1765,11 +1766,11 @@ Child (18535) 4
> #define errExit(msg) do { perror(msg); exit(EXIT_FAILURE); \e
> } while (0)
>
> -static int *futex1, *futex2, *iaddr;
> +static uint32_t *futex1, *futex2, *iaddr;
>
> static int
> -futex(int *uaddr, int futex_op, int val,
> - const struct timespec *timeout, int *uaddr2, int val3)
> +futex(uint32_t *uaddr, int futex_op, uint32_t val,
> + const struct timespec *timeout, uint32_t *uaddr2, uint32_t val3)
> {
> return syscall(SYS_futex, uaddr, futex_op, val,
> timeout, uaddr2, val3);
> @@ -1779,9 +1780,9 @@ futex(int *uaddr, int futex_op, int val,
> become 1, and then set the value to 0. */
>
> static void
> -fwait(int *futexp)
> +fwait(uint32_t *futexp)
> {
> - int s;
> + long s;
>
> /* atomic_compare_exchange_strong(ptr, oldval, newval)
> atomically performs the equivalent of:
> @@ -1794,7 +1795,7 @@ fwait(int *futexp)
> while (1) {
>
> /* Is the futex available? */
> - const int one = 1;
> + const uint32_t one = 1;
> if (atomic_compare_exchange_strong(futexp, &one, 0))
> break; /* Yes */
>
> @@ -1811,13 +1812,13 @@ fwait(int *futexp)
> so that if the peer is blocked in fpost(), it can proceed. */
>
> static void
> -fpost(int *futexp)
> +fpost(uint32_t *futexp)
> {
> - int s;
> + long s;
>
> /* atomic_compare_exchange_strong() was described in comments above */
>
> - const int zero = 0;
> + const uint32_t zero = 0;
> if (atomic_compare_exchange_strong(futexp, &zero, 1)) {
> s = futex(futexp, FUTEX_WAKE, 1, NULL, NULL, 0);
> if (s == \-1)
>


--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/

Subject: Re: [PATCH 2/2] futex.2: Use appropriate types

Hi Alex,

On 10/30/20 2:46 PM, Alejandro Colomar wrote:
> BTW, apparently the kernel doesn't use 'const' for 'utime'
> ('timeout' in the manual page),
> but effectively, it doesn't modify it, AFAICS.
>
> Should the kernel use 'const'?
> Is there a reason for the kernel not using 'const'?
> Should we do anything about it in the manual page?

I'm not sure about the kernel, but I think we don't need to
worry in the manual page.

Thanks,

Michael

> On 2020-10-30 13:39, Alejandro Colomar wrote:
>> The Linux kernel uses the following:
>>
>> kernel/futex.c:3778:
>> SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
>> struct __kernel_timespec __user *, utime, u32 __user *, uaddr2,
>> u32, val3)
>>
>> Since there is no glibc wrapper, use the same types the kernel uses.
>>
>> Signed-off-by: Alejandro Colomar <[email protected]>
>> ---
>> man2/futex.2 | 27 ++++++++++++++-------------
>> 1 file changed, 14 insertions(+), 13 deletions(-)
>>
>> diff --git a/man2/futex.2 b/man2/futex.2
>> index 837adbd25..73de71623 100644
>> --- a/man2/futex.2
>> +++ b/man2/futex.2
>> @@ -26,12 +26,13 @@ futex \- fast user-space locking
>> .nf
>> .PP
>> .B #include <linux/futex.h>
>> +.B #include <stdint.h>
>> .B #include <sys/time.h>
>> .PP
>> -.BI "int futex(int *" uaddr ", int " futex_op ", int " val ,
>> +.BI "long futex(uint32_t *" uaddr ", int " futex_op ", uint32_t " val ,
>> .BI " const struct timespec *" timeout , \
>> " \fR /* or: \fBuint32_t \fIval2\fP */"
>> -.BI " int *" uaddr2 ", int " val3 );
>> +.BI " uint32_t *" uaddr2 ", uint32_t " val3 );
>> .fi
>> .PP
>> .IR Note :
>> @@ -581,8 +582,8 @@ any of the two supplied futex words:
>> .IP
>> .in +4n
>> .EX
>> -int oldval = *(int *) uaddr2;
>> -*(int *) uaddr2 = oldval \fIop\fP \fIoparg\fP;
>> +uint32_t oldval = *(uint32_t *) uaddr2;
>> +*(uint32_t *) uaddr2 = oldval \fIop\fP \fIoparg\fP;
>> futex(uaddr, FUTEX_WAKE, val, 0, 0, 0);
>> if (oldval \fIcmp\fP \fIcmparg\fP)
>> futex(uaddr2, FUTEX_WAKE, val2, 0, 0, 0);
>> @@ -1765,11 +1766,11 @@ Child (18535) 4
>> #define errExit(msg) do { perror(msg); exit(EXIT_FAILURE); \e
>> } while (0)
>>
>> -static int *futex1, *futex2, *iaddr;
>> +static uint32_t *futex1, *futex2, *iaddr;
>>
>> static int
>> -futex(int *uaddr, int futex_op, int val,
>> - const struct timespec *timeout, int *uaddr2, int val3)
>> +futex(uint32_t *uaddr, int futex_op, uint32_t val,
>> + const struct timespec *timeout, uint32_t *uaddr2, uint32_t val3)
>> {
>> return syscall(SYS_futex, uaddr, futex_op, val,
>> timeout, uaddr2, val3);
>> @@ -1779,9 +1780,9 @@ futex(int *uaddr, int futex_op, int val,
>> become 1, and then set the value to 0. */
>>
>> static void
>> -fwait(int *futexp)
>> +fwait(uint32_t *futexp)
>> {
>> - int s;
>> + long s;
>>
>> /* atomic_compare_exchange_strong(ptr, oldval, newval)
>> atomically performs the equivalent of:
>> @@ -1794,7 +1795,7 @@ fwait(int *futexp)
>> while (1) {
>>
>> /* Is the futex available? */
>> - const int one = 1;
>> + const uint32_t one = 1;
>> if (atomic_compare_exchange_strong(futexp, &one, 0))
>> break; /* Yes */
>>
>> @@ -1811,13 +1812,13 @@ fwait(int *futexp)
>> so that if the peer is blocked in fpost(), it can proceed. */
>>
>> static void
>> -fpost(int *futexp)
>> +fpost(uint32_t *futexp)
>> {
>> - int s;
>> + long s;
>>
>> /* atomic_compare_exchange_strong() was described in comments above */
>>
>> - const int zero = 0;
>> + const uint32_t zero = 0;
>> if (atomic_compare_exchange_strong(futexp, &zero, 1)) {
>> s = futex(futexp, FUTEX_WAKE, 1, NULL, NULL, 0);
>> if (s == \-1)
>>


--
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/