* [PATCH v3 11/30] ntsync: Introduce NTSYNC_IOC_EVENT_RESET.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
This corresponds to the NT syscall NtResetEvent().
This sets the event to the unsignaled state, and returns its previous state.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
drivers/misc/ntsync.c | 22 ++++++++++++++++++++++
include/uapi/linux/ntsync.h | 1 +
2 files changed, 23 insertions(+)
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 69f359241cf6..ae78425c87d1 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -508,6 +508,26 @@ static int ntsync_event_set(struct ntsync_obj *event, void __user *argp)
return 0;
}
+static int ntsync_event_reset(struct ntsync_obj *event, void __user *argp)
+{
+ __u32 prev_state;
+
+ if (event->type != NTSYNC_TYPE_EVENT)
+ return -EINVAL;
+
+ spin_lock(&event->lock);
+
+ prev_state = event->u.event.signaled;
+ event->u.event.signaled = false;
+
+ spin_unlock(&event->lock);
+
+ if (put_user(prev_state, (__u32 __user *)argp))
+ return -EFAULT;
+
+ return 0;
+}
+
static int ntsync_obj_release(struct inode *inode, struct file *file)
{
struct ntsync_obj *obj = file->private_data;
@@ -533,6 +553,8 @@ static long ntsync_obj_ioctl(struct file *file, unsigned int cmd,
return ntsync_mutex_kill(obj, argp);
case NTSYNC_IOC_EVENT_SET:
return ntsync_event_set(obj, argp);
+ case NTSYNC_IOC_EVENT_RESET:
+ return ntsync_event_reset(obj, argp);
default:
return -ENOIOCTLCMD;
}
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index 65329d15a472..657542107328 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -53,5 +53,6 @@ struct ntsync_wait_args {
#define NTSYNC_IOC_MUTEX_UNLOCK _IOWR('N', 0x85, struct ntsync_mutex_args)
#define NTSYNC_IOC_MUTEX_KILL _IOW ('N', 0x86, __u32)
#define NTSYNC_IOC_EVENT_SET _IOR ('N', 0x88, __u32)
+#define NTSYNC_IOC_EVENT_RESET _IOR ('N', 0x89, __u32)
#endif
--
2.43.0
^ permalink raw reply related
* [PATCH v3 07/30] ntsync: Introduce NTSYNC_IOC_MUTEX_UNLOCK.
From: Elizabeth Figura @ 2024-03-29 0:05 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
This corresponds to the NT syscall NtReleaseMutant().
This syscall decrements the mutex's recursion count by one, and returns the
previous value. If the mutex is not owned by the given owner ID, the function
instead fails and returns -EPERM.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
drivers/misc/ntsync.c | 64 +++++++++++++++++++++++++++++++++++++
include/uapi/linux/ntsync.h | 1 +
2 files changed, 65 insertions(+)
diff --git a/drivers/misc/ntsync.c b/drivers/misc/ntsync.c
index 173513aeeacc..f7911ef78d5b 100644
--- a/drivers/misc/ntsync.c
+++ b/drivers/misc/ntsync.c
@@ -315,6 +315,68 @@ static int ntsync_sem_post(struct ntsync_obj *sem, void __user *argp)
return ret;
}
+/*
+ * Actually change the mutex state, returning -EPERM if not the owner.
+ */
+static int unlock_mutex_state(struct ntsync_obj *mutex,
+ const struct ntsync_mutex_args *args)
+{
+ lockdep_assert_held(&mutex->lock);
+
+ if (mutex->u.mutex.owner != args->owner)
+ return -EPERM;
+
+ if (!--mutex->u.mutex.count)
+ mutex->u.mutex.owner = 0;
+ return 0;
+}
+
+static int ntsync_mutex_unlock(struct ntsync_obj *mutex, void __user *argp)
+{
+ struct ntsync_mutex_args __user *user_args = argp;
+ struct ntsync_device *dev = mutex->dev;
+ struct ntsync_mutex_args args;
+ __u32 prev_count;
+ int ret;
+
+ if (copy_from_user(&args, argp, sizeof(args)))
+ return -EFAULT;
+ if (!args.owner)
+ return -EINVAL;
+
+ if (mutex->type != NTSYNC_TYPE_MUTEX)
+ return -EINVAL;
+
+ if (atomic_read(&mutex->all_hint) > 0) {
+ spin_lock(&dev->wait_all_lock);
+ spin_lock_nest_lock(&mutex->lock, &dev->wait_all_lock);
+
+ prev_count = mutex->u.mutex.count;
+ ret = unlock_mutex_state(mutex, &args);
+ if (!ret) {
+ try_wake_all_obj(dev, mutex);
+ try_wake_any_mutex(mutex);
+ }
+
+ spin_unlock(&mutex->lock);
+ spin_unlock(&dev->wait_all_lock);
+ } else {
+ spin_lock(&mutex->lock);
+
+ prev_count = mutex->u.mutex.count;
+ ret = unlock_mutex_state(mutex, &args);
+ if (!ret)
+ try_wake_any_mutex(mutex);
+
+ spin_unlock(&mutex->lock);
+ }
+
+ if (!ret && put_user(prev_count, &user_args->count))
+ ret = -EFAULT;
+
+ return ret;
+}
+
static int ntsync_obj_release(struct inode *inode, struct file *file)
{
struct ntsync_obj *obj = file->private_data;
@@ -334,6 +396,8 @@ static long ntsync_obj_ioctl(struct file *file, unsigned int cmd,
switch (cmd) {
case NTSYNC_IOC_SEM_POST:
return ntsync_sem_post(obj, argp);
+ case NTSYNC_IOC_MUTEX_UNLOCK:
+ return ntsync_mutex_unlock(obj, argp);
default:
return -ENOIOCTLCMD;
}
diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
index cd7841cdba49..fa2c9f638d77 100644
--- a/include/uapi/linux/ntsync.h
+++ b/include/uapi/linux/ntsync.h
@@ -43,5 +43,6 @@ struct ntsync_wait_args {
#define NTSYNC_IOC_CREATE_MUTEX _IOWR('N', 0x84, struct ntsync_sem_args)
#define NTSYNC_IOC_SEM_POST _IOWR('N', 0x81, __u32)
+#define NTSYNC_IOC_MUTEX_UNLOCK _IOWR('N', 0x85, struct ntsync_mutex_args)
#endif
--
2.43.0
^ permalink raw reply related
* [PATCH v3 25/30] selftests: ntsync: Add some tests for wakeup signaling with events.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Expand the contended wait tests, which previously only covered events and
semaphores, to cover events as well.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 151 +++++++++++++++++-
1 file changed, 147 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 12ccb4ec28e4..5d17eff6a370 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -622,6 +622,7 @@ TEST(test_wait_any)
TEST(test_wait_all)
{
+ struct ntsync_event_args event_args = {0};
struct ntsync_mutex_args mutex_args = {0};
struct ntsync_sem_args sem_args = {0};
__u32 owner, index, count;
@@ -644,6 +645,11 @@ TEST(test_wait_all)
EXPECT_EQ(0, ret);
EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+ event_args.manual = true;
+ event_args.signaled = true;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+
objs[0] = sem_args.sem;
objs[1] = mutex_args.mutex;
@@ -692,6 +698,14 @@ TEST(test_wait_all)
check_sem_state(sem_args.sem, 1, 3);
check_mutex_state(mutex_args.mutex, 1, 123);
+ objs[0] = sem_args.sem;
+ objs[1] = event_args.event;
+ ret = wait_all(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_event_state(event_args.event, 1, 1);
+
/* test waiting on the same object twice */
objs[0] = objs[1] = sem_args.sem;
ret = wait_all(fd, 2, objs, 123, &index);
@@ -700,6 +714,7 @@ TEST(test_wait_all)
close(sem_args.sem);
close(mutex_args.mutex);
+ close(event_args.event);
close(fd);
}
@@ -746,12 +761,13 @@ static int wait_for_thread(pthread_t thread, unsigned int ms)
TEST(wake_any)
{
+ struct ntsync_event_args event_args = {0};
struct ntsync_mutex_args mutex_args = {0};
struct ntsync_wait_args wait_args = {0};
struct ntsync_sem_args sem_args = {0};
struct wait_args thread_args;
+ __u32 count, index, signaled;
int objs[2], fd, ret;
- __u32 count, index;
pthread_t thread;
fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
@@ -833,10 +849,101 @@ TEST(wake_any)
EXPECT_EQ(0, thread_args.ret);
EXPECT_EQ(1, wait_args.index);
+ /* test waking events */
+
+ event_args.manual = false;
+ event_args.signaled = false;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+
+ objs[1] = event_args.event;
+ wait_args.timeout = get_abs_timeout(1000);
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event_args.event, 0, 0);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(1, wait_args.index);
+
+ wait_args.timeout = get_abs_timeout(1000);
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event_args.event, 0, 0);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(1, wait_args.index);
+
+ close(event_args.event);
+
+ event_args.manual = true;
+ event_args.signaled = false;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+
+ objs[1] = event_args.event;
+ wait_args.timeout = get_abs_timeout(1000);
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event_args.event, 1, 1);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(1, wait_args.index);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+
+ wait_args.timeout = get_abs_timeout(1000);
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event_args.event, 0, 1);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(1, wait_args.index);
+
+ close(event_args.event);
+
/* delete an object while it's being waited on */
wait_args.timeout = get_abs_timeout(200);
wait_args.owner = 123;
+ objs[1] = mutex_args.mutex;
ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
EXPECT_EQ(0, ret);
@@ -856,12 +963,14 @@ TEST(wake_any)
TEST(wake_all)
{
+ struct ntsync_event_args manual_event_args = {0};
+ struct ntsync_event_args auto_event_args = {0};
struct ntsync_mutex_args mutex_args = {0};
struct ntsync_wait_args wait_args = {0};
struct ntsync_sem_args sem_args = {0};
struct wait_args thread_args;
- int objs[2], fd, ret;
- __u32 count, index;
+ __u32 count, index, signaled;
+ int objs[4], fd, ret;
pthread_t thread;
fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
@@ -881,12 +990,24 @@ TEST(wake_all)
EXPECT_EQ(0, ret);
EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+ manual_event_args.manual = true;
+ manual_event_args.signaled = true;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &manual_event_args);
+ EXPECT_EQ(0, ret);
+
+ auto_event_args.manual = false;
+ auto_event_args.signaled = true;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &auto_event_args);
+ EXPECT_EQ(0, ret);
+
objs[0] = sem_args.sem;
objs[1] = mutex_args.mutex;
+ objs[2] = manual_event_args.event;
+ objs[3] = auto_event_args.event;
wait_args.timeout = get_abs_timeout(1000);
wait_args.objs = (uintptr_t)objs;
- wait_args.count = 2;
+ wait_args.count = 4;
wait_args.owner = 456;
thread_args.fd = fd;
thread_args.args = &wait_args;
@@ -920,12 +1041,32 @@ TEST(wake_all)
check_mutex_state(mutex_args.mutex, 0, 0);
+ ret = ioctl(manual_event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+
count = 2;
ret = post_sem(sem_args.sem, &count);
EXPECT_EQ(0, ret);
EXPECT_EQ(0, count);
+ check_sem_state(sem_args.sem, 2, 3);
+
+ ret = ioctl(auto_event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+
+ ret = ioctl(manual_event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+
+ ret = ioctl(auto_event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+
check_sem_state(sem_args.sem, 1, 3);
check_mutex_state(mutex_args.mutex, 1, 456);
+ check_event_state(manual_event_args.event, 1, 1);
+ check_event_state(auto_event_args.event, 0, 0);
ret = wait_for_thread(thread, 100);
EXPECT_EQ(0, ret);
@@ -943,6 +1084,8 @@ TEST(wake_all)
close(sem_args.sem);
close(mutex_args.mutex);
+ close(manual_event_args.event);
+ close(auto_event_args.event);
ret = wait_for_thread(thread, 200);
EXPECT_EQ(0, ret);
--
2.43.0
^ permalink raw reply related
* [PATCH v3 24/30] selftests: ntsync: Add some tests for auto-reset event state.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test event-specific ioctls NTSYNC_IOC_EVENT_SET, NTSYNC_IOC_EVENT_RESET,
NTSYNC_IOC_EVENT_PULSE, NTSYNC_IOC_EVENT_READ for auto-reset events, and
waiting on auto-reset events.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 59 +++++++++++++++++++
1 file changed, 59 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index b6481c2b85cc..12ccb4ec28e4 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -442,6 +442,65 @@ TEST(manual_event_state)
close(fd);
}
+TEST(auto_event_state)
+{
+ struct ntsync_event_args event_args;
+ __u32 index, signaled;
+ int fd, event, ret;
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ event_args.manual = 0;
+ event_args.signaled = 1;
+ event_args.event = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, event_args.event);
+ event = event_args.event;
+
+ check_event_state(event, 1, 0);
+
+ signaled = 0xdeadbeef;
+ ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+ check_event_state(event, 1, 0);
+
+ ret = wait_any(fd, 1, &event, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_event_state(event, 0, 0);
+
+ signaled = 0xdeadbeef;
+ ret = ioctl(event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event, 0, 0);
+
+ ret = wait_any(fd, 1, &event, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, signaled);
+ check_event_state(event, 0, 0);
+
+ ret = ioctl(event, NTSYNC_IOC_EVENT_PULSE, &signaled);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, signaled);
+ check_event_state(event, 0, 0);
+
+ close(event);
+
+ close(fd);
+}
+
TEST(test_wait_any)
{
int objs[NTSYNC_MAX_WAIT_COUNT + 1], fd, ret;
--
2.43.0
^ permalink raw reply related
* [PATCH v3 18/30] selftests: ntsync: Add some tests for mutex state.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test mutex-specific ioctls NTSYNC_IOC_MUTEX_UNLOCK and NTSYNC_IOC_MUTEX_READ,
and waiting on mutexes.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 196 ++++++++++++++++++
1 file changed, 196 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 1e145c6dfded..7cd0f40594fd 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -40,6 +40,39 @@ static int post_sem(int sem, __u32 *count)
return ioctl(sem, NTSYNC_IOC_SEM_POST, count);
}
+static int read_mutex_state(int mutex, __u32 *count, __u32 *owner)
+{
+ struct ntsync_mutex_args args;
+ int ret;
+
+ memset(&args, 0xcc, sizeof(args));
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_READ, &args);
+ *count = args.count;
+ *owner = args.owner;
+ return ret;
+}
+
+#define check_mutex_state(mutex, count, owner) \
+ ({ \
+ __u32 __count, __owner; \
+ int ret = read_mutex_state((mutex), &__count, &__owner); \
+ EXPECT_EQ(0, ret); \
+ EXPECT_EQ((count), __count); \
+ EXPECT_EQ((owner), __owner); \
+ })
+
+static int unlock_mutex(int mutex, __u32 owner, __u32 *count)
+{
+ struct ntsync_mutex_args args;
+ int ret;
+
+ args.owner = owner;
+ args.count = 0xdeadbeef;
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_UNLOCK, &args);
+ *count = args.count;
+ return ret;
+}
+
static int wait_any(int fd, __u32 count, const int *objs, __u32 owner, __u32 *index)
{
struct ntsync_wait_args args = {0};
@@ -146,4 +179,167 @@ TEST(semaphore_state)
close(fd);
}
+TEST(mutex_state)
+{
+ struct ntsync_mutex_args mutex_args;
+ __u32 owner, count, index;
+ struct timespec timeout;
+ int fd, ret, mutex;
+
+ clock_gettime(CLOCK_MONOTONIC, &timeout);
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ mutex_args.owner = 123;
+ mutex_args.count = 0;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ mutex_args.owner = 0;
+ mutex_args.count = 2;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ mutex_args.owner = 123;
+ mutex_args.count = 2;
+ mutex_args.mutex = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+ mutex = mutex_args.mutex;
+ check_mutex_state(mutex, 2, 123);
+
+ ret = unlock_mutex(mutex, 0, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ ret = unlock_mutex(mutex, 456, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EPERM, errno);
+ check_mutex_state(mutex, 2, 123);
+
+ ret = unlock_mutex(mutex, 123, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(2, count);
+ check_mutex_state(mutex, 1, 123);
+
+ ret = unlock_mutex(mutex, 123, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, count);
+ check_mutex_state(mutex, 0, 0);
+
+ ret = unlock_mutex(mutex, 123, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EPERM, errno);
+
+ ret = wait_any(fd, 1, &mutex, 456, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_mutex_state(mutex, 1, 456);
+
+ ret = wait_any(fd, 1, &mutex, 456, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_mutex_state(mutex, 2, 456);
+
+ ret = unlock_mutex(mutex, 456, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(2, count);
+ check_mutex_state(mutex, 1, 456);
+
+ ret = wait_any(fd, 1, &mutex, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ owner = 0;
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ owner = 123;
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EPERM, errno);
+ check_mutex_state(mutex, 1, 456);
+
+ owner = 456;
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(0, ret);
+
+ memset(&mutex_args, 0xcc, sizeof(mutex_args));
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_READ, &mutex_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(0, mutex_args.count);
+ EXPECT_EQ(0, mutex_args.owner);
+
+ memset(&mutex_args, 0xcc, sizeof(mutex_args));
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_READ, &mutex_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(0, mutex_args.count);
+ EXPECT_EQ(0, mutex_args.owner);
+
+ ret = wait_any(fd, 1, &mutex, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(0, index);
+ check_mutex_state(mutex, 1, 123);
+
+ owner = 123;
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(0, ret);
+
+ memset(&mutex_args, 0xcc, sizeof(mutex_args));
+ ret = ioctl(mutex, NTSYNC_IOC_MUTEX_READ, &mutex_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(0, mutex_args.count);
+ EXPECT_EQ(0, mutex_args.owner);
+
+ ret = wait_any(fd, 1, &mutex, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(0, index);
+ check_mutex_state(mutex, 1, 123);
+
+ close(mutex);
+
+ mutex_args.owner = 0;
+ mutex_args.count = 0;
+ mutex_args.mutex = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+ mutex = mutex_args.mutex;
+ check_mutex_state(mutex, 0, 0);
+
+ ret = wait_any(fd, 1, &mutex, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_mutex_state(mutex, 1, 123);
+
+ close(mutex);
+
+ mutex_args.owner = 123;
+ mutex_args.count = ~0u;
+ mutex_args.mutex = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+ mutex = mutex_args.mutex;
+ check_mutex_state(mutex, ~0u, 123);
+
+ ret = wait_any(fd, 1, &mutex, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ close(mutex);
+
+ close(fd);
+}
+
TEST_HARNESS_MAIN
--
2.43.0
^ permalink raw reply related
* [PATCH v3 17/30] selftests: ntsync: Add some tests for semaphore state.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Wine has tests for its synchronization primitives, but these are more accessible
to kernel developers, and also allow us to test some edge cases that Wine does
not care about.
This patch adds tests for semaphore-specific ioctls NTSYNC_IOC_SEM_POST and
NTSYNC_IOC_SEM_READ, and waiting on semaphores.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
tools/testing/selftests/Makefile | 1 +
.../testing/selftests/drivers/ntsync/Makefile | 8 +
tools/testing/selftests/drivers/ntsync/config | 1 +
.../testing/selftests/drivers/ntsync/ntsync.c | 149 ++++++++++++++++++
4 files changed, 159 insertions(+)
create mode 100644 tools/testing/selftests/drivers/ntsync/Makefile
create mode 100644 tools/testing/selftests/drivers/ntsync/config
create mode 100644 tools/testing/selftests/drivers/ntsync/ntsync.c
diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
index e1504833654d..6f95206325e1 100644
--- a/tools/testing/selftests/Makefile
+++ b/tools/testing/selftests/Makefile
@@ -16,6 +16,7 @@ TARGETS += damon
TARGETS += devices
TARGETS += dmabuf-heaps
TARGETS += drivers/dma-buf
+TARGETS += drivers/ntsync
TARGETS += drivers/s390x/uvdevice
TARGETS += drivers/net/bonding
TARGETS += drivers/net/team
diff --git a/tools/testing/selftests/drivers/ntsync/Makefile b/tools/testing/selftests/drivers/ntsync/Makefile
new file mode 100644
index 000000000000..a34da5ccacf0
--- /dev/null
+++ b/tools/testing/selftests/drivers/ntsync/Makefile
@@ -0,0 +1,8 @@
+# SPDX-LICENSE-IDENTIFIER: GPL-2.0-only
+TEST_GEN_PROGS := ntsync
+
+top_srcdir =../../../../..
+CFLAGS += -I$(top_srcdir)/usr/include
+LDLIBS += -lpthread
+
+include ../../lib.mk
diff --git a/tools/testing/selftests/drivers/ntsync/config b/tools/testing/selftests/drivers/ntsync/config
new file mode 100644
index 000000000000..60539c826d06
--- /dev/null
+++ b/tools/testing/selftests/drivers/ntsync/config
@@ -0,0 +1 @@
+CONFIG_WINESYNC=y
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
new file mode 100644
index 000000000000..1e145c6dfded
--- /dev/null
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -0,0 +1,149 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Various unit tests for the "ntsync" synchronization primitive driver.
+ *
+ * Copyright (C) 2021-2022 Elizabeth Figura <zfigura@codeweavers.com>
+ */
+
+#define _GNU_SOURCE
+#include <sys/ioctl.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <time.h>
+#include <pthread.h>
+#include <linux/ntsync.h>
+#include "../../kselftest_harness.h"
+
+static int read_sem_state(int sem, __u32 *count, __u32 *max)
+{
+ struct ntsync_sem_args args;
+ int ret;
+
+ memset(&args, 0xcc, sizeof(args));
+ ret = ioctl(sem, NTSYNC_IOC_SEM_READ, &args);
+ *count = args.count;
+ *max = args.max;
+ return ret;
+}
+
+#define check_sem_state(sem, count, max) \
+ ({ \
+ __u32 __count, __max; \
+ int ret = read_sem_state((sem), &__count, &__max); \
+ EXPECT_EQ(0, ret); \
+ EXPECT_EQ((count), __count); \
+ EXPECT_EQ((max), __max); \
+ })
+
+static int post_sem(int sem, __u32 *count)
+{
+ return ioctl(sem, NTSYNC_IOC_SEM_POST, count);
+}
+
+static int wait_any(int fd, __u32 count, const int *objs, __u32 owner, __u32 *index)
+{
+ struct ntsync_wait_args args = {0};
+ struct timespec timeout;
+ int ret;
+
+ clock_gettime(CLOCK_MONOTONIC, &timeout);
+
+ args.timeout = timeout.tv_sec * 1000000000 + timeout.tv_nsec;
+ args.count = count;
+ args.objs = (uintptr_t)objs;
+ args.owner = owner;
+ args.index = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_WAIT_ANY, &args);
+ *index = args.index;
+ return ret;
+}
+
+TEST(semaphore_state)
+{
+ struct ntsync_sem_args sem_args;
+ struct timespec timeout;
+ __u32 count, index;
+ int fd, ret, sem;
+
+ clock_gettime(CLOCK_MONOTONIC, &timeout);
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ sem_args.count = 3;
+ sem_args.max = 2;
+ sem_args.sem = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ sem_args.count = 2;
+ sem_args.max = 2;
+ sem_args.sem = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, sem_args.sem);
+ sem = sem_args.sem;
+ check_sem_state(sem, 2, 2);
+
+ count = 0;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(2, count);
+ check_sem_state(sem, 2, 2);
+
+ count = 1;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOVERFLOW, errno);
+ check_sem_state(sem, 2, 2);
+
+ ret = wait_any(fd, 1, &sem, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem, 1, 2);
+
+ ret = wait_any(fd, 1, &sem, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem, 0, 2);
+
+ ret = wait_any(fd, 1, &sem, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ count = 3;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOVERFLOW, errno);
+ check_sem_state(sem, 0, 2);
+
+ count = 2;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, count);
+ check_sem_state(sem, 2, 2);
+
+ ret = wait_any(fd, 1, &sem, 123, &index);
+ EXPECT_EQ(0, ret);
+ ret = wait_any(fd, 1, &sem, 123, &index);
+ EXPECT_EQ(0, ret);
+
+ count = 1;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, count);
+ check_sem_state(sem, 1, 2);
+
+ count = ~0u;
+ ret = post_sem(sem, &count);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOVERFLOW, errno);
+ check_sem_state(sem, 1, 2);
+
+ close(sem);
+
+ close(fd);
+}
+
+TEST_HARNESS_MAIN
--
2.43.0
^ permalink raw reply related
* [PATCH v3 19/30] selftests: ntsync: Add some tests for NTSYNC_IOC_WAIT_ANY.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test basic synchronous functionality of NTSYNC_IOC_WAIT_ANY, when objects are
considered signaled or not signaled, and how they are affected by a successful
wait.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 119 ++++++++++++++++++
1 file changed, 119 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 7cd0f40594fd..40ad8cbd3138 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -342,4 +342,123 @@ TEST(mutex_state)
close(fd);
}
+TEST(test_wait_any)
+{
+ int objs[NTSYNC_MAX_WAIT_COUNT + 1], fd, ret;
+ struct ntsync_mutex_args mutex_args = {0};
+ struct ntsync_sem_args sem_args = {0};
+ __u32 owner, index, count, i;
+ struct timespec timeout;
+
+ clock_gettime(CLOCK_MONOTONIC, &timeout);
+
+ fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, fd);
+
+ sem_args.count = 2;
+ sem_args.max = 3;
+ sem_args.sem = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_SEM, &sem_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, sem_args.sem);
+
+ mutex_args.owner = 0;
+ mutex_args.count = 0;
+ mutex_args.mutex = 0xdeadbeef;
+ ret = ioctl(fd, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ EXPECT_NE(0xdeadbeef, mutex_args.mutex);
+
+ objs[0] = sem_args.sem;
+ objs[1] = mutex_args.mutex;
+
+ ret = wait_any(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 1, 3);
+ check_mutex_state(mutex_args.mutex, 0, 0);
+
+ ret = wait_any(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_mutex_state(mutex_args.mutex, 0, 0);
+
+ ret = wait_any(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_mutex_state(mutex_args.mutex, 1, 123);
+
+ count = 1;
+ ret = post_sem(sem_args.sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, count);
+
+ ret = wait_any(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_mutex_state(mutex_args.mutex, 1, 123);
+
+ ret = wait_any(fd, 2, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, index);
+ check_sem_state(sem_args.sem, 0, 3);
+ check_mutex_state(mutex_args.mutex, 2, 123);
+
+ ret = wait_any(fd, 2, objs, 456, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ owner = 123;
+ ret = ioctl(mutex_args.mutex, NTSYNC_IOC_MUTEX_KILL, &owner);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_any(fd, 2, objs, 456, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EOWNERDEAD, errno);
+ EXPECT_EQ(1, index);
+
+ ret = wait_any(fd, 2, objs, 456, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(1, index);
+
+ /* test waiting on the same object twice */
+ count = 2;
+ ret = post_sem(sem_args.sem, &count);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, count);
+
+ objs[0] = objs[1] = sem_args.sem;
+ ret = wait_any(fd, 2, objs, 456, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+ check_sem_state(sem_args.sem, 1, 3);
+
+ ret = wait_any(fd, 0, NULL, 456, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(ETIMEDOUT, errno);
+
+ for (i = 0; i < NTSYNC_MAX_WAIT_COUNT + 1; ++i)
+ objs[i] = sem_args.sem;
+
+ ret = wait_any(fd, NTSYNC_MAX_WAIT_COUNT, objs, 123, &index);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, index);
+
+ ret = wait_any(fd, NTSYNC_MAX_WAIT_COUNT + 1, objs, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ ret = wait_any(fd, -1, objs, 123, &index);
+ EXPECT_EQ(-1, ret);
+ EXPECT_EQ(EINVAL, errno);
+
+ close(sem_args.sem);
+ close(mutex_args.mutex);
+
+ close(fd);
+}
+
TEST_HARNESS_MAIN
--
2.43.0
^ permalink raw reply related
* [PATCH v3 30/30] docs: ntsync: Add documentation for the ntsync uAPI.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Add an overall explanation of the driver architecture, and complete and precise
specification for its intended behaviour.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
Documentation/userspace-api/index.rst | 1 +
Documentation/userspace-api/ntsync.rst | 399 +++++++++++++++++++++++++
2 files changed, 400 insertions(+)
create mode 100644 Documentation/userspace-api/ntsync.rst
diff --git a/Documentation/userspace-api/index.rst b/Documentation/userspace-api/index.rst
index afecfe3cc4a8..d5745a500fa7 100644
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -62,6 +62,7 @@ Everything else
vduse
futex2
perf_ring_buffer
+ ntsync
.. only:: subproject and html
diff --git a/Documentation/userspace-api/ntsync.rst b/Documentation/userspace-api/ntsync.rst
new file mode 100644
index 000000000000..202c2350d3af
--- /dev/null
+++ b/Documentation/userspace-api/ntsync.rst
@@ -0,0 +1,399 @@
+===================================
+NT synchronization primitive driver
+===================================
+
+This page documents the user-space API for the ntsync driver.
+
+ntsync is a support driver for emulation of NT synchronization
+primitives by user-space NT emulators. It exists because implementation
+in user-space, using existing tools, cannot match Windows performance
+while offering accurate semantics. It is implemented entirely in
+software, and does not drive any hardware device.
+
+This interface is meant as a compatibility tool only, and should not
+be used for general synchronization. Instead use generic, versatile
+interfaces such as futex(2) and poll(2).
+
+Synchronization primitives
+==========================
+
+The ntsync driver exposes three types of synchronization primitives:
+semaphores, mutexes, and events.
+
+A semaphore holds a single volatile 32-bit counter, and a static 32-bit
+integer denoting the maximum value. It is considered signaled when the
+counter is nonzero. The counter is decremented by one when a wait is
+satisfied. Both the initial and maximum count are established when the
+semaphore is created.
+
+A mutex holds a volatile 32-bit recursion count, and a volatile 32-bit
+identifier denoting its owner. A mutex is considered signaled when its
+owner is zero (indicating that it is not owned). The recursion count is
+incremented when a wait is satisfied, and ownership is set to the given
+identifier.
+
+A mutex also holds an internal flag denoting whether its previous owner
+has died; such a mutex is said to be abandoned. Owner death is not
+tracked automatically based on thread death, but rather must be
+communicated using ``NTSYNC_IOC_MUTEX_KILL``. An abandoned mutex is
+inherently considered unowned.
+
+Except for the "unowned" semantics of zero, the actual value of the
+owner identifier is not interpreted by the ntsync driver at all. The
+intended use is to store a thread identifier; however, the ntsync
+driver does not actually validate that a calling thread provides
+consistent or unique identifiers.
+
+An event holds a volatile boolean state denoting whether it is signaled
+or not. There are two types of events, auto-reset and manual-reset. An
+auto-reset event is designaled when a wait is satisfied; a manual-reset
+event is not. The event type is specified when the event is created.
+
+Unless specified otherwise, all operations on an object are atomic and
+totally ordered with respect to other operations on the same object.
+
+Objects are represented by files. When all file descriptors to an
+object are closed, that object is deleted.
+
+Char device
+===========
+
+The ntsync driver creates a single char device /dev/ntsync. Each file
+description opened on the device represents a unique instance intended
+to back an individual NT virtual machine. Objects created by one ntsync
+instance may only be used with other objects created by the same
+instance.
+
+ioctl reference
+===============
+
+All operations on the device are done through ioctls. There are four
+structures used in ioctl calls::
+
+ struct ntsync_sem_args {
+ __u32 sem;
+ __u32 count;
+ __u32 max;
+ };
+
+ struct ntsync_mutex_args {
+ __u32 mutex;
+ __u32 owner;
+ __u32 count;
+ };
+
+ struct ntsync_event_args {
+ __u32 event;
+ __u32 signaled;
+ __u32 manual;
+ };
+
+ struct ntsync_wait_args {
+ __u64 timeout;
+ __u64 objs;
+ __u32 count;
+ __u32 owner;
+ __u32 index;
+ __u32 alert;
+ __u32 flags;
+ __u32 pad;
+ };
+
+Depending on the ioctl, members of the structure may be used as input,
+output, or not at all. All ioctls return 0 on success.
+
+The ioctls on the device file are as follows:
+
+.. c:macro:: NTSYNC_IOC_CREATE_SEM
+
+ Create a semaphore object. Takes a pointer to struct
+ :c:type:`ntsync_sem_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``sem``
+ - On output, contains a file descriptor to the created semaphore.
+ * - ``count``
+ - Initial count of the semaphore.
+ * - ``max``
+ - Maximum count of the semaphore.
+
+ Fails with ``EINVAL`` if ``count`` is greater than ``max``.
+
+.. c:macro:: NTSYNC_IOC_CREATE_MUTEX
+
+ Create a mutex object. Takes a pointer to struct
+ :c:type:`ntsync_mutex_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``mutex``
+ - On output, contains a file descriptor to the created mutex.
+ * - ``count``
+ - Initial recursion count of the mutex.
+ * - ``owner``
+ - Initial owner of the mutex.
+
+ If ``owner`` is nonzero and ``count`` is zero, or if ``owner`` is
+ zero and ``count`` is nonzero, the function fails with ``EINVAL``.
+
+.. c:macro:: NTSYNC_IOC_CREATE_EVENT
+
+ Create an event object. Takes a pointer to struct
+ :c:type:`ntsync_event_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``event``
+ - On output, contains a file descriptor to the created event.
+ * - ``signaled``
+ - If nonzero, the event is initially signaled, otherwise
+ nonsignaled.
+ * - ``manual``
+ - If nonzero, the event is a manual-reset event, otherwise
+ auto-reset.
+
+The ioctls on the individual objects are as follows:
+
+.. c:macro:: NTSYNC_IOC_SEM_POST
+
+ Post to a semaphore object. Takes a pointer to a 32-bit integer,
+ which on input holds the count to be added to the semaphore, and on
+ output contains its previous count.
+
+ If adding to the semaphore's current count would raise the latter
+ past the semaphore's maximum count, the ioctl fails with
+ ``EOVERFLOW`` and the semaphore is not affected. If raising the
+ semaphore's count causes it to become signaled, eligible threads
+ waiting on this semaphore will be woken and the semaphore's count
+ decremented appropriately.
+
+.. c:macro:: NTSYNC_IOC_MUTEX_UNLOCK
+
+ Release a mutex object. Takes a pointer to struct
+ :c:type:`ntsync_mutex_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``mutex``
+ - Ignored.
+ * - ``owner``
+ - Specifies the owner trying to release this mutex.
+ * - ``count``
+ - On output, contains the previous recursion count.
+
+ If ``owner`` is zero, the ioctl fails with ``EINVAL``. If ``owner``
+ is not the current owner of the mutex, the ioctl fails with
+ ``EPERM``.
+
+ The mutex's count will be decremented by one. If decrementing the
+ mutex's count causes it to become zero, the mutex is marked as
+ unowned and signaled, and eligible threads waiting on it will be
+ woken as appropriate.
+
+.. c:macro:: NTSYNC_IOC_SET_EVENT
+
+ Signal an event object. Takes a pointer to a 32-bit integer, which on
+ output contains the previous state of the event.
+
+ Eligible threads will be woken, and auto-reset events will be
+ designaled appropriately.
+
+.. c:macro:: NTSYNC_IOC_RESET_EVENT
+
+ Designal an event object. Takes a pointer to a 32-bit integer, which
+ on output contains the previous state of the event.
+
+.. c:macro:: NTSYNC_IOC_PULSE_EVENT
+
+ Wake threads waiting on an event object while leaving it in an
+ unsignaled state. Takes a pointer to a 32-bit integer, which on
+ output contains the previous state of the event.
+
+ A pulse operation can be thought of as a set followed by a reset,
+ performed as a single atomic operation. If two threads are waiting on
+ an auto-reset event which is pulsed, only one will be woken. If two
+ threads are waiting a manual-reset event which is pulsed, both will
+ be woken. However, in both cases, the event will be unsignaled
+ afterwards, and a simultaneous read operation will always report the
+ event as unsignaled.
+
+.. c:macro:: NTSYNC_IOC_READ_SEM
+
+ Read the current state of a semaphore object. Takes a pointer to
+ struct :c:type:`ntsync_sem_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``sem``
+ - Ignored.
+ * - ``count``
+ - On output, contains the current count of the semaphore.
+ * - ``max``
+ - On output, contains the maximum count of the semaphore.
+
+.. c:macro:: NTSYNC_IOC_READ_MUTEX
+
+ Read the current state of a mutex object. Takes a pointer to struct
+ :c:type:`ntsync_mutex_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``mutex``
+ - Ignored.
+ * - ``owner``
+ - On output, contains the current owner of the mutex, or zero
+ if the mutex is not currently owned.
+ * - ``count``
+ - On output, contains the current recursion count of the mutex.
+
+ If the mutex is marked as abandoned, the function fails with
+ ``EOWNERDEAD``. In this case, ``count`` and ``owner`` are set to
+ zero.
+
+.. c:macro:: NTSYNC_IOC_READ_EVENT
+
+ Read the current state of an event object. Takes a pointer to struct
+ :c:type:`ntsync_event_args`, which is used as follows:
+
+ .. list-table::
+
+ * - ``event``
+ - Ignored.
+ * - ``signaled``
+ - On output, contains the current state of the event.
+ * - ``manual``
+ - On output, contains 1 if the event is a manual-reset event,
+ and 0 otherwise.
+
+.. c:macro:: NTSYNC_IOC_KILL_OWNER
+
+ Mark a mutex as unowned and abandoned if it is owned by the given
+ owner. Takes an input-only pointer to a 32-bit integer denoting the
+ owner. If the owner is zero, the ioctl fails with ``EINVAL``. If the
+ owner does not own the mutex, the function fails with ``EPERM``.
+
+ Eligible threads waiting on the mutex will be woken as appropriate
+ (and such waits will fail with ``EOWNERDEAD``, as described below).
+
+.. c:macro:: NTSYNC_IOC_WAIT_ANY
+
+ Poll on any of a list of objects, atomically acquiring at most one.
+ Takes a pointer to struct :c:type:`ntsync_wait_args`, which is
+ used as follows:
+
+ .. list-table::
+
+ * - ``timeout``
+ - Absolute timeout in nanoseconds. If ``NTSYNC_WAIT_REALTIME``
+ is set, the timeout is measured against the REALTIME clock;
+ otherwise it is measured against the MONOTONIC clock. If the
+ timeout is equal to or earlier than the current time, the
+ function returns immediately without sleeping. If ``timeout``
+ is U64_MAX, the function will sleep until an object is
+ signaled, and will not fail with ``ETIMEDOUT``.
+ * - ``objs``
+ - Pointer to an array of ``count`` file descriptors
+ (specified as an integer so that the structure has the same
+ size regardless of architecture). If any object is
+ invalid, the function fails with ``EINVAL``.
+ * - ``count``
+ - Number of objects specified in the ``objs`` array.
+ If greater than ``NTSYNC_MAX_WAIT_COUNT``, the function fails
+ with ``EINVAL``.
+ * - ``owner``
+ - Mutex owner identifier. If any object in ``objs`` is a mutex,
+ the ioctl will attempt to acquire that mutex on behalf of
+ ``owner``. If ``owner`` is zero, the ioctl fails with
+ ``EINVAL``.
+ * - ``index``
+ - On success, contains the index (into ``objs``) of the object
+ which was signaled. If ``alert`` was signaled instead,
+ this contains ``count``.
+ * - ``alert``
+ - Optional event object file descriptor. If nonzero, this
+ specifies an "alert" event object which, if signaled, will
+ terminate the wait. If nonzero, the identifier must point to a
+ valid event.
+ * - ``flags``
+ - Zero or more flags. Currently the only flag is
+ ``NTSYNC_WAIT_REALTIME``, which causes the timeout to be
+ measured against the REALTIME clock instead of MONOTONIC.
+ * - ``pad``
+ - Unused, must be set to zero.
+
+ This function attempts to acquire one of the given objects. If unable
+ to do so, it sleeps until an object becomes signaled, subsequently
+ acquiring it, or the timeout expires. In the latter case the ioctl
+ fails with ``ETIMEDOUT``. The function only acquires one object, even
+ if multiple objects are signaled.
+
+ A semaphore is considered to be signaled if its count is nonzero, and
+ is acquired by decrementing its count by one. A mutex is considered
+ to be signaled if it is unowned or if its owner matches the ``owner``
+ argument, and is acquired by incrementing its recursion count by one
+ and setting its owner to the ``owner`` argument. An auto-reset event
+ is acquired by designaling it; a manual-reset event is not affected
+ by acquisition.
+
+ Acquisition is atomic and totally ordered with respect to other
+ operations on the same object. If two wait operations (with different
+ ``owner`` identifiers) are queued on the same mutex, only one is
+ signaled. If two wait operations are queued on the same semaphore,
+ and a value of one is posted to it, only one is signaled. The order
+ in which threads are signaled is not specified.
+
+ If an abandoned mutex is acquired, the ioctl fails with
+ ``EOWNERDEAD``. Although this is a failure return, the function may
+ otherwise be considered successful. The mutex is marked as owned by
+ the given owner (with a recursion count of 1) and as no longer
+ abandoned, and ``index`` is still set to the index of the mutex.
+
+ The ``alert`` argument is an "extra" event which can terminate the
+ wait, independently of all other objects. If members of ``objs`` and
+ ``alert`` are both simultaneously signaled, a member of ``objs`` will
+ always be given priority and acquired first.
+
+ It is valid to pass the same object more than once, including by
+ passing the same event in the ``objs`` array and in ``alert``. If a
+ wakeup occurs due to that object being signaled, ``index`` is set to
+ the lowest index corresponding to that object.
+
+ The function may fail with ``EINTR`` if a signal is received.
+
+.. c:macro:: NTSYNC_IOC_WAIT_ALL
+
+ Poll on a list of objects, atomically acquiring all of them. Takes a
+ pointer to struct :c:type:`ntsync_wait_args`, which is used
+ identically to ``NTSYNC_IOC_WAIT_ANY``, except that ``index`` is
+ always filled with zero on success if not woken via alert.
+
+ This function attempts to simultaneously acquire all of the given
+ objects. If unable to do so, it sleeps until all objects become
+ simultaneously signaled, subsequently acquiring them, or the timeout
+ expires. In the latter case the ioctl fails with ``ETIMEDOUT`` and no
+ objects are modified.
+
+ Objects may become signaled and subsequently designaled (through
+ acquisition by other threads) while this thread is sleeping. Only
+ once all objects are simultaneously signaled does the ioctl acquire
+ them and return. The entire acquisition is atomic and totally ordered
+ with respect to other operations on any of the given objects.
+
+ If an abandoned mutex is acquired, the ioctl fails with
+ ``EOWNERDEAD``. Similarly to ``NTSYNC_IOC_WAIT_ANY``, all objects are
+ nevertheless marked as acquired. Note that if multiple mutex objects
+ are specified, there is no way to know which were marked as
+ abandoned.
+
+ As with "any" waits, the ``alert`` argument is an "extra" event which
+ can terminate the wait. Critically, however, an "all" wait will
+ succeed if all members in ``objs`` are signaled, *or* if ``alert`` is
+ signaled. In the latter case ``index`` will be set to ``count``. As
+ with "any" waits, if both conditions are filled, the former takes
+ priority, and objects in ``objs`` will be acquired.
+
+ Unlike ``NTSYNC_IOC_WAIT_ANY``, it is not valid to pass the same
+ object more than once, nor is it valid to pass the same object in
+ ``objs`` and in ``alert``. If this is attempted, the function fails
+ with ``EINVAL``.
--
2.43.0
^ permalink raw reply related
* [PATCH v3 29/30] maintainers: Add an entry for ntsync.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Add myself as maintainer, supported by CodeWeavers.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
MAINTAINERS | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index aa3b947fb080..84d03d95f44b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15720,6 +15720,15 @@ T: git https://github.com/Paragon-Software-Group/linux-ntfs3.git
F: Documentation/filesystems/ntfs3.rst
F: fs/ntfs3/
+NTSYNC SYNCHRONIZATION PRIMITIVE DRIVER
+M: Elizabeth Figura <zfigura@codeweavers.com>
+L: wine-devel@winehq.org
+S: Supported
+F: Documentation/userspace-api/ntsync.rst
+F: drivers/misc/ntsync.c
+F: include/uapi/linux/ntsync.h
+F: tools/testing/selftests/drivers/ntsync/
+
NUBUS SUBSYSTEM
M: Finn Thain <fthain@linux-m68k.org>
L: linux-m68k@lists.linux-m68k.org
--
2.43.0
^ permalink raw reply related
* [PATCH v3 28/30] selftests: ntsync: Add a stress test for contended waits.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Test a more realistic usage pattern, and one with heavy contention, in order to
actually exercise ntsync's internal synchronization.
This test has several threads in a tight loop acquiring a mutex, modifying some
shared data, and then releasing the mutex. At the end we check if the data is
consistent.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 74 +++++++++++++++++++
1 file changed, 74 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 968874d7e325..5fa2c9a0768c 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -1330,4 +1330,78 @@ TEST(alert_all)
close(fd);
}
+#define STRESS_LOOPS 10000
+#define STRESS_THREADS 4
+
+static unsigned int stress_counter;
+static int stress_device, stress_start_event, stress_mutex;
+
+static void *stress_thread(void *arg)
+{
+ struct ntsync_wait_args wait_args = {0};
+ __u32 index, count, i;
+ int ret;
+
+ wait_args.timeout = UINT64_MAX;
+ wait_args.count = 1;
+ wait_args.objs = (uintptr_t)&stress_start_event;
+ wait_args.owner = gettid();
+ wait_args.index = 0xdeadbeef;
+
+ ioctl(stress_device, NTSYNC_IOC_WAIT_ANY, &wait_args);
+
+ wait_args.objs = (uintptr_t)&stress_mutex;
+
+ for (i = 0; i < STRESS_LOOPS; ++i) {
+ ioctl(stress_device, NTSYNC_IOC_WAIT_ANY, &wait_args);
+
+ ++stress_counter;
+
+ unlock_mutex(stress_mutex, wait_args.owner, &count);
+ }
+
+ return NULL;
+}
+
+TEST(stress_wait)
+{
+ struct ntsync_event_args event_args;
+ struct ntsync_mutex_args mutex_args;
+ pthread_t threads[STRESS_THREADS];
+ __u32 signaled, i;
+ int ret;
+
+ stress_device = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
+ ASSERT_LE(0, stress_device);
+
+ mutex_args.owner = 0;
+ mutex_args.count = 0;
+ ret = ioctl(stress_device, NTSYNC_IOC_CREATE_MUTEX, &mutex_args);
+ EXPECT_EQ(0, ret);
+ stress_mutex = mutex_args.mutex;
+
+ event_args.manual = 1;
+ event_args.signaled = 0;
+ ret = ioctl(stress_device, NTSYNC_IOC_CREATE_EVENT, &event_args);
+ EXPECT_EQ(0, ret);
+ stress_start_event = event_args.event;
+
+ for (i = 0; i < STRESS_THREADS; ++i)
+ pthread_create(&threads[i], NULL, stress_thread, NULL);
+
+ ret = ioctl(stress_start_event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+
+ for (i = 0; i < STRESS_THREADS; ++i) {
+ ret = pthread_join(threads[i], NULL);
+ EXPECT_EQ(0, ret);
+ }
+
+ EXPECT_EQ(STRESS_LOOPS * STRESS_THREADS, stress_counter);
+
+ close(stress_start_event);
+ close(stress_mutex);
+ close(stress_device);
+}
+
TEST_HARNESS_MAIN
--
2.43.0
^ permalink raw reply related
* [PATCH v3 27/30] selftests: ntsync: Add some tests for wakeup signaling via alerts.
From: Elizabeth Figura @ 2024-03-29 0:06 UTC (permalink / raw)
To: Arnd Bergmann, Greg Kroah-Hartman, Jonathan Corbet, Shuah Khan
Cc: linux-kernel, linux-api, wine-devel, André Almeida,
Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra, Andy Lutomirski,
linux-doc, linux-kselftest, Randy Dunlap, Elizabeth Figura
In-Reply-To: <20240329000621.148791-1-zfigura@codeweavers.com>
Expand the alert tests to cover alerting a thread mid-wait, to test that the
relevant scheduling logic works correctly.
Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
---
.../testing/selftests/drivers/ntsync/ntsync.c | 62 +++++++++++++++++++
1 file changed, 62 insertions(+)
diff --git a/tools/testing/selftests/drivers/ntsync/ntsync.c b/tools/testing/selftests/drivers/ntsync/ntsync.c
index 5465a16d38b3..968874d7e325 100644
--- a/tools/testing/selftests/drivers/ntsync/ntsync.c
+++ b/tools/testing/selftests/drivers/ntsync/ntsync.c
@@ -1113,9 +1113,12 @@ TEST(wake_all)
TEST(alert_any)
{
struct ntsync_event_args event_args = {0};
+ struct ntsync_wait_args wait_args = {0};
struct ntsync_sem_args sem_args = {0};
__u32 index, count, signaled;
+ struct wait_args thread_args;
int objs[2], fd, ret;
+ pthread_t thread;
fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
ASSERT_LE(0, fd);
@@ -1163,6 +1166,34 @@ TEST(alert_any)
EXPECT_EQ(0, ret);
EXPECT_EQ(2, index);
+ /* test wakeup via alert */
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+
+ wait_args.timeout = get_abs_timeout(1000);
+ wait_args.objs = (uintptr_t)objs;
+ wait_args.count = 2;
+ wait_args.owner = 123;
+ wait_args.index = 0xdeadbeef;
+ wait_args.alert = event_args.event;
+ thread_args.fd = fd;
+ thread_args.args = &wait_args;
+ thread_args.request = NTSYNC_IOC_WAIT_ANY;
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(2, wait_args.index);
+
close(event_args.event);
/* test with an auto-reset event */
@@ -1199,9 +1230,12 @@ TEST(alert_any)
TEST(alert_all)
{
struct ntsync_event_args event_args = {0};
+ struct ntsync_wait_args wait_args = {0};
struct ntsync_sem_args sem_args = {0};
+ struct wait_args thread_args;
__u32 index, count, signaled;
int objs[2], fd, ret;
+ pthread_t thread;
fd = open("/dev/ntsync", O_CLOEXEC | O_RDONLY);
ASSERT_LE(0, fd);
@@ -1235,6 +1269,34 @@ TEST(alert_all)
EXPECT_EQ(0, ret);
EXPECT_EQ(2, index);
+ /* test wakeup via alert */
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_RESET, &signaled);
+ EXPECT_EQ(0, ret);
+
+ wait_args.timeout = get_abs_timeout(1000);
+ wait_args.objs = (uintptr_t)objs;
+ wait_args.count = 2;
+ wait_args.owner = 123;
+ wait_args.index = 0xdeadbeef;
+ wait_args.alert = event_args.event;
+ thread_args.fd = fd;
+ thread_args.args = &wait_args;
+ thread_args.request = NTSYNC_IOC_WAIT_ALL;
+ ret = pthread_create(&thread, NULL, wait_thread, &thread_args);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(ETIMEDOUT, ret);
+
+ ret = ioctl(event_args.event, NTSYNC_IOC_EVENT_SET, &signaled);
+ EXPECT_EQ(0, ret);
+
+ ret = wait_for_thread(thread, 100);
+ EXPECT_EQ(0, ret);
+ EXPECT_EQ(0, thread_args.ret);
+ EXPECT_EQ(2, wait_args.index);
+
close(event_args.event);
/* test with an auto-reset event */
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Sagi Maimon @ 2024-03-28 15:40 UTC (permalink / raw)
To: Thomas Gleixner
Cc: richardcochran, luto, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, mszeredi, casey, reibax,
davem, brauner, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <878r29hjds.ffs@tglx>
Hi Thomas
On Sat, Mar 23, 2024 at 2:38 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Sagi!
>
> On Wed, Mar 20 2024 at 16:42, Sagi Maimon wrote:
> > On Thu, Mar 14, 2024 at 8:08 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> Which is maximaly precise under the assumption that in the time between
> >> the sample points a1 and a2 neither the system clock nor the PCH clocks
> >> are changing their frequency significantly. That is a valid assumption
> >> when you put a reasonable upper bound on d2.
> >>
> >
> > You are right.
> > In fact, we are running this calculation on a user space application.
> > We use the new system call to get pairs of mono and PHC and then run
> > that calculation in user space.
> > That is why the system call returns pairs of clock samples and not the
> > diff between them.
>
> Please stop claiming things which are fundamentally wrong:
>
> The proposed system call returns the PHC sample and the midpoint of
> two CLOCK_WHATEVER samples which have been sampled before and after
> the PHC sample.
>
> That is fundamentally different from a pair of samples as I explained
> to you in great length more than once by now.
>
> I understand that you can't rely on the PTP_SYS_OFFSET_PRECISE IOCTL
> alone because there is not much hardware support, but what you are
> proposing is way worse than the other two PTP_SYS_OFFSET variants.
>
> PTP_SYS_OFFSET at least gives the caller a choice of analysis of the
> interleaving system timestamps.
>
> PTP_SYS_OFFSET_EXTENDED moves the outer sample points as close as
> possible to the actual PCH read and provides both outer samples to user
> space for analysis. It was introduced for a reason, no?
>
> Your proposed system call is just declaring arbitrarily that the
> CLOCK_WHATEVER sample is exactly the midpoint of the two outer samples
> and is therefore superior and correct.
>
> It is neither superior nor correct because the midpoint is an
> ill-defined assumption as I explained to you multiple times now.
>
> Aside of that the approach loses the extended information of
> PTP_SYS_OFFSET and PTP_SYS_OFFSET_EXTENDED including the more precise
> sampling behaviour of the latter. IOW, it is ignoring and throwing away
> the effort of people who cared about making the best out of the
> limitations of hardware including the already existing algorithms to
> make sense out of it.
>
> The P at the beginning of PTP does not mean 'Potentially precise' and
> the lack of C in PTP does not mean that Correctness is overrated.
>
> The problem is that these non hardware assisted IOCTL variants sample
> only CLOCK_REALTIME and not CLOCK_MONOTONIC_RAW, which is all you need
> to solve your problem at hand, no?
>
> That's absolutely not rocket science to solve. The below sketch does
> exactly that without creating an ill-defined syscall monstrosity, at the
> same time it is fully preserving the benefits of the existing IOCTL
> variants and therefore allows to apply already existing algorithms to
> analyse that data. That's too simple and too obviously correct, right?
>
> The thing is a sketch and it's neither compiled nor tested. It's just
> for illustration and you can keep all bugs you might find in it.
>
> On top this needs an analyis whether any of the gettimex64()
> implementations does something special instead of invoking the
> ptp_read_system_prets() and ptp_read_system_postts() helpers as close as
> possible to the PCH readout, but that's not rocket science either. It's
> just 21 callbacks to look at.
>
I like your suggestion, thanks!
it is what our user space needs from the kernel and with minimum kernel changes.
I will write it, test it and upload it with your permission (it is you
idea after all).
> It might also require a new set of variant '3' IOTCLS to make that flag
> field work, but that's not going to make the change more complex and
> it's an exercise left to the experts of that IOCTL interface.
>
I think that I understand your meaning.
There is a backward compatibility problem here.
Existing user space application using PTP_SYS_OFFSET_EXTENDED ioctl
won't have any problems
because of the "extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]"
test, but what about all
old user space applications using: PTP_SYS_OFFSET ?
How can it be solved?
Can you explain what you meant above regarding the IOCTL?
Thanks,
Sagi
> Thanks,
>
> tglx
> ---
> drivers/ptp/ptp_chardev.c | 36 ++++++++++++++++++++++--------------
> include/linux/ptp_clock_kernel.h | 28 +++++++++++++++++++---------
> include/uapi/linux/ptp_clock.h | 10 ++++++++--
> 3 files changed, 49 insertions(+), 25 deletions(-)
>
> --- a/drivers/ptp/ptp_chardev.c
> +++ b/drivers/ptp/ptp_chardev.c
> @@ -164,9 +164,9 @@ long ptp_ioctl(struct posix_clock_contex
> struct ptp_sys_offset_precise precise_offset;
> struct system_device_crosststamp xtstamp;
> struct ptp_clock_info *ops = ptp->info;
> + struct ptp_system_timestamp sts = { };
> struct ptp_sys_offset *sysoff = NULL;
> struct timestamp_event_queue *tsevq;
> - struct ptp_system_timestamp sts;
> struct ptp_clock_request req;
> struct ptp_clock_caps caps;
> struct ptp_clock_time *pct;
> @@ -358,11 +358,13 @@ long ptp_ioctl(struct posix_clock_contex
> extoff = NULL;
> break;
> }
> - if (extoff->n_samples > PTP_MAX_SAMPLES
> - || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
> + if (!extoff->n_samples || extoff->n_samples > PTP_MAX_SAMPLES ||
> + (extoff->flags & ~PTP_SYS_OFFSET_VALID_FLAGS) ||
> + extoff->rsv[0] || extoff->rsv[1]) {
> err = -EINVAL;
> break;
> }
> + sts.flags = extoff->flags;
> for (i = 0; i < extoff->n_samples; i++) {
> err = ptp->info->gettimex64(ptp->info, &ts, &sts);
> if (err)
> @@ -386,29 +388,35 @@ long ptp_ioctl(struct posix_clock_contex
> sysoff = NULL;
> break;
> }
> - if (sysoff->n_samples > PTP_MAX_SAMPLES) {
> + if (!sysoff->n_samples || sysoff->n_samples > PTP_MAX_SAMPLES ||
> + (sysoff->flags & ~PTP_SYS_OFFSET_VALID_FLAGS) ||
> + sysoff->rsv[0] || sysoff->rsv[1]) {
> err = -EINVAL;
> break;
> }
> + sts.flags = sysoff->flags;
> pct = &sysoff->ts[0];
> for (i = 0; i < sysoff->n_samples; i++) {
> - ktime_get_real_ts64(&ts);
> - pct->sec = ts.tv_sec;
> - pct->nsec = ts.tv_nsec;
> - pct++;
> - if (ops->gettimex64)
> - err = ops->gettimex64(ops, &ts, NULL);
> - else
> + if (ops->gettimex64) {
> + err = ops->gettimex64(ops, &ts, &sts);
> + } else {
> + ptp_read_system_prets(&sts);
> err = ops->gettime64(ops, &ts);
> + }
> if (err)
> goto out;
> +
> + pct->sec = sts.pre_ts.tv_sec;
> + pct->nsec = sts.pre_ts.tv_nsec;
> + pct++;
> pct->sec = ts.tv_sec;
> pct->nsec = ts.tv_nsec;
> pct++;
> }
> - ktime_get_real_ts64(&ts);
> - pct->sec = ts.tv_sec;
> - pct->nsec = ts.tv_nsec;
> + if (!ops->gettimex64)
> + ptp_read_system_postts(&sts);
> + pct->sec = sts.post_ts.tv_sec;
> + pct->nsec = sts.post_ts.tv_nsec;
> if (copy_to_user((void __user *)arg, sysoff, sizeof(*sysoff)))
> err = -EFAULT;
> break;
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -44,13 +44,15 @@ struct ptp_clock_request {
> struct system_device_crosststamp;
>
> /**
> - * struct ptp_system_timestamp - system time corresponding to a PHC timestamp
> - * @pre_ts: system timestamp before capturing PHC
> - * @post_ts: system timestamp after capturing PHC
> + * struct ptp_system_timestamp - System time corresponding to a PHC timestamp
> + * @flags: Flags to select the system clock to sample
> + * @pre_ts: System timestamp before capturing PHC
> + * @post_ts: System timestamp after capturing PHC
> */
> struct ptp_system_timestamp {
> - struct timespec64 pre_ts;
> - struct timespec64 post_ts;
> + unsigned int flags;
> + struct timespec64 pre_ts;
> + struct timespec64 post_ts;
> };
>
> /**
> @@ -457,14 +459,22 @@ static inline ktime_t ptp_convert_timest
>
> static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
> {
> - if (sts)
> - ktime_get_real_ts64(&sts->pre_ts);
> + if (sts) {
> + if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> + ktime_get_raw_ts64(&sts->pre_ts);
> + else
> + ktime_get_real_ts64(&sts->pre_ts);
> + }
> }
>
> static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
> {
> - if (sts)
> - ktime_get_real_ts64(&sts->post_ts);
> + if (sts) {
> + if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
> + ktime_get_raw_ts64(&sts->post_ts);
> + else
> + ktime_get_real_ts64(&sts->post_ts);
> + }
> }
>
> #endif
> --- a/include/uapi/linux/ptp_clock.h
> +++ b/include/uapi/linux/ptp_clock.h
> @@ -76,6 +76,10 @@
> */
> #define PTP_PEROUT_V1_VALID_FLAGS (0)
>
> +/* Bits for PTP_SYS_OFFSET and PTP_SYS_OFFSET_EXTENDED */
> +#define PTP_SYS_OFFSET_MONO_RAW (1U << 0)
> +#define PTP_SYS_OFFSET_VALID_FLAGS (PTP_SYS_OFFSET_MONO_RAW)
> +
> /*
> * struct ptp_clock_time - represents a time value
> *
> @@ -146,7 +150,8 @@ struct ptp_perout_request {
>
> struct ptp_sys_offset {
> unsigned int n_samples; /* Desired number of measurements. */
> - unsigned int rsv[3]; /* Reserved for future use. */
> + unsigned int flags;
> + unsigned int rsv[2]; /* Reserved for future use. */
> /*
> * Array of interleaved system/phc time stamps. The kernel
> * will provide 2*n_samples + 1 time stamps, with the last
> @@ -157,7 +162,8 @@ struct ptp_sys_offset {
>
> struct ptp_sys_offset_extended {
> unsigned int n_samples; /* Desired number of measurements. */
> - unsigned int rsv[3]; /* Reserved for future use. */
> + unsigned int flags;
> + unsigned int rsv[2]; /* Reserved for future use. */
> /*
> * Array of [system, phc, system] time stamps. The kernel will provide
> * 3*n_samples time stamps.
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Kurt Kanzenbach @ 2024-03-24 11:04 UTC (permalink / raw)
To: Thomas Gleixner, Sagi Maimon
Cc: richardcochran, luto, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, mszeredi, casey, reibax,
davem, brauner, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <875xxdhj8k.ffs@tglx>
[-- Attachment #1: Type: text/plain, Size: 760 bytes --]
On Sat Mar 23 2024, Thomas Gleixner wrote:
> On Sat, Mar 23 2024 at 01:38, Thomas Gleixner wrote:
>> PTP_SYS_OFFSET_EXTENDED moves the outer sample points as close as
>> possible to the actual PCH read and provides both outer samples to user
>> space for analysis. It was introduced for a reason, no?
>
> That said, it's a sad state of affairs that 16 drivers which did exist
> before the introduction of the gettimex64() callback have not been
> converted over to it within 4.5 years.
>
> What's even worse is that 14 drivers have been merged _after_ the
> gettimex64() callback got introduced without implementing it:
>
[...]
> 2020-11-05 drivers/net/dsa/hirschmann/hellcreek_ptp.c
Oh, my bad. Let me switch this one to gettimex64() then.
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-03-23 0:42 UTC (permalink / raw)
To: Sagi Maimon
Cc: richardcochran, luto, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, mszeredi, casey, reibax,
davem, brauner, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <878r29hjds.ffs@tglx>
On Sat, Mar 23 2024 at 01:38, Thomas Gleixner wrote:
> PTP_SYS_OFFSET_EXTENDED moves the outer sample points as close as
> possible to the actual PCH read and provides both outer samples to user
> space for analysis. It was introduced for a reason, no?
That said, it's a sad state of affairs that 16 drivers which did exist
before the introduction of the gettimex64() callback have not been
converted over to it within 4.5 years.
What's even worse is that 14 drivers have been merged _after_ the
gettimex64() callback got introduced without implementing it:
2019-02-12 drivers/net/ethernet/freescale/enetc/enetc_ptp.c
2019-10-24 drivers/net/ethernet/aquantia/atlantic/aq_ptp.c
2019-11-15 drivers/net/dsa/ocelot/felix_vsc9959.c
2020-01-12 drivers/net/ethernet/xscale/ptp_ixp46x.c
2020-06-20 drivers/net/ethernet/mscc/ocelot_vsc7514.c
2020-06-24 drivers/net/phy/mscc/mscc_ptp.c
2020-08-24 drivers/net/ethernet/marvell/octeontx2/nic/otx2_ptp.c
2020-11-05 drivers/net/dsa/hirschmann/hellcreek_ptp.c
2022-02-01 drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
2022-03-04 drivers/net/ethernet/microchip/sparx5/sparx5_ptp.c
2022-05-10 drivers/net/ethernet/sfc/siena/ptp.c
2022-11-02 drivers/net/ethernet/renesas/rcar_gen4_ptp.c
2023-01-13 drivers/net/dsa/microchip/ksz_ptp.c
2023-03-22 drivers/net/wireless/intel/iwlwifi/mvm/ptp.c
Not Sagi's fault at all, but it's telling and coherent with the approach
to solve the problem at hand.
See the previous reply for the observation on the letters P and C in PTP.
Oh well,
tglx
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Thomas Gleixner @ 2024-03-23 0:38 UTC (permalink / raw)
To: Sagi Maimon
Cc: richardcochran, luto, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, mszeredi, casey, reibax,
davem, brauner, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <CAMuE1bHBky9NGP22PVHKdi2+WniwxiLSmMnwRM6wm36sU8W4jA@mail.gmail.com>
Sagi!
On Wed, Mar 20 2024 at 16:42, Sagi Maimon wrote:
> On Thu, Mar 14, 2024 at 8:08 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>> Which is maximaly precise under the assumption that in the time between
>> the sample points a1 and a2 neither the system clock nor the PCH clocks
>> are changing their frequency significantly. That is a valid assumption
>> when you put a reasonable upper bound on d2.
>>
>
> You are right.
> In fact, we are running this calculation on a user space application.
> We use the new system call to get pairs of mono and PHC and then run
> that calculation in user space.
> That is why the system call returns pairs of clock samples and not the
> diff between them.
Please stop claiming things which are fundamentally wrong:
The proposed system call returns the PHC sample and the midpoint of
two CLOCK_WHATEVER samples which have been sampled before and after
the PHC sample.
That is fundamentally different from a pair of samples as I explained
to you in great length more than once by now.
I understand that you can't rely on the PTP_SYS_OFFSET_PRECISE IOCTL
alone because there is not much hardware support, but what you are
proposing is way worse than the other two PTP_SYS_OFFSET variants.
PTP_SYS_OFFSET at least gives the caller a choice of analysis of the
interleaving system timestamps.
PTP_SYS_OFFSET_EXTENDED moves the outer sample points as close as
possible to the actual PCH read and provides both outer samples to user
space for analysis. It was introduced for a reason, no?
Your proposed system call is just declaring arbitrarily that the
CLOCK_WHATEVER sample is exactly the midpoint of the two outer samples
and is therefore superior and correct.
It is neither superior nor correct because the midpoint is an
ill-defined assumption as I explained to you multiple times now.
Aside of that the approach loses the extended information of
PTP_SYS_OFFSET and PTP_SYS_OFFSET_EXTENDED including the more precise
sampling behaviour of the latter. IOW, it is ignoring and throwing away
the effort of people who cared about making the best out of the
limitations of hardware including the already existing algorithms to
make sense out of it.
The P at the beginning of PTP does not mean 'Potentially precise' and
the lack of C in PTP does not mean that Correctness is overrated.
The problem is that these non hardware assisted IOCTL variants sample
only CLOCK_REALTIME and not CLOCK_MONOTONIC_RAW, which is all you need
to solve your problem at hand, no?
That's absolutely not rocket science to solve. The below sketch does
exactly that without creating an ill-defined syscall monstrosity, at the
same time it is fully preserving the benefits of the existing IOCTL
variants and therefore allows to apply already existing algorithms to
analyse that data. That's too simple and too obviously correct, right?
The thing is a sketch and it's neither compiled nor tested. It's just
for illustration and you can keep all bugs you might find in it.
On top this needs an analyis whether any of the gettimex64()
implementations does something special instead of invoking the
ptp_read_system_prets() and ptp_read_system_postts() helpers as close as
possible to the PCH readout, but that's not rocket science either. It's
just 21 callbacks to look at.
It might also require a new set of variant '3' IOTCLS to make that flag
field work, but that's not going to make the change more complex and
it's an exercise left to the experts of that IOCTL interface.
Thanks,
tglx
---
drivers/ptp/ptp_chardev.c | 36 ++++++++++++++++++++++--------------
include/linux/ptp_clock_kernel.h | 28 +++++++++++++++++++---------
include/uapi/linux/ptp_clock.h | 10 ++++++++--
3 files changed, 49 insertions(+), 25 deletions(-)
--- a/drivers/ptp/ptp_chardev.c
+++ b/drivers/ptp/ptp_chardev.c
@@ -164,9 +164,9 @@ long ptp_ioctl(struct posix_clock_contex
struct ptp_sys_offset_precise precise_offset;
struct system_device_crosststamp xtstamp;
struct ptp_clock_info *ops = ptp->info;
+ struct ptp_system_timestamp sts = { };
struct ptp_sys_offset *sysoff = NULL;
struct timestamp_event_queue *tsevq;
- struct ptp_system_timestamp sts;
struct ptp_clock_request req;
struct ptp_clock_caps caps;
struct ptp_clock_time *pct;
@@ -358,11 +358,13 @@ long ptp_ioctl(struct posix_clock_contex
extoff = NULL;
break;
}
- if (extoff->n_samples > PTP_MAX_SAMPLES
- || extoff->rsv[0] || extoff->rsv[1] || extoff->rsv[2]) {
+ if (!extoff->n_samples || extoff->n_samples > PTP_MAX_SAMPLES ||
+ (extoff->flags & ~PTP_SYS_OFFSET_VALID_FLAGS) ||
+ extoff->rsv[0] || extoff->rsv[1]) {
err = -EINVAL;
break;
}
+ sts.flags = extoff->flags;
for (i = 0; i < extoff->n_samples; i++) {
err = ptp->info->gettimex64(ptp->info, &ts, &sts);
if (err)
@@ -386,29 +388,35 @@ long ptp_ioctl(struct posix_clock_contex
sysoff = NULL;
break;
}
- if (sysoff->n_samples > PTP_MAX_SAMPLES) {
+ if (!sysoff->n_samples || sysoff->n_samples > PTP_MAX_SAMPLES ||
+ (sysoff->flags & ~PTP_SYS_OFFSET_VALID_FLAGS) ||
+ sysoff->rsv[0] || sysoff->rsv[1]) {
err = -EINVAL;
break;
}
+ sts.flags = sysoff->flags;
pct = &sysoff->ts[0];
for (i = 0; i < sysoff->n_samples; i++) {
- ktime_get_real_ts64(&ts);
- pct->sec = ts.tv_sec;
- pct->nsec = ts.tv_nsec;
- pct++;
- if (ops->gettimex64)
- err = ops->gettimex64(ops, &ts, NULL);
- else
+ if (ops->gettimex64) {
+ err = ops->gettimex64(ops, &ts, &sts);
+ } else {
+ ptp_read_system_prets(&sts);
err = ops->gettime64(ops, &ts);
+ }
if (err)
goto out;
+
+ pct->sec = sts.pre_ts.tv_sec;
+ pct->nsec = sts.pre_ts.tv_nsec;
+ pct++;
pct->sec = ts.tv_sec;
pct->nsec = ts.tv_nsec;
pct++;
}
- ktime_get_real_ts64(&ts);
- pct->sec = ts.tv_sec;
- pct->nsec = ts.tv_nsec;
+ if (!ops->gettimex64)
+ ptp_read_system_postts(&sts);
+ pct->sec = sts.post_ts.tv_sec;
+ pct->nsec = sts.post_ts.tv_nsec;
if (copy_to_user((void __user *)arg, sysoff, sizeof(*sysoff)))
err = -EFAULT;
break;
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -44,13 +44,15 @@ struct ptp_clock_request {
struct system_device_crosststamp;
/**
- * struct ptp_system_timestamp - system time corresponding to a PHC timestamp
- * @pre_ts: system timestamp before capturing PHC
- * @post_ts: system timestamp after capturing PHC
+ * struct ptp_system_timestamp - System time corresponding to a PHC timestamp
+ * @flags: Flags to select the system clock to sample
+ * @pre_ts: System timestamp before capturing PHC
+ * @post_ts: System timestamp after capturing PHC
*/
struct ptp_system_timestamp {
- struct timespec64 pre_ts;
- struct timespec64 post_ts;
+ unsigned int flags;
+ struct timespec64 pre_ts;
+ struct timespec64 post_ts;
};
/**
@@ -457,14 +459,22 @@ static inline ktime_t ptp_convert_timest
static inline void ptp_read_system_prets(struct ptp_system_timestamp *sts)
{
- if (sts)
- ktime_get_real_ts64(&sts->pre_ts);
+ if (sts) {
+ if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
+ ktime_get_raw_ts64(&sts->pre_ts);
+ else
+ ktime_get_real_ts64(&sts->pre_ts);
+ }
}
static inline void ptp_read_system_postts(struct ptp_system_timestamp *sts)
{
- if (sts)
- ktime_get_real_ts64(&sts->post_ts);
+ if (sts) {
+ if (sts->flags & PTP_SYS_OFFSET_MONO_RAW)
+ ktime_get_raw_ts64(&sts->post_ts);
+ else
+ ktime_get_real_ts64(&sts->post_ts);
+ }
}
#endif
--- a/include/uapi/linux/ptp_clock.h
+++ b/include/uapi/linux/ptp_clock.h
@@ -76,6 +76,10 @@
*/
#define PTP_PEROUT_V1_VALID_FLAGS (0)
+/* Bits for PTP_SYS_OFFSET and PTP_SYS_OFFSET_EXTENDED */
+#define PTP_SYS_OFFSET_MONO_RAW (1U << 0)
+#define PTP_SYS_OFFSET_VALID_FLAGS (PTP_SYS_OFFSET_MONO_RAW)
+
/*
* struct ptp_clock_time - represents a time value
*
@@ -146,7 +150,8 @@ struct ptp_perout_request {
struct ptp_sys_offset {
unsigned int n_samples; /* Desired number of measurements. */
- unsigned int rsv[3]; /* Reserved for future use. */
+ unsigned int flags;
+ unsigned int rsv[2]; /* Reserved for future use. */
/*
* Array of interleaved system/phc time stamps. The kernel
* will provide 2*n_samples + 1 time stamps, with the last
@@ -157,7 +162,8 @@ struct ptp_sys_offset {
struct ptp_sys_offset_extended {
unsigned int n_samples; /* Desired number of measurements. */
- unsigned int rsv[3]; /* Reserved for future use. */
+ unsigned int flags;
+ unsigned int rsv[2]; /* Reserved for future use. */
/*
* Array of [system, phc, system] time stamps. The kernel will provide
* 3*n_samples time stamps.
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Sreenivas Bagalkote @ 2024-03-22 13:24 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-cxl, Brett Henning, Harold Johnson, Sumanesh Samanta,
Williams, Dan J, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh, Ariel.Sibley
In-Reply-To: <20240322093212.00003173@Huawei.com>
[-- Attachment #1.1: Type: text/plain, Size: 17763 bytes --]
Jonathan,
>
> What is the use case? My understanding so far is that clouds and
> similar sometimes use an in band path but it would be from a management
> only host, not a general purpose host running other software
>
The overwhelming majority of the PCIe switches get deployed in a single
server. Typically four to eight switches are connected to two or more root
complexes in one or two CPUs. The deployment scenario you have in mind -
multiple physical hosts running general workloads and a management-only
host - exists. But it is insignificant.
>
> For telemetry(subject to any odd corners like commands that might lock
> the interface up for a long time, which we've seen with commands in the
> Spec!) I don't see any problem supporting those on all host software.
> They should be non destructive to other hosts etc.
>
Thank you. As you do this, please keep in mind that your concern about not
affecting "other" hosts is theoretically valid but doesn't exist in the
real world beyond science experiments. If there are real-world deployments,
they are insignificant. I urge you all to make your stuff work with 99.99%
of the deployments.
>
> 'Maybe' if you were to publish a specification for those particular
> vendor defined commands, it might be fine to add them to the allow list
> for the switch-cci.
>
Your proposal sounds reasonable. I will let you all experts figure out how
to support the vendor-defined commands. CXL spec has them for a reason and
they need to be supported.
Sreeni
On Fri, Mar 22, 2024 at 2:32 AM Jonathan Cameron <
Jonathan.Cameron@huawei.com> wrote:
> On Thu, 21 Mar 2024 14:41:00 -0700
> Sreenivas Bagalkote <sreenivas.bagalkote@broadcom.com> wrote:
>
> > Thank you for kicking off this discussion, Jonathan.
>
> Hi Sreenivas,
>
> >
> > We need guidance from the community.
> >
> > 1. Datacenter customers must be able to manage PCIe switches in-band.
>
> What is the use case? My understanding so far is that clouds and
> similar sometimes use an in band path but it would be from a management
> only host, not a general purpose host running other software. Sure
> that control host just connects to a different upstream port so, from
> a switch point of view, it's the same as any other host. From a host
> software point of view it's not running general cloud workloads or
> (at least in most cases) a general purpose OS distribution.
>
> This is the key question behind this discussion.
>
> > 2. Management of switches includes getting health, performance, and error
> > telemetry.
>
> For telemetry(subject to any odd corners like commands that might lock
> the interface up for a long time, which we've seen with commands in the
> Spec!) I don't see any problem supporting those on all host software.
> They should be non destructive to other hosts etc.
>
> > 3. These telemetry functions are not yet part of the CXL standard
>
> Ok, so this we should try to pin down the boundaries around this.
> The thread linked below lays out the reasoning behind a general rule
> of not accepting vendor defined commands, but perhaps there are routes
> to answer some of those concerns.
>
> 'Maybe' if you were to publish a specification for those particular
> vendor defined commands, it might be fine to add them to the allow list
> for the switch-cci. Key here is that Broadcom would be committing to not
> using those particular opcodes from the vendor space for anything else
> in the future (so we could match on VID + opcode). This is similar to
> some DVSEC usage in PCIe (and why DVSEC is different from VSEC).
>
> Effectively you'd be publishing an additional specification building on
> CXL.
> Those are expected to surface anyway from various standards orgs - should
> we treat a company published one differently? I don't see why.
> Exactly how this would work might take some figuring out (in main code,
> separate driver module etc?)
>
> That specification would be expected to provide a similar level of detail
> to CXL spec defined commands (ideally the less vague ones, but meh, up to
> you as long as any side effects are clearly documented!)
>
> Speaking for myself, I'd consider this approach.
> Particularly true if I see clear effort in the standards org to push
> these into future specifications as that shows broadcom are trying to
> enhance the ecosystems.
>
>
> > 4. We built the CCI mailboxes into our PCIe switches per CXL spec and
> > developed our management scheme around them.
> >
> > If the Linux community does not allow a CXL spec-compliant switch to be
> > managed via the CXL spec-defined CCI mailbox, then please guide us on
> > the right approach. Please tell us how you propose we manage our switches
> > in-band.
>
> The Linux community is fine supporting this in the kernel (the BMC or
> Fabric Management only host case - option 2 below, so the code will be
> there)
> the question here is what advice we offer to the general purpose
> distributions and what protections we need to put in place to mitigate the
> 'blast radius' concerns.
>
> Jonathan
> >
> > Thank you
> > Sreeni
> >
> > On Thu, Mar 21, 2024 at 10:44 AM Jonathan Cameron <
> > Jonathan.Cameron@huawei.com> wrote:
> >
> > > Hi All,
> > >
> > > This is has come up in a number of discussions both on list and in
> private,
> > > so I wanted to lay out a potential set of rules when deciding whether
> or
> > > not
> > > to provide a user space interface for a particular feature of CXL
> Fabric
> > > Management. The intent is to drive discussion, not to simply tell
> people
> > > a set of rules. I've brought this to the public lists as it's a Linux
> > > kernel
> > > policy discussion, not a standards one.
> > >
> > > Whilst I'm writing the RFC this my attempt to summarize a possible
> > > position rather than necessarily being my personal view.
> > >
> > > It's a straw man - shoot at it!
> > >
> > > Not everyone in this discussion is familiar with relevant kernel or CXL
> > > concepts
> > > so I've provided more info than I normally would.
> > >
> > > First some background:
> > > ======================
> > >
> > > CXL has two different types of Fabric. The comments here refer to
> both, but
> > > for now the kernel stack is focused on the simpler VCS fabric, not the
> more
> > > recent Port Based Routing (PBR) Fabrics. A typical example for 2 hosts
> > > connected to a common switch looks something like:
> > >
> > > ________________ _______________
> > > | | | | Hosts - each sees
> > > | HOST A | | HOST B | a PCIe style tree
> > > | | | | but from a fabric
> > > config
> > > | |Root Port| | | |Root Port| | point of view it's
> more
> > > -------|-------- -------|------- complex.
> > > | |
> > > | |
> > > _______|______________________________|________
> > > | USP (SW-CCI) USP | Switch can have lots
> of
> > > | | | | Upstream Ports. Each
> one
> > > | ____|________ _______|______ | has a virtual
> hierarchy.
> > > | | | | | |
> > > | vPPB vPPB vPPB vPPB| There are virtual
> > > | x | | | | "downstream
> > > ports."(vPPBs)
> > > | \ / / | That can be bound to
> real
> > > | \ / / | downstream ports.
> > > | \ / / |
> > > | \ / / | Multi Logical
> Devices are
> > > | DSP0 DSP1 DSP 2 | support more than one
> > > vPPB
> > > ------------------------------------------------ bound to a single
> > > physical
> > > | | | DSP (transactions are
> > > tagged
> > > | | | with an LD-ID)
> > > SLD0 MLD0 SLD1
> > >
> > > Some typical fabric management activities:
> > > 1) Bind/Unbind vPPB to physical DSP (Results in hotplug / unplug
> events)
> > > 2) Access config space or BAR space of End Points below the switch.
> > > 3) Tunneling messages through to devices downstream (e.g Dynamic
> Capacity
> > > Forced Remove that will blow away some memory even if a host is
> using
> > > it).
> > > 4) Non destructive stuff like status read back.
> > >
> > > Given the hosts may be using the Type 3 hosted memory (either Single
> > > Logical
> > > Device - SLD, or an LD on a Multi logical Device - MLD) as normal
> memory,
> > > unbinding a device in use can result in the memory access from a
> > > different host being removed. The 'blast radius' is perhaps a rack of
> > > servers. This discussion applies equally to FM-API commands sent to
> Multi
> > > Head Devices (see CXL r3.1).
> > >
> > > The Fabric Management actions are done using the CXL spec defined
> Fabric
> > > Management API, (FM-API) which is transported over various means
> including
> > > OoB MCTP over your favourite transport (I2C, PCIe-VDM...) or via normal
> > > PCIe read/write to a Switch-CCI. A Switch-CCI is mailbox in PCI BAR
> > > space on a function found alongside one of the switch upstream ports;
> > > this mailbox is very similar to the MMPT definition found in PCIe r6.2.
> > >
> > > In many cases this switch CCI / MCTP connection is used by a BMC rather
> > > than a normal host, but there have been some questions raised about
> whether
> > > a general purpose server OS would have a valid reason to use this
> interface
> > > (beyond debug and testing) to configure the switch or an MHD.
> > >
> > > If people have a use case for this, please reply to this thread to give
> > > more details.
> > >
> > > The most recently posted CXL Switch-CCI support only provided the RAW
> CXL
> > > command IOCTL interface that is already available for Type 3 memory
> > > devices.
> > > That allows for unfettered control of the switch but, because it is
> > > extremely easy to shoot yourself in the foot and cause unsolvable bug
> > > reports,
> > > it taints the kernel. There have been several requests to provide this
> > > interface
> > > without the taint for these switch configuration mailboxes.
> > >
> > > Last posted series:
> > >
> > >
> https://lore.kernel.org/all/20231016125323.18318-1-Jonathan.Cameron@huawei.com/
> > > Note there are unrelated reasons why that code hasn't been updated
> since
> > > v6.6 time,
> > > but I am planning to get back to it shortly.
> > >
> > > Similar issues will occur for other uses of PCIe MMPT (new mailbox in
> PCI
> > > that
> > > sometimes is used for similarly destructive activity such as PLDM based
> > > firmware update).
> > >
> > >
> > > On to the proposed rules:
> > >
> > > 1) Kernel space use of the various mailboxes, or filtered controls from
> > > user space.
> > >
> > >
> ==================================================================================
> > >
> > > Absolutely fine - no one worries about this, but the mediated traffic
> will
> > > be filtered for potentially destructive side effects. E.g. it will
> reject
> > > attempts to change anything routing related if the kernel either knows
> a
> > > host is
> > > using memory that will be blown away, or has no way to know (so
> affecting
> > > routing to another host). This includes blocking 'all' vendor defined
> > > messages as we have no idea what the do. Note this means the kernel
> has
> > > an allow list and new commands are not initially allowed.
> > >
> > > This isn't currently enabled for Switch CCIs because they are only
> really
> > > interesting if the potentially destructive stuff is available (an
> earlier
> > > version did enable query commands, but it wasn't particularly useful to
> > > know what your switch could do but not be allowed to do any of it).
> > > If you take a MMPT usecase of PLDM firmware update, the filtering would
> > > check that the device was in a state where a firmware update won't rip
> > > memory out from under a host, which would be messy if that host is
> > > doing the update.
> > >
> > > 2) Unfiltered userspace use of mailbox for Fabric Management - BMC
> kernels
> > >
> ==========================================================================
> > >
> > > (This would just be a kernel option that we'd advise normal server
> > > distributions not to turn on. Would be enabled by openBMC etc)
> > >
> > > This is fine - there is some work to do, but the switch-cci PCI driver
> > > will hopefully be ready for upstream merge soon. There is no filtering
> of
> > > accesses. Think of this as similar to all the damage you can do via
> > > MCTP from a BMC. Similarly it is likely that much of the complexity
> > > of the actual commands will be left to user space tooling:
> > > https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
> > >
> > > Whether Kconfig help text is strong enough to ensure this only gets
> > > enabled for BMC targeted distros is an open question we can address
> > > alongside an updated patch set.
> > >
> > > (On to the one that the "debate" is about)
> > >
> > > 3) Unfiltered user space use of mailbox for Fabric Management - Distro
> > > kernels
> > >
> > >
> =============================================================================
> > > (General purpose Linux Server Distro (Redhat, Suse etc))
> > >
> > > This is equivalent of RAW command support on CXL Type 3 memory devices.
> > > You can enable those in a distro kernel build despite the scary config
> > > help text, but if you use it the kernel is tainted. The result
> > > of the taint is to add a flag to bug reports and print a big message
> to say
> > > that you've used a feature that might result in you shooting yourself
> > > in the foot.
> > >
> > > The taint is there because software is not at first written to deal
> with
> > > everything that can happen smoothly (e.g. surprise removal) It's hard
> > > to survive some of these events, so is never on the initial feature
> list
> > > for any bus, so this flag is just to indicate we have entered a world
> > > where almost all bets are off wrt to stability. We might not know what
> > > a command does so we can't assess the impact (and no one trusts vendor
> > > commands to report affects right in the Command Effects Log - which
> > > in theory tells you if a command can result problems).
> > >
> > > A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
> > > (a r3.1 feature) but, as I understand it, these are intended for a
> > > host to configure and should not have side effects on other hosts?
> > > My working assumption is that the kernel driver stack will handle
> > > these (once we catch up with the current feature backlog!) Currently
> > > we have no visibility of what the OS driver stack for a fabrics will
> > > actually look like - the spec is just the starting point for that.
> > > (patches welcome ;)
> > >
> > > The various CXL upstream developers and maintainers may have
> > > differing views of course, but my current understanding is we want
> > > to support 1 and 2, but are very resistant to 3!
> > >
> > > General Notes
> > > =============
> > >
> > > One side aspect of why we really don't like unfiltered userspace
> access to
> > > any
> > > of these devices is that people start building non standard hacks in
> and we
> > > lose the ecosystem advantages. Forcing a considered discussion +
> patches
> > > to let a particular command be supported, drives standardization.
> > >
> > >
> > >
> https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
> > > provides some history on vendor specific extensions and why in general
> we
> > > won't support them upstream.
> > >
> > > To address another question raised in an earlier discussion:
> > > Putting these Fabric Management interfaces behind guard rails of some
> type
> > > (e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
> > > of non standard interfaces, because we will be even less likely to
> accept
> > > those upstream!
> > >
> > > If anyone needs more details on any aspect of this please ask.
> > > There are a lot of things involved and I've only tried to give a fairly
> > > minimal illustration to drive the discussion. I may well have missed
> > > something crucial.
> > >
> > > Jonathan
> > >
> > >
> >
>
>
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
[-- Attachment #1.2: Type: text/html, Size: 22928 bytes --]
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4230 bytes --]
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-03-22 9:32 UTC (permalink / raw)
To: Sreenivas Bagalkote
Cc: linux-cxl, Brett Henning, Harold Johnson, Sumanesh Samanta,
Williams, Dan J, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh, Ariel.Sibley
In-Reply-To: <CACX_a4VPV16OFNZLCVUJrGFR5brcdiYN3aAgrxtO8ksUdNdkQQ@mail.gmail.com>
On Thu, 21 Mar 2024 14:41:00 -0700
Sreenivas Bagalkote <sreenivas.bagalkote@broadcom.com> wrote:
> Thank you for kicking off this discussion, Jonathan.
Hi Sreenivas,
>
> We need guidance from the community.
>
> 1. Datacenter customers must be able to manage PCIe switches in-band.
What is the use case? My understanding so far is that clouds and
similar sometimes use an in band path but it would be from a management
only host, not a general purpose host running other software. Sure
that control host just connects to a different upstream port so, from
a switch point of view, it's the same as any other host. From a host
software point of view it's not running general cloud workloads or
(at least in most cases) a general purpose OS distribution.
This is the key question behind this discussion.
> 2. Management of switches includes getting health, performance, and error
> telemetry.
For telemetry(subject to any odd corners like commands that might lock
the interface up for a long time, which we've seen with commands in the
Spec!) I don't see any problem supporting those on all host software.
They should be non destructive to other hosts etc.
> 3. These telemetry functions are not yet part of the CXL standard
Ok, so this we should try to pin down the boundaries around this.
The thread linked below lays out the reasoning behind a general rule
of not accepting vendor defined commands, but perhaps there are routes
to answer some of those concerns.
'Maybe' if you were to publish a specification for those particular
vendor defined commands, it might be fine to add them to the allow list
for the switch-cci. Key here is that Broadcom would be committing to not
using those particular opcodes from the vendor space for anything else
in the future (so we could match on VID + opcode). This is similar to
some DVSEC usage in PCIe (and why DVSEC is different from VSEC).
Effectively you'd be publishing an additional specification building on CXL.
Those are expected to surface anyway from various standards orgs - should
we treat a company published one differently? I don't see why.
Exactly how this would work might take some figuring out (in main code,
separate driver module etc?)
That specification would be expected to provide a similar level of detail
to CXL spec defined commands (ideally the less vague ones, but meh, up to
you as long as any side effects are clearly documented!)
Speaking for myself, I'd consider this approach.
Particularly true if I see clear effort in the standards org to push
these into future specifications as that shows broadcom are trying to
enhance the ecosystems.
> 4. We built the CCI mailboxes into our PCIe switches per CXL spec and
> developed our management scheme around them.
>
> If the Linux community does not allow a CXL spec-compliant switch to be
> managed via the CXL spec-defined CCI mailbox, then please guide us on
> the right approach. Please tell us how you propose we manage our switches
> in-band.
The Linux community is fine supporting this in the kernel (the BMC or
Fabric Management only host case - option 2 below, so the code will be there)
the question here is what advice we offer to the general purpose
distributions and what protections we need to put in place to mitigate the
'blast radius' concerns.
Jonathan
>
> Thank you
> Sreeni
>
> On Thu, Mar 21, 2024 at 10:44 AM Jonathan Cameron <
> Jonathan.Cameron@huawei.com> wrote:
>
> > Hi All,
> >
> > This is has come up in a number of discussions both on list and in private,
> > so I wanted to lay out a potential set of rules when deciding whether or
> > not
> > to provide a user space interface for a particular feature of CXL Fabric
> > Management. The intent is to drive discussion, not to simply tell people
> > a set of rules. I've brought this to the public lists as it's a Linux
> > kernel
> > policy discussion, not a standards one.
> >
> > Whilst I'm writing the RFC this my attempt to summarize a possible
> > position rather than necessarily being my personal view.
> >
> > It's a straw man - shoot at it!
> >
> > Not everyone in this discussion is familiar with relevant kernel or CXL
> > concepts
> > so I've provided more info than I normally would.
> >
> > First some background:
> > ======================
> >
> > CXL has two different types of Fabric. The comments here refer to both, but
> > for now the kernel stack is focused on the simpler VCS fabric, not the more
> > recent Port Based Routing (PBR) Fabrics. A typical example for 2 hosts
> > connected to a common switch looks something like:
> >
> > ________________ _______________
> > | | | | Hosts - each sees
> > | HOST A | | HOST B | a PCIe style tree
> > | | | | but from a fabric
> > config
> > | |Root Port| | | |Root Port| | point of view it's more
> > -------|-------- -------|------- complex.
> > | |
> > | |
> > _______|______________________________|________
> > | USP (SW-CCI) USP | Switch can have lots of
> > | | | | Upstream Ports. Each one
> > | ____|________ _______|______ | has a virtual hierarchy.
> > | | | | | |
> > | vPPB vPPB vPPB vPPB| There are virtual
> > | x | | | | "downstream
> > ports."(vPPBs)
> > | \ / / | That can be bound to real
> > | \ / / | downstream ports.
> > | \ / / |
> > | \ / / | Multi Logical Devices are
> > | DSP0 DSP1 DSP 2 | support more than one
> > vPPB
> > ------------------------------------------------ bound to a single
> > physical
> > | | | DSP (transactions are
> > tagged
> > | | | with an LD-ID)
> > SLD0 MLD0 SLD1
> >
> > Some typical fabric management activities:
> > 1) Bind/Unbind vPPB to physical DSP (Results in hotplug / unplug events)
> > 2) Access config space or BAR space of End Points below the switch.
> > 3) Tunneling messages through to devices downstream (e.g Dynamic Capacity
> > Forced Remove that will blow away some memory even if a host is using
> > it).
> > 4) Non destructive stuff like status read back.
> >
> > Given the hosts may be using the Type 3 hosted memory (either Single
> > Logical
> > Device - SLD, or an LD on a Multi logical Device - MLD) as normal memory,
> > unbinding a device in use can result in the memory access from a
> > different host being removed. The 'blast radius' is perhaps a rack of
> > servers. This discussion applies equally to FM-API commands sent to Multi
> > Head Devices (see CXL r3.1).
> >
> > The Fabric Management actions are done using the CXL spec defined Fabric
> > Management API, (FM-API) which is transported over various means including
> > OoB MCTP over your favourite transport (I2C, PCIe-VDM...) or via normal
> > PCIe read/write to a Switch-CCI. A Switch-CCI is mailbox in PCI BAR
> > space on a function found alongside one of the switch upstream ports;
> > this mailbox is very similar to the MMPT definition found in PCIe r6.2.
> >
> > In many cases this switch CCI / MCTP connection is used by a BMC rather
> > than a normal host, but there have been some questions raised about whether
> > a general purpose server OS would have a valid reason to use this interface
> > (beyond debug and testing) to configure the switch or an MHD.
> >
> > If people have a use case for this, please reply to this thread to give
> > more details.
> >
> > The most recently posted CXL Switch-CCI support only provided the RAW CXL
> > command IOCTL interface that is already available for Type 3 memory
> > devices.
> > That allows for unfettered control of the switch but, because it is
> > extremely easy to shoot yourself in the foot and cause unsolvable bug
> > reports,
> > it taints the kernel. There have been several requests to provide this
> > interface
> > without the taint for these switch configuration mailboxes.
> >
> > Last posted series:
> >
> > https://lore.kernel.org/all/20231016125323.18318-1-Jonathan.Cameron@huawei.com/
> > Note there are unrelated reasons why that code hasn't been updated since
> > v6.6 time,
> > but I am planning to get back to it shortly.
> >
> > Similar issues will occur for other uses of PCIe MMPT (new mailbox in PCI
> > that
> > sometimes is used for similarly destructive activity such as PLDM based
> > firmware update).
> >
> >
> > On to the proposed rules:
> >
> > 1) Kernel space use of the various mailboxes, or filtered controls from
> > user space.
> >
> > ==================================================================================
> >
> > Absolutely fine - no one worries about this, but the mediated traffic will
> > be filtered for potentially destructive side effects. E.g. it will reject
> > attempts to change anything routing related if the kernel either knows a
> > host is
> > using memory that will be blown away, or has no way to know (so affecting
> > routing to another host). This includes blocking 'all' vendor defined
> > messages as we have no idea what the do. Note this means the kernel has
> > an allow list and new commands are not initially allowed.
> >
> > This isn't currently enabled for Switch CCIs because they are only really
> > interesting if the potentially destructive stuff is available (an earlier
> > version did enable query commands, but it wasn't particularly useful to
> > know what your switch could do but not be allowed to do any of it).
> > If you take a MMPT usecase of PLDM firmware update, the filtering would
> > check that the device was in a state where a firmware update won't rip
> > memory out from under a host, which would be messy if that host is
> > doing the update.
> >
> > 2) Unfiltered userspace use of mailbox for Fabric Management - BMC kernels
> > ==========================================================================
> >
> > (This would just be a kernel option that we'd advise normal server
> > distributions not to turn on. Would be enabled by openBMC etc)
> >
> > This is fine - there is some work to do, but the switch-cci PCI driver
> > will hopefully be ready for upstream merge soon. There is no filtering of
> > accesses. Think of this as similar to all the damage you can do via
> > MCTP from a BMC. Similarly it is likely that much of the complexity
> > of the actual commands will be left to user space tooling:
> > https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
> >
> > Whether Kconfig help text is strong enough to ensure this only gets
> > enabled for BMC targeted distros is an open question we can address
> > alongside an updated patch set.
> >
> > (On to the one that the "debate" is about)
> >
> > 3) Unfiltered user space use of mailbox for Fabric Management - Distro
> > kernels
> >
> > =============================================================================
> > (General purpose Linux Server Distro (Redhat, Suse etc))
> >
> > This is equivalent of RAW command support on CXL Type 3 memory devices.
> > You can enable those in a distro kernel build despite the scary config
> > help text, but if you use it the kernel is tainted. The result
> > of the taint is to add a flag to bug reports and print a big message to say
> > that you've used a feature that might result in you shooting yourself
> > in the foot.
> >
> > The taint is there because software is not at first written to deal with
> > everything that can happen smoothly (e.g. surprise removal) It's hard
> > to survive some of these events, so is never on the initial feature list
> > for any bus, so this flag is just to indicate we have entered a world
> > where almost all bets are off wrt to stability. We might not know what
> > a command does so we can't assess the impact (and no one trusts vendor
> > commands to report affects right in the Command Effects Log - which
> > in theory tells you if a command can result problems).
> >
> > A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
> > (a r3.1 feature) but, as I understand it, these are intended for a
> > host to configure and should not have side effects on other hosts?
> > My working assumption is that the kernel driver stack will handle
> > these (once we catch up with the current feature backlog!) Currently
> > we have no visibility of what the OS driver stack for a fabrics will
> > actually look like - the spec is just the starting point for that.
> > (patches welcome ;)
> >
> > The various CXL upstream developers and maintainers may have
> > differing views of course, but my current understanding is we want
> > to support 1 and 2, but are very resistant to 3!
> >
> > General Notes
> > =============
> >
> > One side aspect of why we really don't like unfiltered userspace access to
> > any
> > of these devices is that people start building non standard hacks in and we
> > lose the ecosystem advantages. Forcing a considered discussion + patches
> > to let a particular command be supported, drives standardization.
> >
> >
> > https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
> > provides some history on vendor specific extensions and why in general we
> > won't support them upstream.
> >
> > To address another question raised in an earlier discussion:
> > Putting these Fabric Management interfaces behind guard rails of some type
> > (e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
> > of non standard interfaces, because we will be even less likely to accept
> > those upstream!
> >
> > If anyone needs more details on any aspect of this please ask.
> > There are a lot of things involved and I've only tried to give a fairly
> > minimal illustration to drive the discussion. I may well have missed
> > something crucial.
> >
> > Jonathan
> >
> >
>
^ permalink raw reply
* Re: RFC: Restricting userspace interfaces for CXL fabric management
From: Sreenivas Bagalkote @ 2024-03-21 21:41 UTC (permalink / raw)
To: Jonathan Cameron
Cc: linux-cxl, Brett Henning, Harold Johnson, Sumanesh Samanta,
Williams, Dan J, linux-kernel, Davidlohr Bueso, Dave Jiang,
Alison Schofield, Vishal Verma, Ira Weiny, linuxarm, linux-api,
Lorenzo Pieralisi, Natu, Mahesh
In-Reply-To: <20240321174423.00007e0d@Huawei.com>
[-- Attachment #1.1: Type: text/plain, Size: 12219 bytes --]
Thank you for kicking off this discussion, Jonathan.
We need guidance from the community.
1. Datacenter customers must be able to manage PCIe switches in-band.
2. Management of switches includes getting health, performance, and error
telemetry.
3. These telemetry functions are not yet part of the CXL standard
4. We built the CCI mailboxes into our PCIe switches per CXL spec and
developed our management scheme around them.
If the Linux community does not allow a CXL spec-compliant switch to be
managed via the CXL spec-defined CCI mailbox, then please guide us on
the right approach. Please tell us how you propose we manage our switches
in-band.
Thank you
Sreeni
On Thu, Mar 21, 2024 at 10:44 AM Jonathan Cameron <
Jonathan.Cameron@huawei.com> wrote:
> Hi All,
>
> This is has come up in a number of discussions both on list and in private,
> so I wanted to lay out a potential set of rules when deciding whether or
> not
> to provide a user space interface for a particular feature of CXL Fabric
> Management. The intent is to drive discussion, not to simply tell people
> a set of rules. I've brought this to the public lists as it's a Linux
> kernel
> policy discussion, not a standards one.
>
> Whilst I'm writing the RFC this my attempt to summarize a possible
> position rather than necessarily being my personal view.
>
> It's a straw man - shoot at it!
>
> Not everyone in this discussion is familiar with relevant kernel or CXL
> concepts
> so I've provided more info than I normally would.
>
> First some background:
> ======================
>
> CXL has two different types of Fabric. The comments here refer to both, but
> for now the kernel stack is focused on the simpler VCS fabric, not the more
> recent Port Based Routing (PBR) Fabrics. A typical example for 2 hosts
> connected to a common switch looks something like:
>
> ________________ _______________
> | | | | Hosts - each sees
> | HOST A | | HOST B | a PCIe style tree
> | | | | but from a fabric
> config
> | |Root Port| | | |Root Port| | point of view it's more
> -------|-------- -------|------- complex.
> | |
> | |
> _______|______________________________|________
> | USP (SW-CCI) USP | Switch can have lots of
> | | | | Upstream Ports. Each one
> | ____|________ _______|______ | has a virtual hierarchy.
> | | | | | |
> | vPPB vPPB vPPB vPPB| There are virtual
> | x | | | | "downstream
> ports."(vPPBs)
> | \ / / | That can be bound to real
> | \ / / | downstream ports.
> | \ / / |
> | \ / / | Multi Logical Devices are
> | DSP0 DSP1 DSP 2 | support more than one
> vPPB
> ------------------------------------------------ bound to a single
> physical
> | | | DSP (transactions are
> tagged
> | | | with an LD-ID)
> SLD0 MLD0 SLD1
>
> Some typical fabric management activities:
> 1) Bind/Unbind vPPB to physical DSP (Results in hotplug / unplug events)
> 2) Access config space or BAR space of End Points below the switch.
> 3) Tunneling messages through to devices downstream (e.g Dynamic Capacity
> Forced Remove that will blow away some memory even if a host is using
> it).
> 4) Non destructive stuff like status read back.
>
> Given the hosts may be using the Type 3 hosted memory (either Single
> Logical
> Device - SLD, or an LD on a Multi logical Device - MLD) as normal memory,
> unbinding a device in use can result in the memory access from a
> different host being removed. The 'blast radius' is perhaps a rack of
> servers. This discussion applies equally to FM-API commands sent to Multi
> Head Devices (see CXL r3.1).
>
> The Fabric Management actions are done using the CXL spec defined Fabric
> Management API, (FM-API) which is transported over various means including
> OoB MCTP over your favourite transport (I2C, PCIe-VDM...) or via normal
> PCIe read/write to a Switch-CCI. A Switch-CCI is mailbox in PCI BAR
> space on a function found alongside one of the switch upstream ports;
> this mailbox is very similar to the MMPT definition found in PCIe r6.2.
>
> In many cases this switch CCI / MCTP connection is used by a BMC rather
> than a normal host, but there have been some questions raised about whether
> a general purpose server OS would have a valid reason to use this interface
> (beyond debug and testing) to configure the switch or an MHD.
>
> If people have a use case for this, please reply to this thread to give
> more details.
>
> The most recently posted CXL Switch-CCI support only provided the RAW CXL
> command IOCTL interface that is already available for Type 3 memory
> devices.
> That allows for unfettered control of the switch but, because it is
> extremely easy to shoot yourself in the foot and cause unsolvable bug
> reports,
> it taints the kernel. There have been several requests to provide this
> interface
> without the taint for these switch configuration mailboxes.
>
> Last posted series:
>
> https://lore.kernel.org/all/20231016125323.18318-1-Jonathan.Cameron@huawei.com/
> Note there are unrelated reasons why that code hasn't been updated since
> v6.6 time,
> but I am planning to get back to it shortly.
>
> Similar issues will occur for other uses of PCIe MMPT (new mailbox in PCI
> that
> sometimes is used for similarly destructive activity such as PLDM based
> firmware update).
>
>
> On to the proposed rules:
>
> 1) Kernel space use of the various mailboxes, or filtered controls from
> user space.
>
> ==================================================================================
>
> Absolutely fine - no one worries about this, but the mediated traffic will
> be filtered for potentially destructive side effects. E.g. it will reject
> attempts to change anything routing related if the kernel either knows a
> host is
> using memory that will be blown away, or has no way to know (so affecting
> routing to another host). This includes blocking 'all' vendor defined
> messages as we have no idea what the do. Note this means the kernel has
> an allow list and new commands are not initially allowed.
>
> This isn't currently enabled for Switch CCIs because they are only really
> interesting if the potentially destructive stuff is available (an earlier
> version did enable query commands, but it wasn't particularly useful to
> know what your switch could do but not be allowed to do any of it).
> If you take a MMPT usecase of PLDM firmware update, the filtering would
> check that the device was in a state where a firmware update won't rip
> memory out from under a host, which would be messy if that host is
> doing the update.
>
> 2) Unfiltered userspace use of mailbox for Fabric Management - BMC kernels
> ==========================================================================
>
> (This would just be a kernel option that we'd advise normal server
> distributions not to turn on. Would be enabled by openBMC etc)
>
> This is fine - there is some work to do, but the switch-cci PCI driver
> will hopefully be ready for upstream merge soon. There is no filtering of
> accesses. Think of this as similar to all the damage you can do via
> MCTP from a BMC. Similarly it is likely that much of the complexity
> of the actual commands will be left to user space tooling:
> https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
>
> Whether Kconfig help text is strong enough to ensure this only gets
> enabled for BMC targeted distros is an open question we can address
> alongside an updated patch set.
>
> (On to the one that the "debate" is about)
>
> 3) Unfiltered user space use of mailbox for Fabric Management - Distro
> kernels
>
> =============================================================================
> (General purpose Linux Server Distro (Redhat, Suse etc))
>
> This is equivalent of RAW command support on CXL Type 3 memory devices.
> You can enable those in a distro kernel build despite the scary config
> help text, but if you use it the kernel is tainted. The result
> of the taint is to add a flag to bug reports and print a big message to say
> that you've used a feature that might result in you shooting yourself
> in the foot.
>
> The taint is there because software is not at first written to deal with
> everything that can happen smoothly (e.g. surprise removal) It's hard
> to survive some of these events, so is never on the initial feature list
> for any bus, so this flag is just to indicate we have entered a world
> where almost all bets are off wrt to stability. We might not know what
> a command does so we can't assess the impact (and no one trusts vendor
> commands to report affects right in the Command Effects Log - which
> in theory tells you if a command can result problems).
>
> A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
> (a r3.1 feature) but, as I understand it, these are intended for a
> host to configure and should not have side effects on other hosts?
> My working assumption is that the kernel driver stack will handle
> these (once we catch up with the current feature backlog!) Currently
> we have no visibility of what the OS driver stack for a fabrics will
> actually look like - the spec is just the starting point for that.
> (patches welcome ;)
>
> The various CXL upstream developers and maintainers may have
> differing views of course, but my current understanding is we want
> to support 1 and 2, but are very resistant to 3!
>
> General Notes
> =============
>
> One side aspect of why we really don't like unfiltered userspace access to
> any
> of these devices is that people start building non standard hacks in and we
> lose the ecosystem advantages. Forcing a considered discussion + patches
> to let a particular command be supported, drives standardization.
>
>
> https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
> provides some history on vendor specific extensions and why in general we
> won't support them upstream.
>
> To address another question raised in an earlier discussion:
> Putting these Fabric Management interfaces behind guard rails of some type
> (e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
> of non standard interfaces, because we will be even less likely to accept
> those upstream!
>
> If anyone needs more details on any aspect of this please ask.
> There are a lot of things involved and I've only tried to give a fairly
> minimal illustration to drive the discussion. I may well have missed
> something crucial.
>
> Jonathan
>
>
--
This electronic communication and the information and any files transmitted
with it, or attached to it, are confidential and are intended solely for
the use of the individual or entity to whom it is addressed and may contain
information that is confidential, legally privileged, protected by privacy
laws, or otherwise restricted from disclosure to anyone else. If you are
not the intended recipient or the person responsible for delivering the
e-mail to the intended recipient, you are hereby notified that any use,
copying, distributing, dissemination, forwarding, printing, or copying of
this e-mail is strictly prohibited. If you received this e-mail in error,
please return the e-mail to the sender, delete it from your computer, and
destroy any printed copy of it.
[-- Attachment #1.2: Type: text/html, Size: 14975 bytes --]
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4230 bytes --]
^ permalink raw reply
* RFC: Restricting userspace interfaces for CXL fabric management
From: Jonathan Cameron @ 2024-03-21 17:44 UTC (permalink / raw)
To: linux-cxl
Cc: Sreenivas Bagalkote, Brett Henning, Harold Johnson,
Sumanesh Samanta, Williams, Dan J, linux-kernel, Davidlohr Bueso,
Dave Jiang, Alison Schofield, Vishal Verma, Ira Weiny, linuxarm,
linux-api, Lorenzo Pieralisi, Natu, Mahesh
Hi All,
This is has come up in a number of discussions both on list and in private,
so I wanted to lay out a potential set of rules when deciding whether or not
to provide a user space interface for a particular feature of CXL Fabric
Management. The intent is to drive discussion, not to simply tell people
a set of rules. I've brought this to the public lists as it's a Linux kernel
policy discussion, not a standards one.
Whilst I'm writing the RFC this my attempt to summarize a possible
position rather than necessarily being my personal view.
It's a straw man - shoot at it!
Not everyone in this discussion is familiar with relevant kernel or CXL concepts
so I've provided more info than I normally would.
First some background:
======================
CXL has two different types of Fabric. The comments here refer to both, but
for now the kernel stack is focused on the simpler VCS fabric, not the more
recent Port Based Routing (PBR) Fabrics. A typical example for 2 hosts
connected to a common switch looks something like:
________________ _______________
| | | | Hosts - each sees
| HOST A | | HOST B | a PCIe style tree
| | | | but from a fabric config
| |Root Port| | | |Root Port| | point of view it's more
-------|-------- -------|------- complex.
| |
| |
_______|______________________________|________
| USP (SW-CCI) USP | Switch can have lots of
| | | | Upstream Ports. Each one
| ____|________ _______|______ | has a virtual hierarchy.
| | | | | |
| vPPB vPPB vPPB vPPB| There are virtual
| x | | | | "downstream ports."(vPPBs)
| \ / / | That can be bound to real
| \ / / | downstream ports.
| \ / / |
| \ / / | Multi Logical Devices are
| DSP0 DSP1 DSP 2 | support more than one vPPB
------------------------------------------------ bound to a single physical
| | | DSP (transactions are tagged
| | | with an LD-ID)
SLD0 MLD0 SLD1
Some typical fabric management activities:
1) Bind/Unbind vPPB to physical DSP (Results in hotplug / unplug events)
2) Access config space or BAR space of End Points below the switch.
3) Tunneling messages through to devices downstream (e.g Dynamic Capacity
Forced Remove that will blow away some memory even if a host is using it).
4) Non destructive stuff like status read back.
Given the hosts may be using the Type 3 hosted memory (either Single Logical
Device - SLD, or an LD on a Multi logical Device - MLD) as normal memory,
unbinding a device in use can result in the memory access from a
different host being removed. The 'blast radius' is perhaps a rack of
servers. This discussion applies equally to FM-API commands sent to Multi
Head Devices (see CXL r3.1).
The Fabric Management actions are done using the CXL spec defined Fabric
Management API, (FM-API) which is transported over various means including
OoB MCTP over your favourite transport (I2C, PCIe-VDM...) or via normal
PCIe read/write to a Switch-CCI. A Switch-CCI is mailbox in PCI BAR
space on a function found alongside one of the switch upstream ports;
this mailbox is very similar to the MMPT definition found in PCIe r6.2.
In many cases this switch CCI / MCTP connection is used by a BMC rather
than a normal host, but there have been some questions raised about whether
a general purpose server OS would have a valid reason to use this interface
(beyond debug and testing) to configure the switch or an MHD.
If people have a use case for this, please reply to this thread to give
more details.
The most recently posted CXL Switch-CCI support only provided the RAW CXL
command IOCTL interface that is already available for Type 3 memory devices.
That allows for unfettered control of the switch but, because it is
extremely easy to shoot yourself in the foot and cause unsolvable bug reports,
it taints the kernel. There have been several requests to provide this interface
without the taint for these switch configuration mailboxes.
Last posted series:
https://lore.kernel.org/all/20231016125323.18318-1-Jonathan.Cameron@huawei.com/
Note there are unrelated reasons why that code hasn't been updated since v6.6 time,
but I am planning to get back to it shortly.
Similar issues will occur for other uses of PCIe MMPT (new mailbox in PCI that
sometimes is used for similarly destructive activity such as PLDM based
firmware update).
On to the proposed rules:
1) Kernel space use of the various mailboxes, or filtered controls from user space.
==================================================================================
Absolutely fine - no one worries about this, but the mediated traffic will
be filtered for potentially destructive side effects. E.g. it will reject
attempts to change anything routing related if the kernel either knows a host is
using memory that will be blown away, or has no way to know (so affecting
routing to another host). This includes blocking 'all' vendor defined
messages as we have no idea what the do. Note this means the kernel has
an allow list and new commands are not initially allowed.
This isn't currently enabled for Switch CCIs because they are only really
interesting if the potentially destructive stuff is available (an earlier
version did enable query commands, but it wasn't particularly useful to
know what your switch could do but not be allowed to do any of it).
If you take a MMPT usecase of PLDM firmware update, the filtering would
check that the device was in a state where a firmware update won't rip
memory out from under a host, which would be messy if that host is
doing the update.
2) Unfiltered userspace use of mailbox for Fabric Management - BMC kernels
==========================================================================
(This would just be a kernel option that we'd advise normal server
distributions not to turn on. Would be enabled by openBMC etc)
This is fine - there is some work to do, but the switch-cci PCI driver
will hopefully be ready for upstream merge soon. There is no filtering of
accesses. Think of this as similar to all the damage you can do via
MCTP from a BMC. Similarly it is likely that much of the complexity
of the actual commands will be left to user space tooling:
https://gitlab.com/jic23/cxl-fmapi-tests has some test examples.
Whether Kconfig help text is strong enough to ensure this only gets
enabled for BMC targeted distros is an open question we can address
alongside an updated patch set.
(On to the one that the "debate" is about)
3) Unfiltered user space use of mailbox for Fabric Management - Distro kernels
=============================================================================
(General purpose Linux Server Distro (Redhat, Suse etc))
This is equivalent of RAW command support on CXL Type 3 memory devices.
You can enable those in a distro kernel build despite the scary config
help text, but if you use it the kernel is tainted. The result
of the taint is to add a flag to bug reports and print a big message to say
that you've used a feature that might result in you shooting yourself
in the foot.
The taint is there because software is not at first written to deal with
everything that can happen smoothly (e.g. surprise removal) It's hard
to survive some of these events, so is never on the initial feature list
for any bus, so this flag is just to indicate we have entered a world
where almost all bets are off wrt to stability. We might not know what
a command does so we can't assess the impact (and no one trusts vendor
commands to report affects right in the Command Effects Log - which
in theory tells you if a command can result problems).
A concern was raised about GAE/FAST/LDST tables for CXL Fabrics
(a r3.1 feature) but, as I understand it, these are intended for a
host to configure and should not have side effects on other hosts?
My working assumption is that the kernel driver stack will handle
these (once we catch up with the current feature backlog!) Currently
we have no visibility of what the OS driver stack for a fabrics will
actually look like - the spec is just the starting point for that.
(patches welcome ;)
The various CXL upstream developers and maintainers may have
differing views of course, but my current understanding is we want
to support 1 and 2, but are very resistant to 3!
General Notes
=============
One side aspect of why we really don't like unfiltered userspace access to any
of these devices is that people start building non standard hacks in and we
lose the ecosystem advantages. Forcing a considered discussion + patches
to let a particular command be supported, drives standardization.
https://lore.kernel.org/linux-cxl/CAPcyv4gDShAYih5iWabKg_eTHhuHm54vEAei8ZkcmHnPp3B0cw@mail.gmail.com/
provides some history on vendor specific extensions and why in general we
won't support them upstream.
To address another question raised in an earlier discussion:
Putting these Fabric Management interfaces behind guard rails of some type
(e.g. CONFIG_IM_A_BMC_AND_CAN_MAKE_A_MESS) does not encourage the risk
of non standard interfaces, because we will be even less likely to accept
those upstream!
If anyone needs more details on any aspect of this please ask.
There are a lot of things involved and I've only tried to give a fairly
minimal illustration to drive the discussion. I may well have missed
something crucial.
Jonathan
^ permalink raw reply
* Re: [PATCH v7] posix-timers: add clock_compare system call
From: Sagi Maimon @ 2024-03-20 14:42 UTC (permalink / raw)
To: Thomas Gleixner
Cc: richardcochran, luto, mingo, bp, dave.hansen, x86, hpa, arnd,
geert, peterz, hannes, sohil.mehta, rick.p.edgecombe, nphamcs,
palmer, keescook, legion, mark.rutland, mszeredi, casey, reibax,
davem, brauner, linux-kernel, linux-api, linux-arch, netdev
In-Reply-To: <874jd8n0ta.ffs@tglx>
On Thu, Mar 14, 2024 at 8:08 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Sagi!
>
> On Thu, Mar 14 2024 at 14:19, Sagi Maimon wrote:
> > On Thu, Mar 14, 2024 at 1:12 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >> On Thu, Mar 14 2024 at 11:05, Sagi Maimon wrote:
> >> > Some user space applications need to read a couple of different clocks.
> >> > Each read requires moving from user space to kernel space.
> >> > Reading each clock separately (syscall) introduces extra
> >> > unpredictable/unmeasurable delay. Minimizing this delay contributes to user
> >> > space actions on these clocks (e.g. synchronization etc).
> >>
> >> I asked for a proper description of the actual problem several times now
> >> and you still provide some handwaving blurb. Feel free to ignore me, but
> >> then please don't be surprised if I ignore you too.
> >>
> > Nobody is ignoring your notes, and I address any notes given by any
> > maintainer in the most serious way.
> > As far as explaining the actual problem this is the best that I can,
> > but let me try to explain better:
> > We did many tests with different CPU loading and compared sampling the
> > same clock twice,
> > once in user space and once by using the system call.
> > We have noticed an improvement up to hundreds of nanoseconds while
> > using the system call.
> > Those results improved our ability to sync different PHCs
>
> So let me express how I understand the problem - as far as I decoded it
> from your writeups:
>
> Synchronizing two PHCs requires to read timestamps from both and
> correlate them. Currently this requires several seperate system calls.
> This is subject to unmeasurable delays due to system call overhead,
> preemption and interrupts which makes the correlation imprecise.
>
> Therefore you want a system call, which samples two clocks at once, to
> make the correlation more precise.
>
> Right? For the further comments I assume this is what you are trying to
> say and to solve
You are right.
>
> So far so good, except that I do not agree with that reasoning at all:
>
> 1. The delays are measurable and as precise as the cross time stamp
> mechanism (hardware or software based) allows.
>
Most of the PHCs do not support crosstime stamps.
> 2. The system call overhead is completely irrelevant.
>
You are right in case of long preemption, but in other cases it is relevant.
> 3. The time deltas between the sample points are irrelevant within a
> reasonable upper bound to the time delta between the two outer
> sample points.
>
See below
> 4. The alledged higher precision is based on a guesstimate and not on
> correctness. Just because it behaves slightly better in testing
> does not make it any more correct.
>
See below
> 5. The problem can be solved with maximal possible accuracy by using
> the existing PTP IOCTLs.
>
> See below.
>
> >> Also why does reading two random clocks make any sense at all? Your code
> >> allows to read CLOCK_TAI and CLOCK_THREAD_CPUTIME_ID. What for?
> >>
> > Initially we needed to sync some different PHCs for our user space
> > application, that is why we came with this idea.
> > The first idea was an IOCTL that returned samples of several PHCs for
> > the need of synchronization.
> > Richard Cochran suggested a system call instead, which will add the
> > ability to get various system clocks, while this
> > implementation is more complex then IOCTL, I think that he was right,
> > for future usage.
>
> Which future usage? We are not introducing swiss army knife interfaces
> just because there might be an illusional use case somewhere in the
> unspecified future.
>
> Adding a system call needs a proper design and justification. Handwaving
> future usage is not enough.
>
> Documentation/process/adding-syscalls.rst is very clear about what is
> required for a new system call.
>
> >> This needs to be split up into:
> >>
> >> 1) Infrastructure in posix-timers.c
> >> 2) Wire up the syscall in x86
> >> 3) Infrastructure in posix-clock.c
> >> 4) Usage in ptp_clock.c
> >>
> >> and not as a big lump of everything.
> >>
> > I know, but I think the benefit worth it
>
> It's worth it because it makes review easier. It's well documented in
> the process documentation that patches should do one thing and not a
> whole lump of changes.
>
No problem it will be split into two different patches.
> >> > + if (!error) {
> >> > + if (clock_b == CLOCK_MONOTONIC_RAW) {
> >> > + ts_b = ktime_to_timespec64(xtstamp_a1.sys_monoraw);
> >> > + ts_a1 = ktime_to_timespec64(xtstamp_a1.device);
> >> > + goto out;
> >> > + } else if (clock_b == CLOCK_REALTIME) {
> >> > + ts_b = ktime_to_timespec64(xtstamp_a1.sys_realtime);
> >> > + ts_a1 = ktime_to_timespec64(xtstamp_a1.device);
> >> > + goto out;
> >> > + } else {
> >> > + crosstime_support_a = true;
> >>
> >> Right. If clock_b is anything else than CLOCK_MONOTONIC_RAW or
> >> CLOCK_REALTIME then this is true.
> >>
> >> > + }
> >> > + }
> >>
> >> So in case of an error, this just keeps going with an uninitialized
> >> xtstamp_a1 and if the clock_b part succeeds it continues and operates on
> >> garbage.
> >>
> > On error xtstamp_a1 will be taken again using clock_get_crosstimespec
> > so no one will be operating on garbage.
>
> It will not, because crosstime_support_a == false. It will therefore
> fall back to kc_a->clock_get_timespec(), no?
>
> Sorry, I misread the code vs. using the uninitialized value, but this is
> just unneccesary hard to follow.
>
> >> > + if (crosstime_support_a)
> >> > + error = kc_a->clock_get_crosstimespec(clock_a, &xtstamp_a2);
> >> > + else
> >> > + error = kc_a->clock_get_timespec(clock_a, &ts_a2);
> >> > +
> >> > + if (error)
> >> > + return error;
> >>
> >> The logic and the code flow here are unreadable garbage and there are
> >> zero comments what this is supposed to do.
> >>
> > I will add comments.
> > please no need to use negative words like "garbage" (not the first time),
> > please keep it professional and civilized.
>
> Let me rephrase:
>
> The code and the logic is incomprehensible unless I waste an unjustified
> amount of time to decode it. Sorry, I don't have that time.
>
> >> > + if (crosstime_support_a) {
> >> > + ktime_a = ktime_sub(xtstamp_a2.device, xtstamp_a1.device);
> >> > + ts_offs_err = ktime_divns(ktime_a, 2);
> >> > + ktime_a = ktime_add_ns(xtstamp_a1.device, (u64)ts_offs_err);
> >> > + ts_a1 = ktime_to_timespec64(ktime_a);
> >>
> >> This is just wrong.
> >>
> >> read(a1);
> >> read(b);
> >> read(a2);
> >>
> >> You _CANNOT_ assume that (a1 + ((a2 - a1) / 2) is anywhere close to the
> >> point in time where 'b' is read. This code is preemtible and
> >> interruptible. I explained this to you before.
> >>
> >> Your explanation in the comment above the function is just wishful
> >> thinking.
> >>
> > you explained it before, but still it is better then two consecutive
> > user space calls which are also preemptible
> > and the userspace to kernel context switch time is added.
>
> It might be marginally better, but it is still just _pretending_ that it
> does the right thing, is correct and better than the existing IOCTLs.
>
> If your user space implementation has the same algorithm, then I'm
> absolutely not surprised that the results are not useful. Why?
>
> You simply cannot use the midpoint of the outer samples if you want to
> have precise results if there is no guarantee that b was sampled exactly
> in the midpoint of a1 and a2. A hardware implementation might give that
> guarantee, but the kernel cannot.
>
> But why using the midpoint in the first place?
>
> There is absolutely no reason to do so because the sampling points a1, b
> and a2 can be precisely determined with the precision of the cross time
> stamp mechanism, which is best with a hardware based cross time stamp
> obviously.
>
> The whole point of ptp::info::getcrosststamp() is to get properly
> correlated clock samples of
>
> 1) PHC clock
> 2) CLOCK_MONOTONIC_RAW
> 3) CLOCK_REALTIME
>
> So if you take 3 samples:
>
> get_cross_timestamp(a1);
> get_cross_timestamp(b);
> get_cross_timestamp(a2);
>
> then each of them provides:
>
> - device time
> - correlated CLOCK_MONOTONIC_RAW
> - correlated CLOCK_REALTIME
>
> Ergo the obvious thing to do is:
>
> d1 = b.sys_monoraw - a1.sys_monoraw;
> d2 = a2.sys_monoraw - a1.sys_monoraw;
>
> tsa = a1.device + ((a2.device - a1.device) * d1) / d2;
>
> Which is maximaly precise under the assumption that in the time between
> the sample points a1 and a2 neither the system clock nor the PCH clocks
> are changing their frequency significantly. That is a valid assumption
> when you put a reasonable upper bound on d2.
>
You are right.
In fact, we are running this calculation on a user space application.
We use the new system call to get pairs of mono and PHC and then run
that calculation in user space.
That is why the system call returns pairs of clock samples and not the
diff between them.
> Even when the device does not implement getcrosststamp() then loop based
> sampling like it is implemented in the PTP_SYS_OFFSET[_EXTENDED] IOTCLs
> is providing reasonably accurate results to the extent possible.
>
> Your algorithm is imprecise by definition and you can apply as much
> testing as you want, it won't become magically correct. It's still a
> guesstimate, i.e. an estimate made without using adequate or complete
> information.
>
> Now why a new syscall?
>
> This can be done from user space with existing interfaces and the very
> same precision today:
>
> ioctl(fda, PTP_SYS_OFFSET*, &a1);
> ioctl(fdb, PTP_SYS_OFFSET*, &b);
> ioctl(fda, PTP_SYS_OFFSET*, &a2);
>
> u64 d1 = timespec_delta_ns(b.sys_monoraw, a1.sys_monoraw);
> u64 d2 = timespec_delta_ns(a2.sys_monoraw, a1.sys_monoraw);
> u64 td = (timespec_delta_ns(a2.device, a1.device) * d1) / d2
>
> tsa = timespec_add_ns(a1.device, td);
> tsb = b.device;
>
> with the extra benefit of:
>
> 1) The correct CLOCK_REALTIME at that sample point,
> i.e. b.sys_realtime
>
> 2) The correct CLOCK_MONOTONIC_RAW at that sample point,
> i.e. b.sys_monoraw
>
If PTP_SYS_OFFSET IOCTL returns sys_monoraw, then you are right, but
unfortunately the only IOCTL that returns sys_monoraw is
PTP_SYS_OFFSET_PRECISE (getcrosststamp)
And most of the drivers does not support it.
> It works with PTP_SYS_OFFSET_PRECISE and PTP_SYS_OFFSET[_EXTENDED], with
> the obvious limitations of PTP_SYS_OFFSET[_EXTENDED], which are still
> vastly superior to your proposed (a2 - a1) / 2 guestimate which is just
> reading the PCH clocks with clock_get_timespec().
>
It only works with PTP_SYS_OFFSET_PRECISE (which most of the NIC
drivers does not support and I take it under consideration in my
system call),
PTP_SYS_OFFSET_EXTENDED ioctl returns system time before, PHC, system
time after , and no monotic raw.
> It is completely independent of the load, the syscall overhead and the
> actual time delta between the sample points when you apply a reasonable
> upper bound for d2, i.e. the time delta between the sample points a1 and
> a2 to eliminate the issue that system clock and/or the PCH clocks change
> their frequency significantly during that time. You'd need to do that in
> the kernel too.
>
> The actual frequency difference between PCH A and system clock is
> completely irrelevant when the frequencies of both are stable accross
> the sample period.
>
> You might still argue that the time delta between the sample points a1
> and a2 matters and is slightly shorter in the kernel, but that is a
> non-argument because:
>
> 1) The kernel implementation does not guarantee atomicity of the
> consecutive samples either. The time delta is just statistically
> better, which is obviously useless when you want to make
> guarantees.
>
> 2) It does not matter when the time delta is slightly larger because
> you need a large frequency change of the involved clocks in the
> sample interval between the sample points a1 and a2 to make an
> actual difference in the resulting accuracy.
>
> A typical temperature drift of a high quality cyrstal is less than
> 1ppm per degree Celsius and even if you assume that the overall
> system drift is 10ppm per degree Celsius then still the actual
> error for a bound time delta between the sample points a1 and a2 is
> just somewhere in the irrelevant noise, unless you manage to blow
> torch or ice spray your crystals during the sample interval.
>
> If your clocks are not stable enough then nothing can cure it and
> you cannot do high precision timekeeping with them.
>
You are right in case of long preemption (which still the system call
is better), but in other cases it is relevant.
> So what is your new syscall solving that can't be done with the existing
> IOCTLs other than providing worse precision results based on
> guesstimates and some handwavy future use for random clock ids?
>
> Nothing as far as I can tell, but I might be missing something important
> here.
>
Few points to consider:
1) Most of the PHCs do not support cross time stamping.
2) Users can implement your suggested code in user space while using
the new system call to get pairs of mono and PHC
. This is what we did already in user space.
3) User with less tight requirement will benefit high accuracy with
the new system call
> Thanks,
>
> tglx
> ---
> arch/x86/kernel/tsc.c:119: "Math is hard, let's go shopping." - John Stultz
^ permalink raw reply
* Re: [RFC v3 0/3] move_phys_pages syscall - migrate page contents given
From: Huang, Ying @ 2024-03-20 6:01 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, dan.j.williams, honggyu.kim, corbet, arnd, luto,
akpm, shuah
In-Reply-To: <Zfpohg3EGxxOEcWg@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Wed, Mar 20, 2024 at 10:48:44AM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>>
>> > Doing this reverse-translation outside of the kernel requires considerable
>> > space and compute, and it will have to be performed again by the existing
>> > system calls. Much of this work can be avoided if the pages can be
>> > migrated directly with physical memory addressing.
>>
>> One difficulty of the idea of the physical address is that we lacks some
>> user space specified policy information to make decision. For example,
>> users may want to pin some pages in DRAM to improve latency, or pin some
>> pages in CXL memory to do some best effort work. To make the correct
>> decision, we need PID and virtual address.
>>
>
> I think of this as a second or third order problem. The core problem
> right now isn't the practicality of how userland would actually use this
> interface - the core problem is whether the data generated by offloaded
> monitoring is even worth collecting and operating on in the first place.
>
> So this is a quick hack to do some research about whether it's even
> worth developing the whole abstraction described by Willy.
>
> This is why it's labeled RFC. I upped a v3 because I know of two groups
> actively looking at using it for research, and because the folio updates
> broke the old version. It's also easier for me to engage through the
> list than via private channels for this particular work.
>
>
> Do I suggest we merge this interface as-is? No, too many concerns about
> side channels. However, it's a clean reuse of move_pages code to
> bootstrap the investigation, and it at least gets the gears turning.
Got it! Thanks for detailed explanation.
I think that one of the difficulties of offloaded monitoring is that
it's hard to obey these user specified policies. The policies may
become more complex in the future, for example, allocate DRAM among
workloads.
> Example notes from a sidebar earlier today:
>
> * An interesting proposal from Dan Williams would be to provide some
> sort of `/sys/.../memory_tiering/tierN/promote_hot` interface, with
> a callback mechanism into the relevant hardware drivers that allows
> for this to be abstracted. This could be done on some interval and
> some threshhold (# pages, hotness threshhold, etc).
>
>
> The code to execute promotions ends up looking like what I have now
>
> 1) Validate the page is elgibile to be promoted by walking the vmas
> 2) invoking the existing move_pages code
>
> The above idea can be implemented trivially in userland without
> having to plumb through a whole brand new callback system.
>
>
> Sometimes you have to post stupid ideas to get to the good ones :]
>
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [RFC v3 0/3] move_phys_pages syscall - migrate page contents given
From: Gregory Price @ 2024-03-20 4:39 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, dan.j.williams, honggyu.kim, corbet, arnd, luto,
akpm, shuah
In-Reply-To: <87v85hsjn7.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Wed, Mar 20, 2024 at 10:48:44AM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
>
> > Doing this reverse-translation outside of the kernel requires considerable
> > space and compute, and it will have to be performed again by the existing
> > system calls. Much of this work can be avoided if the pages can be
> > migrated directly with physical memory addressing.
>
> One difficulty of the idea of the physical address is that we lacks some
> user space specified policy information to make decision. For example,
> users may want to pin some pages in DRAM to improve latency, or pin some
> pages in CXL memory to do some best effort work. To make the correct
> decision, we need PID and virtual address.
>
I think of this as a second or third order problem. The core problem
right now isn't the practicality of how userland would actually use this
interface - the core problem is whether the data generated by offloaded
monitoring is even worth collecting and operating on in the first place.
So this is a quick hack to do some research about whether it's even
worth developing the whole abstraction described by Willy.
This is why it's labeled RFC. I upped a v3 because I know of two groups
actively looking at using it for research, and because the folio updates
broke the old version. It's also easier for me to engage through the
list than via private channels for this particular work.
Do I suggest we merge this interface as-is? No, too many concerns about
side channels. However, it's a clean reuse of move_pages code to
bootstrap the investigation, and it at least gets the gears turning.
Example notes from a sidebar earlier today:
* An interesting proposal from Dan Williams would be to provide some
sort of `/sys/.../memory_tiering/tierN/promote_hot` interface, with
a callback mechanism into the relevant hardware drivers that allows
for this to be abstracted. This could be done on some interval and
some threshhold (# pages, hotness threshhold, etc).
The code to execute promotions ends up looking like what I have now
1) Validate the page is elgibile to be promoted by walking the vmas
2) invoking the existing move_pages code
The above idea can be implemented trivially in userland without
having to plumb through a whole brand new callback system.
Sometimes you have to post stupid ideas to get to the good ones :]
~Gregory
^ permalink raw reply
* Re: [RFC v3 0/3] move_phys_pages syscall - migrate page contents given
From: Huang, Ying @ 2024-03-20 2:48 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-api, linux-arch, linux-kselftest, linux-kernel,
dan.j.williams, honggyu.kim, corbet, arnd, luto, akpm, shuah,
Gregory Price
In-Reply-To: <20240319172609.332900-1-gregory.price@memverge.com>
Gregory Price <gourry.memverge@gmail.com> writes:
> v3:
> - pull forward to v6.8
> - style and small fixups recommended by jcameron
> - update syscall number (will do all archs when RFC tag drops)
> - update for new folio code
> - added OCP link to device-tracked address hotness proposal
> - kept void* over __u64 simply because it integrates cleanly with
> existing migration code. If there's strong opinions, I can refactor.
>
> This patch set is a proposal for a syscall analogous to move_pages,
> that migrates pages between NUMA nodes using physical addressing.
>
> The intent is to better enable user-land system-wide memory tiering
> as CXL devices begin to provide memory resources on the PCIe bus.
>
> For example, user-land software which is making decisions based on
> data sources which expose physical address information no longer
> must convert that information to virtual addressing to act upon it
> (see background for info on how physical addresses are acquired).
>
> The syscall requires CAP_SYS_ADMIN, since physical address source
> information is typically protected by the same (or CAP_SYS_NICE).
>
> This patch set broken into 3 patches:
> 1) refactor of existing migration code for code reuse
> 2) The sys_move_phys_pages system call.
> 3) ktest of the syscall
>
> The sys_move_phys_pages system call validates the page may be
> migrated by checking migratable-status of each vma mapping the page,
> and the intersection of cpuset policies each vma's task.
>
>
> Background:
>
> Userspace job schedulers, memory managers, and tiering software
> solutions depend on page migration syscalls to reallocate resources
> across NUMA nodes. Currently, these calls enable movement of memory
> associated with a specific PID. Moves can be requested in coarse,
> process-sized strokes (as with migrate_pages), and on specific virtual
> pages (via move_pages).
>
> However, a number of profiling mechanisms provide system-wide information
> that would benefit from a physical-addressing version move_pages.
>
> There are presently at least 4 ways userland can acquire physical
> address information for use with this interface, and 1 hardware offload
> mechanism being proposed by opencompute.
>
> 1) /proc/pid/pagemap: can be used to do page table translations.
> This is only really useful for testing, and the ktest was
> written using this functionality.
>
> 2) X86: IBS (AMD) and PEBS (Intel) can be configured to return physical
> and/or vitual address information.
>
> 3) zoneinfo: /proc/zoneinfo exposes the start PFN of zones
>
> 4) /sys/kernel/mm/page_idle: A way to query whether a PFN is idle.
> So long as the page size is known, this can be used to identify
> system-wide idle pages that could be migrated to lower tiers.
>
> https://docs.kernel.org/admin-guide/mm/idle_page_tracking.html
>
> 5) CXL Offloaded Hotness Monitoring (Proposed): a CXL memory device
> may provide hot/cold information about its memory. For example,
> it may report the hottest device addresses (0-based) or a physical
> address (if it has access to decoders for convert bases).
>
> DPA can be cheaply converted to HPA by combining it with data
> exposed by /sys/bus/cxl/ information (region address bases).
>
> See: https://www.opencompute.org/documents/ocp-cms-hotness-tracking-requirements-white-paper-pdf-1
>
>
> Information from these sources facilitates systemwide resource management,
> but with the limitations of migrate_pages and move_pages applying to
> individual tasks, their outputs must be converted back to virtual addresses
> and re-associated with specific PIDs.
>
> Doing this reverse-translation outside of the kernel requires considerable
> space and compute, and it will have to be performed again by the existing
> system calls. Much of this work can be avoided if the pages can be
> migrated directly with physical memory addressing.
One difficulty of the idea of the physical address is that we lacks some
user space specified policy information to make decision. For example,
users may want to pin some pages in DRAM to improve latency, or pin some
pages in CXL memory to do some best effort work. To make the correct
decision, we need PID and virtual address.
Yes, I found that you have tried to avoid to break the existing policy
in the code. But it seems better to consider the policy beforehand to
avoid to make the wrong decision in the first place.
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktest
From: Gregory Price @ 2024-03-19 18:50 UTC (permalink / raw)
To: Matthew Wilcox
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, ying.huang, dan.j.williams, honggyu.kim, corbet,
arnd, luto, akpm, shuah
In-Reply-To: <Zfnbn8H4O9neZhcm@casper.infradead.org>
On Tue, Mar 19, 2024 at 06:38:23PM +0000, Matthew Wilcox wrote:
> > The syscall design is mostly being posted right now to collaborate via
> > public channels, but if the idea is so fundamentally offensive then i'll
> > drop it and relay the opinion accordingly.
>
> The syscall design is wrong. Exposing physical addresses to userspace
> is never the right answer. Think rowhammer.
>
1) The syscall does not expose physical addresses information, it
consumes it.
2) The syscall does not allow the user to select target physical address
only the target node. Now, that said, if source-pages are zeroed on
migration, that's definitely a concern. I did not see this to be the
case, however, and the frequency of write required to make use of
that for rowhammer seems to be a mitigating factor.
3) there exist 4 interfaces which do expose physical address information
- /proc/pid/pagemap
- perf / IBS and PEBs
- zoneinfo
- /sys/kerne/mm/page_idle (PFNs)
4) The syscall requires CAP_SYS_ADMIN because these other sources
require the same, though as v1/v2 discussed there could be an
argument for CAP_SYS_NIDE.
> I'm vehemently opposed to all of the bullshit around CXL. However, if you
> are going to propose something, it should be based around an abstraction.
> Say "We have 8 pools of memory. This VMA is backed by memory from pools
> 3 & 6. The relative hotness of the 8 pools are <vector>. The quantities
> of memory in the 8 ppols are <vector>". And then you can say "migrate
> this range of memory to pool 2".
>
> That's just an initial response to the idea. I refuse to invest a
> serious amount of time in a dead-end idea like CXL memory pooling.
Who said anything about pools? Local memory expanders are capable of
hosting hotness tracking offload.
~Gregory
^ permalink raw reply
* Re: [RFC v3 3/3] ktest: sys_move_phys_pages ktest
From: Matthew Wilcox @ 2024-03-19 18:38 UTC (permalink / raw)
To: Gregory Price
Cc: Gregory Price, linux-mm, linux-api, linux-arch, linux-kselftest,
linux-kernel, ying.huang, dan.j.williams, honggyu.kim, corbet,
arnd, luto, akpm, shuah
In-Reply-To: <ZfnaMa6x/O68ENsP@memverge.com>
On Tue, Mar 19, 2024 at 02:32:17PM -0400, Gregory Price wrote:
> On Tue, Mar 19, 2024 at 06:20:33PM +0000, Matthew Wilcox wrote:
> > On Tue, Mar 19, 2024 at 02:14:33PM -0400, Gregory Price wrote:
> > > On Tue, Mar 19, 2024 at 05:52:46PM +0000, Matthew Wilcox wrote:
> > > > On Tue, Mar 19, 2024 at 01:26:09PM -0400, Gregory Price wrote:
> > > > > Implement simple ktest that looks up the physical address via
> > > > > /proc/self/pagemap and migrates the page based on that information.
> > > >
> > > > What? LOL. No.
> > > >
> > >
> > > Certainly the test is stupid and requires admin, but I could not
> > > come up an easier test to demonstrate the concept - and the docs
> > > say to include a test with all syscall proposals.
> > >
> > > Am I missing something else important?
> > > (stupid question: of course I am, but alas I must ask it)
> >
> > It's not that the test is stupid. It's the concept that's stupid.
>
> Ok i'll bite.
>
> The 2 major ways page-hotness is detected right now is page-faults
> (induced or otherwise) and things like IBS/PEBS.
>
> page-faults cause overhead, and IBS/PEBS actually miss upwards of ~66%
> of all traffic (if you want the details i can dig up the presentation,
> but TL;DR: prefetcher traffic is missed entirely).
>
> so OCP folks have been proposing hotness-tracking offloaded to the
> memory devices themselves:
>
> https://www.opencompute.org/documents/ocp-cms-hotness-tracking-requirements-white-paper-pdf-1
>
> (it's come along further than this white paper, but i need to dig up
> the new information).
>
> These devices are incapable of providing virtual addressing information,
> and doing reverse lookups of addresses is inordinately expensive from
> user space. This leaves: Do it all in a kernel task, or give user space
> an an interface to operate on data provided by the device.
>
> The syscall design is mostly being posted right now to collaborate via
> public channels, but if the idea is so fundamentally offensive then i'll
> drop it and relay the opinion accordingly.
The syscall design is wrong. Exposing physical addresses to userspace
is never the right answer. Think rowhammer.
I'm vehemently opposed to all of the bullshit around CXL. However, if you
are going to propose something, it should be based around an abstraction.
Say "We have 8 pools of memory. This VMA is backed by memory from pools
3 & 6. The relative hotness of the 8 pools are <vector>. The quantities
of memory in the 8 ppols are <vector>". And then you can say "migrate
this range of memory to pool 2".
That's just an initial response to the idea. I refuse to invest a
serious amount of time in a dead-end idea like CXL memory pooling.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox