Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[John Paul Adrian Glaubitz]

Hi Jens!

On 11/5/20 5:17 PM, Jens Axboe wrote:
> Gentle nudge on this one.

I can build- and boot-test on SH and IA64.

I assume Rich will be fine with the SH changes, not sure about the IA64 and Tony.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 11/5/20 9:20 AM, John Paul Adrian Glaubitz wrote:
> Hi Jens!
> 
> On 11/5/20 5:17 PM, Jens Axboe wrote:
>> Gentle nudge on this one.
> 
> I can build- and boot-test on SH and IA64.

That'd be great, thanks!

> I assume Rich will be fine with the SH changes, not sure about the IA64 and Tony.

Let's add Tony - maybe he'll have a chance to take a look at the ia64 change.

-- 
Jens Axboe

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[John Paul Adrian Glaubitz]

Hi Jens!

On 11/5/20 6:15 PM, Jens Axboe wrote:
> On 11/5/20 9:20 AM, John Paul Adrian Glaubitz wrote:
>> Hi Jens!
>>
>> On 11/5/20 5:17 PM, Jens Axboe wrote:
>>> Gentle nudge on this one.
>>
>> I can build- and boot-test on SH and IA64.
> 
> That'd be great, thanks!

Sorry for the delay. I'm busy at the moment and my SH board is currently
building the Perl 5.32 package for Debian. Will try to test your patches
by tomorrow, also ia64.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 11/9/20 3:59 AM, John Paul Adrian Glaubitz wrote:
> Hi Jens!
> 
> On 11/5/20 6:15 PM, Jens Axboe wrote:
>> On 11/5/20 9:20 AM, John Paul Adrian Glaubitz wrote:
>>> Hi Jens!
>>>
>>> On 11/5/20 5:17 PM, Jens Axboe wrote:
>>>> Gentle nudge on this one.
>>>
>>> I can build- and boot-test on SH and IA64.
>>
>> That'd be great, thanks!
> 
> Sorry for the delay. I'm busy at the moment and my SH board is currently
> building the Perl 5.32 package for Debian. Will try to test your patches
> by tomorrow, also ia64.

Thanks, both would be appreciated! Just CC'ed you on the updated patch
for sh.

-- 
Jens Axboe

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[John Paul Adrian Glaubitz]

Hi Jens!

On 11/9/20 3:14 PM, Jens Axboe wrote:
>> Sorry for the delay. I'm busy at the moment and my SH board is currently
>> building the Perl 5.32 package for Debian. Will try to test your patches
>> by tomorrow, also ia64.
> 
> Thanks, both would be appreciated! Just CC'ed you on the updated patch
> for sh.

Is this still relevant for testing? I'm ready to test now, much later than
I thought, sorry.

I'm going to build Linus' latest kernel for my SH and IA64 machines now
and then I can test additional patches on top of it.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 11/16/20 10:26 PM, John Paul Adrian Glaubitz wrote:
> Hi Jens!
> 
> On 11/9/20 3:14 PM, Jens Axboe wrote:
>>> Sorry for the delay. I'm busy at the moment and my SH board is currently
>>> building the Perl 5.32 package for Debian. Will try to test your patches
>>> by tomorrow, also ia64.
>>
>> Thanks, both would be appreciated! Just CC'ed you on the updated patch
>> for sh.
> 
> Is this still relevant for testing? I'm ready to test now, much later than
> I thought, sorry.
> 
> I'm going to build Linus' latest kernel for my SH and IA64 machines now
> and then I can test additional patches on top of it.

Thanks, would definitely still appreciate testing. You can just run
linux-next if you want, it's got everything in there. Or apply the
separate patches to -git, either approach is fine.

-- 
Jens Axboe

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[John Paul Adrian Glaubitz]

Hi Jens!

On 11/17/20 4:06 PM, Jens Axboe wrote:
> On 11/16/20 10:26 PM, John Paul Adrian Glaubitz wrote:
>> Hi Jens!
>>
>> On 11/9/20 3:14 PM, Jens Axboe wrote:
>>>> Sorry for the delay. I'm busy at the moment and my SH board is currently
>>>> building the Perl 5.32 package for Debian. Will try to test your patches
>>>> by tomorrow, also ia64.
>>>
>>> Thanks, both would be appreciated! Just CC'ed you on the updated patch
>>> for sh.
>>
>> Is this still relevant for testing? I'm ready to test now, much later than
>> I thought, sorry.
>>
>> I'm going to build Linus' latest kernel for my SH and IA64 machines now
>> and then I can test additional patches on top of it.
> 
> Thanks, would definitely still appreciate testing. You can just run
> linux-next if you want, it's got everything in there. Or apply the
> separate patches to -git, either approach is fine.

Apologies for the late reply.

I just pulled Linus' latest tree today with your patch for SH included and
I'm not seeing any regressions.

Is there away to test the change itself?

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

[-- Attachment #1: Type: text/plain, Size: 1584 bytes --]

On 1/1/21 7:06 AM, John Paul Adrian Glaubitz wrote:
> Hi Jens!
> 
> On 11/17/20 4:06 PM, Jens Axboe wrote:
>> On 11/16/20 10:26 PM, John Paul Adrian Glaubitz wrote:
>>> Hi Jens!
>>>
>>> On 11/9/20 3:14 PM, Jens Axboe wrote:
>>>>> Sorry for the delay. I'm busy at the moment and my SH board is currently
>>>>> building the Perl 5.32 package for Debian. Will try to test your patches
>>>>> by tomorrow, also ia64.
>>>>
>>>> Thanks, both would be appreciated! Just CC'ed you on the updated patch
>>>> for sh.
>>>
>>> Is this still relevant for testing? I'm ready to test now, much later than
>>> I thought, sorry.
>>>
>>> I'm going to build Linus' latest kernel for my SH and IA64 machines now
>>> and then I can test additional patches on top of it.
>>
>> Thanks, would definitely still appreciate testing. You can just run
>> linux-next if you want, it's got everything in there. Or apply the
>> separate patches to -git, either approach is fine.
> 
> Apologies for the late reply.
> 
> I just pulled Linus' latest tree today with your patch for SH included and
> I'm not seeing any regressions.
> 
> Is there away to test the change itself?

The only user of TWA_SIGNAL, which uses TIF_NOTIFY_SIGNAL, so far is io_uring.
You need something that triggers deferred task_work processing, which is
basically anything that ends up being poll driven for data/space readiness.
Here's a small test app from the liburing test suite, that'll trigger it.

If you install liburing, compile with:

gcc -Wall -O2 -o socket-rw socket-rw.c -luring

and run it without any arguments.

-- 
Jens Axboe


[-- Attachment #2: socket-rw.c --]
[-- Type: text/x-csrc, Size: 2839 bytes --]

/* SPDX-License-Identifier: MIT */
/*
 * Check that a readv on a socket queued before a writev doesn't hang
 * the processing.
 *
 * From Hrvoje Zeba <zeba.hrvoje@gmail.com>
 */
#include <stdio.h>
#include <stdlib.h>
#include <stdint.h>
#include <assert.h>

#include <errno.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/socket.h>
#include <sys/un.h>
#include <netinet/tcp.h>
#include <netinet/in.h>

#include <liburing.h>

int main(int argc, char *argv[])
{
	int p_fd[2], ret;
	int32_t recv_s0;
	int32_t val = 1;
	struct sockaddr_in addr;

	if (argc > 1)
		return 0;

	recv_s0 = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);

	ret = setsockopt(recv_s0, SOL_SOCKET, SO_REUSEPORT, &val, sizeof(val));
	assert(ret != -1);
	ret = setsockopt(recv_s0, SOL_SOCKET, SO_REUSEADDR, &val, sizeof(val));
	assert(ret != -1);

	addr.sin_family = AF_INET;
	addr.sin_port = 0x1235;
	addr.sin_addr.s_addr = 0x0100007fU;

	ret = bind(recv_s0, (struct sockaddr*)&addr, sizeof(addr));
	assert(ret != -1);
	ret = listen(recv_s0, 128);
	assert(ret != -1);


	p_fd[1] = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP);

	val = 1;
	ret = setsockopt(p_fd[1], IPPROTO_TCP, TCP_NODELAY, &val, sizeof(val));
	assert(ret != -1);

	int32_t flags = fcntl(p_fd[1], F_GETFL, 0);
	assert(flags != -1);

	flags |= O_NONBLOCK;
	ret = fcntl(p_fd[1], F_SETFL, flags);
	assert(ret != -1);

	ret = connect(p_fd[1], (struct sockaddr*)&addr, sizeof(addr));
	assert(ret == -1);

	flags = fcntl(p_fd[1], F_GETFL, 0);
	assert(flags != -1);

	flags &= ~O_NONBLOCK;
	ret = fcntl(p_fd[1], F_SETFL, flags);
	assert(ret != -1);

	p_fd[0] = accept(recv_s0, NULL, NULL);
	assert(p_fd[0] != -1);

	while (1) {
		int32_t code;
		socklen_t code_len = sizeof(code);

		ret = getsockopt(p_fd[1], SOL_SOCKET, SO_ERROR, &code, &code_len);
		assert(ret != -1);

		if (!code)
			break;
	}

	struct io_uring m_io_uring;

	ret = io_uring_queue_init(32, &m_io_uring, 0);
	assert(ret >= 0);

	char recv_buff[128];
	char send_buff[128];

	{
		struct iovec iov[1];

		iov[0].iov_base = recv_buff;
		iov[0].iov_len = sizeof(recv_buff);

		struct io_uring_sqe* sqe = io_uring_get_sqe(&m_io_uring);
		assert(sqe != NULL);

		io_uring_prep_readv(sqe, p_fd[0], iov, 1, 0);
	}

	{
		struct iovec iov[1];

		iov[0].iov_base = send_buff;
		iov[0].iov_len = sizeof(send_buff);

		struct io_uring_sqe* sqe = io_uring_get_sqe(&m_io_uring);
		assert(sqe != NULL);

		io_uring_prep_writev(sqe, p_fd[1], iov, 1, 0);
	}

	ret = io_uring_submit_and_wait(&m_io_uring, 2);
	assert(ret != -1);

	struct io_uring_cqe* cqe;
	uint32_t head;
	uint32_t count = 0;

	while (count != 2) {
		io_uring_for_each_cqe(&m_io_uring, head, cqe) {
			assert(cqe->res == 128);
			count++;
		}

		assert(count <= 2);
		io_uring_cq_advance(&m_io_uring, count);
	}

	io_uring_queue_exit(&m_io_uring);
	return 0;
}

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[John Paul Adrian Glaubitz]

Hi Jens!

On 1/1/21 4:08 PM, Jens Axboe wrote:
> On 1/1/21 7:06 AM, John Paul Adrian Glaubitz wrote:
>> Is there away to test the change itself?
> 
> The only user of TWA_SIGNAL, which uses TIF_NOTIFY_SIGNAL, so far is io_uring.
> You need something that triggers deferred task_work processing, which is
> basically anything that ends up being poll driven for data/space readiness.
> Here's a small test app from the liburing test suite, that'll trigger it.
> 
> If you install liburing, compile with:
> 
> gcc -Wall -O2 -o socket-rw socket-rw.c -luring
> 
> and run it without any arguments.

How long is this test supposed to run? It's already been running for some minutes
on my 600 MHz machine.

Adrian

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 1/1/21 8:30 AM, John Paul Adrian Glaubitz wrote:
> Hi Jens!
> 
> On 1/1/21 4:08 PM, Jens Axboe wrote:
>> On 1/1/21 7:06 AM, John Paul Adrian Glaubitz wrote:
>>> Is there away to test the change itself?
>>
>> The only user of TWA_SIGNAL, which uses TIF_NOTIFY_SIGNAL, so far is io_uring.
>> You need something that triggers deferred task_work processing, which is
>> basically anything that ends up being poll driven for data/space readiness.
>> Here's a small test app from the liburing test suite, that'll trigger it.
>>
>> If you install liburing, compile with:
>>
>> gcc -Wall -O2 -o socket-rw socket-rw.c -luring
>>
>> and run it without any arguments.
> 
> How long is this test supposed to run? It's already been running for some minutes
> on my 600 MHz machine.

It's supposed to finish very quickly:

axboe@p1 ~> time ./socket-rw                                                             0.000s

________________________________________________________
Executed in    1.10 millis    fish           external 
   usr time  888.00 micros  278.00 micros  610.00 micros 
   sys time   35.00 micros   35.00 micros    0.00 micros 

If it doesn't, can you try:

# echo 1 > /sys/kernel/debug/tracing/events/io_uring/enable

Then run the socket-rw app, and then do:

# cat /sys/kernel/debug/tracing/trace

and send that output? Might also be useful to include the strace
of the socket-rw just in case, so maybe run it ala

strace -o foo ./socket-rw

and include foo in the reply as well.

-- 
Jens Axboe

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[John Paul Adrian Glaubitz]

On 1/1/21 4:35 PM, Jens Axboe wrote:> It's supposed to finish very quickly:
> 
> axboe@p1 ~> time ./socket-rw                                                             0.000s
> 
> ________________________________________________________
> Executed in    1.10 millis    fish           external 
>    usr time  888.00 micros  278.00 micros  610.00 micros 
>    sys time   35.00 micros   35.00 micros    0.00 micros 
> 
> If it doesn't, can you try:
> 
> # echo 1 > /sys/kernel/debug/tracing/events/io_uring/enable
> 
> Then run the socket-rw app, and then do:
> 
> # cat /sys/kernel/debug/tracing/trace
> 
> and send that output? Might also be useful to include the strace
> of the socket-rw just in case, so maybe run it ala
> 
> strace -o foo ./socket-rw
> 
> and include foo in the reply as well.

Odd, I just ran it through strace and it exited immediately:

root@tirpitz:~> strace ./socket-rw 
execve("./socket-rw", ["./socket-rw"], 0x7bb26670 /* 25 vars */) = 0
brk(NULL)                               = 0x412000
uname({sysname="Linux", nodename="tirpitz", ...}) = 0
access("/etc/ld.so.preload", R_OK)      = -1 ENOENT (No such file or directory)
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = 3
fstat64(3, {st_mode=S_IFREG|0644, st_sized232, ...}) = 0
mmap2(NULL, 64232, PROT_READ, MAP_PRIVATE, 3, 0) = 0x29584000
close(3)                                = 0
mmap2(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x29574000
openat(AT_FDCWD, "/usr/lib/sh4-linux-gnu/liburing.so.1", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = 3
read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0*\0\1\0\0\0\324\17\0\0004\0\0\0"..., 512) = 512
fstat64(3, {st_mode=S_IFREG|0644, st_size•36, ...}) = 0
mmap2(NULL, 73852, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x29594000
mprotect(0x29596000, 61440, PROT_NONE)  = 0
mmap2(0x295a5000, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x1000) = 0x295a5000
close(3)                                = 0
openat(AT_FDCWD, "/lib/sh4-linux-gnu/libc.so.6", O_RDONLY|O_LARGEFILE|O_CLOEXEC) = 3
read(3, "\177ELF\1\1\1\0\0\0\0\0\0\0\0\0\3\0*\0\1\0\0\0\224\306\1\0004\0\0\0"..., 512) = 512
pread64(3, "\4\0\0\0\20\0\0\0\1\0\0\0GNU\0\0\0\0\0\3\0\0\0\2\0\0\0\0\0\0\0", 32, 1290836) = 32
fstat64(3, {st_mode=S_IFREG|0755, st_size\x1297036, ...}) = 0
mmap2(NULL, 1366256, PROT_READ|PROT_EXEC, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x295a8000
mprotect(0x296e1000, 61440, PROT_NONE)  = 0
mmap2(0x296f0000, 16384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_DENYWRITE, 3, 0x138000) = 0x296f0000
mmap2(0x296f4000, 6384, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_FIXED|MAP_ANONYMOUS, -1, 0) = 0x296f4000
close(3)                                = 0
mprotect(0x296f0000, 8192, PROT_READ)   = 0
mprotect(0x295a5000, 4096, PROT_READ)   = 0
mprotect(0x410000, 4096, PROT_READ)     = 0
mprotect(0x29582000, 4096, PROT_READ)   = 0
munmap(0x29584000, 64232)               = 0
socket(AF_INET, SOCK_STREAM|SOCK_CLOEXEC, IPPROTO_TCP) = 3
setsockopt(3, SOL_SOCKET, SO_REUSEPORT, [1], 4) = 0
setsockopt(3, SOL_SOCKET, SO_REUSEADDR, [1], 4) = 0
bind(3, {sa_family¯_INET, sin_port=htons(13586), sin_addr=inet_addr("127.0.0.1")}, 16) = 0
listen(3, 128)                          = 0
socket(AF_INET, SOCK_STREAM|SOCK_CLOEXEC, IPPROTO_TCP) = 4
setsockopt(4, SOL_TCP, TCP_NODELAY, [1], 4) = 0
fcntl64(4, F_GETFL)                     = 0x2 (flags O_RDWR)
fcntl64(4, F_SETFL, O_RDWR|O_NONBLOCK)  = 0
connect(4, {sa_family¯_INET, sin_port=htons(13586), sin_addr=inet_addr("127.0.0.1")}, 16) = -1 EINPROGRESS (Operation now in progress)
fcntl64(4, F_GETFL)                     = 0x802 (flags O_RDWR|O_NONBLOCK)
fcntl64(4, F_SETFL, O_RDWR)             = 0
accept(3, NULL, NULL)                   = 5
getsockopt(4, SOL_SOCKET, SO_ERROR, [0], [4]) = 0
io_uring_setup(32, {flags=0, sq_thread_cpu=0, sq_thread_idle=0, sq_entries2, cq_entriesd, features=IORING_FEAT_SINGLE_MMAP|IORING_FEAT_NODROP|IORING_FEAT_SUBMIT_STABLE|IORING_FEAT_RW_CUR_POS|IORING_FEAT_CUR_PERSONALITY|0x1e0, sq_off={head=0, tail=4, ring_mask\x16, ring_entries$, flags6, dropped2, array\x1072}, cq_off={head=8, tail\x12, ring_mask , ring_entries(, overflowD, cqesH, resv=[0x28, 0]}}) = 6
mmap2(NULL, 1200, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_POPULATE, 6, 0) = 0x29576000
mmap2(NULL, 2048, PROT_READ|PROT_WRITE, MAP_SHARED|MAP_POPULATE, 6, 0x10000000) = 0x29578000
io_uring_enter(6, 2, 2, IORING_ENTER_GETEVENTS, NULL, 8) = 2
munmap(0x29578000, 2048)                = 0
munmap(0x29576000, 1200)                = 0
close(6)                                = 0
exit_group(0)                           = ?
+++ exited with 0 +++
root@tirpitz:~>

-- 
 .''`.  John Paul Adrian Glaubitz
: :' :  Debian Developer - glaubitz@debian.org
`. `'   Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de
  `-    GPG: 62FF 8A75 84E0 2956 9546  0006 7426 3B37 F5B5 F913

Re: [PATCH] sh: add support for TIF_NOTIFY_SIGNAL

[Jens Axboe]

On 1/1/21 11:16 AM, John Paul Adrian Glaubitz wrote:
> On 1/1/21 4:35 PM, Jens Axboe wrote:> It's supposed to finish very quickly:
>>
>> axboe@p1 ~> time ./socket-rw                                                             0.000s
>>
>> ________________________________________________________
>> Executed in    1.10 millis    fish           external 
>>    usr time  888.00 micros  278.00 micros  610.00 micros 
>>    sys time   35.00 micros   35.00 micros    0.00 micros 
>>
>> If it doesn't, can you try:
>>
>> # echo 1 > /sys/kernel/debug/tracing/events/io_uring/enable
>>
>> Then run the socket-rw app, and then do:
>>
>> # cat /sys/kernel/debug/tracing/trace
>>
>> and send that output? Might also be useful to include the strace
>> of the socket-rw just in case, so maybe run it ala
>>
>> strace -o foo ./socket-rw
>>
>> and include foo in the reply as well.
> 
> Odd, I just ran it through strace and it exited immediately:

That looks correct, it sets up a ring, issues and waits for two
requests. Both of those requests require TWA_SIGNAL processing.

-- 
Jens Axboe