Re: [PATCH net-next 1/3] connector/cn_proc: Add hash table for threads

2024-10-12 Thread Simon Horman
On Fri, Oct 11, 2024 at 05:45:30PM -0700, Anjali Kulkarni wrote:
> Add a new type PROC_CN_MCAST_NOTIFY to proc connector API, which allows a
> thread to notify the kernel that it has exited abnormally. Thread can also
> send the exit status code it wants returned in the notification with it.
> Exiting thread can call this either when it wants to call pthread_exit()
> with non-zero value or from signal handler.
> 
> Add a new file cn_hash.c which implements a hash table storing the exit
> codes of abnormally exiting threads, received by the system call above.
> The key used for the hash table is the pid of the thread, so when the
> thread actually exits, we lookup it's pid in the hash table and retrieve
> the exit code sent by user. If the exit code in struct task is 0, we
> then replace it with the user supplied non-zero exit code.
> 
> cn_hash.c implements the hash table add, delete, lookup operations.
> mutex_lock() and mutex_unlock() operations are used to safeguard the
> integrity of the hash table while adding or deleting elements.
> connector.c has the API calls, called from cn_proc.c, as well as calls
> to allocate, initialize and free the hash table.
> 
> Add a new flag in PF_* flags of task_struct - EXIT_NOTIFY. This flag is
> set when user sends the exit code via PROC_CN_MCAST_NOTIFY. While
> exiting, this flag is checked and the hash table add or delete calls
> are only made if this flag is set.
> 
> A refcount field hrefcnt is added in struct cn_hash_dev, to keep track
> of number of threads which have added an entry in hash table. Before
> freeing the struct cn_hash_dev, this value must be 0.
> 
> Signed-off-by: Anjali Kulkarni 

...

> diff --git a/drivers/connector/cn_proc.c b/drivers/connector/cn_proc.c
> index 44b19e696176..8c6e002069d9 100644
> --- a/drivers/connector/cn_proc.c
> +++ b/drivers/connector/cn_proc.c

...

> @@ -326,9 +328,16 @@ void proc_exit_connector(struct task_struct *task)
>   struct proc_event *ev;
>   struct task_struct *parent;
>   __u8 buffer[CN_PROC_MSG_SIZE] __aligned(8);
> + __u32 uexit_code;
> + int err;
>  
> - if (atomic_read(&proc_event_num_listeners) < 1)
> + if (atomic_read(&proc_event_num_listeners) < 1) {
> + if (likely(!(task->flags & PF_EXIT_NOTIFY)))
> + return;
> +
> + err = cn_del_elem(task->pid);

Hi Anjali,

err is set but otherwise unused in this function; probably it can be removed.

>   return;
> + }
>  
>   msg = buffer_to_cn_msg(buffer);
>   ev = (struct proc_event *)msg->data;

...



[PATCH net-next 1/3] connector/cn_proc: Add hash table for threads

2024-10-11 Thread Anjali Kulkarni
Add a new type PROC_CN_MCAST_NOTIFY to proc connector API, which allows a
thread to notify the kernel that it has exited abnormally. Thread can also
send the exit status code it wants returned in the notification with it.
Exiting thread can call this either when it wants to call pthread_exit()
with non-zero value or from signal handler.

Add a new file cn_hash.c which implements a hash table storing the exit
codes of abnormally exiting threads, received by the system call above.
The key used for the hash table is the pid of the thread, so when the
thread actually exits, we lookup it's pid in the hash table and retrieve
the exit code sent by user. If the exit code in struct task is 0, we
then replace it with the user supplied non-zero exit code.

cn_hash.c implements the hash table add, delete, lookup operations.
mutex_lock() and mutex_unlock() operations are used to safeguard the
integrity of the hash table while adding or deleting elements.
connector.c has the API calls, called from cn_proc.c, as well as calls
to allocate, initialize and free the hash table.

Add a new flag in PF_* flags of task_struct - EXIT_NOTIFY. This flag is
set when user sends the exit code via PROC_CN_MCAST_NOTIFY. While
exiting, this flag is checked and the hash table add or delete calls
are only made if this flag is set.

A refcount field hrefcnt is added in struct cn_hash_dev, to keep track
of number of threads which have added an entry in hash table. Before
freeing the struct cn_hash_dev, this value must be 0.

Signed-off-by: Anjali Kulkarni 
---
 drivers/connector/Makefile|   2 +-
 drivers/connector/cn_hash.c   | 195 ++
 drivers/connector/cn_proc.c   |  59 +-
 drivers/connector/connector.c |  83 ++-
 include/linux/connector.h |  43 
 include/linux/sched.h |   2 +-
 include/uapi/linux/cn_proc.h  |   4 +-
 7 files changed, 379 insertions(+), 9 deletions(-)
 create mode 100644 drivers/connector/cn_hash.c

diff --git a/drivers/connector/Makefile b/drivers/connector/Makefile
index 1bf67d3df97d..cb1dcdf067ad 100644
--- a/drivers/connector/Makefile
+++ b/drivers/connector/Makefile
@@ -2,4 +2,4 @@
 obj-$(CONFIG_CONNECTOR)+= cn.o
 obj-$(CONFIG_PROC_EVENTS)  += cn_proc.o
 
-cn-y   += cn_queue.o connector.o
+cn-y   += cn_hash.o cn_queue.o connector.o
diff --git a/drivers/connector/cn_hash.c b/drivers/connector/cn_hash.c
new file mode 100644
index ..a0211cd99132
--- /dev/null
+++ b/drivers/connector/cn_hash.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Author: Anjali Kulkarni 
+ *
+ * Copyright (c) 2024 Oracle and/or its affiliates.
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+struct cn_hash_dev *cn_hash_alloc_dev(const char *name)
+{
+   struct cn_hash_dev *hdev;
+
+   hdev = kzalloc(sizeof(*hdev), GFP_KERNEL);
+   if (!hdev)
+   return NULL;
+
+   snprintf(hdev->name, sizeof(hdev->name), "%s", name);
+   atomic_set(&hdev->hrefcnt, 0);
+   mutex_init(&hdev->uexit_hash_lock);
+   hash_init(hdev->uexit_pid_htable);
+   return hdev;
+}
+
+void cn_hash_free_dev(struct cn_hash_dev *hdev)
+{
+   struct uexit_pid_hnode *hnode;
+   struct hlist_node *tmp;
+   int bucket;
+
+   pr_debug("%s: Freeing entire hdev %p\n", __func__, hdev);
+
+   mutex_lock(&hdev->uexit_hash_lock);
+   hash_for_each_safe(hdev->uexit_pid_htable, bucket, tmp,
+   hnode, uexit_pid_hlist) {
+   hash_del(&hnode->uexit_pid_hlist);
+   pr_debug("%s: Freeing node for pid %d\n",
+   __func__, hnode->pid);
+   kfree(hnode);
+   }
+
+   mutex_unlock(&hdev->uexit_hash_lock);
+   mutex_destroy(&hdev->uexit_hash_lock);
+
+   while (atomic_read(&hdev->hrefcnt)) {
+   pr_info("Waiting for %s to become free: refcnt=%d\n",
+   hdev->name, atomic_read(&hdev->hrefcnt));
+   msleep(1000);
+   }
+
+   kfree(hdev);
+}
+
+static struct uexit_pid_hnode *cn_hash_alloc_elem(__u32 uexit_code, pid_t pid)
+{
+   struct uexit_pid_hnode *elem;
+
+   elem = kzalloc(sizeof(*elem), GFP_KERNEL);
+   if (!elem)
+   return NULL;
+
+   INIT_HLIST_NODE(&elem->uexit_pid_hlist);
+   elem->uexit_code = uexit_code;
+   elem->pid = pid;
+   return elem;
+}
+
+void cn_hash_free_elem(struct uexit_pid_hnode *elem)
+{
+   kfree(elem);
+}
+
+int cn_hash_add_elem(struct cn_hash_dev *hdev, __u32 uexit_code, pid_t pid)
+{
+   struct uexit_pid_hnode *elem, *hnode;
+
+   elem = cn_hash_alloc_elem(uexit_code, pid);
+   if (!elem) {
+   pr_err("%s: cn_hash_alloc_elem() returned NULL pid %d\n",
+   __func__, pid);
+   return -ENOMEM;
+   }
+
+   mutex_lock(&hdev->uexit_hash_lock)