mbox series

[net-next,v1,0/3] Threads support in proc connector

Message ID 20241013170617.2139204-1-anjali.k.kulkarni@oracle.com (mailing list archive)
Headers show
Series Threads support in proc connector | expand

Message

Anjali Kulkarni Oct. 13, 2024, 5:06 p.m. UTC 
Recently we committed a fix to allow processes to receive notifications for
non-zero exits via the process connector module. Commit is a4c9a56e6a2c.

However, for threads, when it does a pthread_exit(&exit_status) call, the
kernel is not aware of the exit status with which pthread_exit is called.
It is sent by child thread to the parent process, if it is waiting in
pthread_join(). Hence, for a thread exiting abnormally, kernel cannot
send notifications to any listening processes.

The exception to this is if the thread is sent a signal which it has not
handled, and dies along with it's process as a result; for eg. SIGSEGV or
SIGKILL. In this case, kernel is aware of the non-zero exit and sends a
notification for it.

For our use case, we cannot have parent wait in pthread_join, one of the
main reasons for this being that we do not want to track normal
pthread_exit(), which could be a very large number. We only want to be
notified of any abnormal exits. Hence, threads are created with
pthread_attr_t set to PTHREAD_CREATE_DETACHED.

To fix this problem, we add a new type PROC_CN_MCAST_NOTIFY to proc connector
API, which allows a thread to send it's exit status to kernel either when
it needs to call pthread_exit() with non-zero value to indicate some
error or from signal handler before pthread_exit().

v->v1 changes:
- Handled comment by Simon Horman to remove unused err in cn_proc.c
- Handled comment by Simon Horman to make adata and key_display static
  in cn_hash_test.c

Anjali Kulkarni (3):
  connector/cn_proc: Add hash table for threads
  connector/cn_proc: Kunit tests for threads hash table
  connector/cn_proc: Selftest for threads

 drivers/connector/Makefile                    |   2 +-
 drivers/connector/cn_hash.c                   | 240 ++++++++++++++++++
 drivers/connector/cn_proc.c                   |  58 ++++-
 drivers/connector/connector.c                 |  96 ++++++-
 include/linux/connector.h                     |  47 ++++
 include/linux/sched.h                         |   2 +-
 include/uapi/linux/cn_proc.h                  |   4 +-
 lib/Kconfig.debug                             |  17 ++
 lib/Makefile                                  |   1 +
 lib/cn_hash_test.c                            | 167 ++++++++++++
 lib/cn_hash_test.h                            |  12 +
 tools/testing/selftests/connector/Makefile    |  23 +-
 .../testing/selftests/connector/proc_filter.c |   5 +
 tools/testing/selftests/connector/thread.c    |  90 +++++++
 .../selftests/connector/thread_filter.c       |  93 +++++++
 15 files changed, 847 insertions(+), 10 deletions(-)
 create mode 100644 drivers/connector/cn_hash.c
 create mode 100644 lib/cn_hash_test.c
 create mode 100644 lib/cn_hash_test.h
 create mode 100644 tools/testing/selftests/connector/thread.c
 create mode 100644 tools/testing/selftests/connector/thread_filter.c

Comments

Jakub Kicinski Oct. 15, 2024, 4:01 p.m. UTC | #1 
On Sun, 13 Oct 2024 10:06:14 -0700 Anjali Kulkarni wrote:
> However, for threads, when it does a pthread_exit(&exit_status) call, the
> kernel is not aware of the exit status with which pthread_exit is called.
> It is sent by child thread to the parent process, if it is waiting in
> pthread_join(). Hence, for a thread exiting abnormally, kernel cannot
> send notifications to any listening processes.

I really don't think this should be going via networking.
We can help review the netlink bits, if any, but otherwise we are far
outside of our comfort zone. 

IOW when you repost please drop the net-next designation and you'll
need to find someone else to merge these patches, or send a PR directly
to Linus himself during the merge window.
Anjali Kulkarni Oct. 15, 2024, 4:22 p.m. UTC | #2 
On 10/15/24, 9:02 AM, "Jakub Kicinski" <kuba@kernel.org <mailto:kuba@kernel.org>> wrote:


On Sun, 13 Oct 2024 10:06:14 -0700 Anjali Kulkarni wrote:
> However, for threads, when it does a pthread_exit(&exit_status) call, the
> kernel is not aware of the exit status with which pthread_exit is called.
> It is sent by child thread to the parent process, if it is waiting in
> pthread_join(). Hence, for a thread exiting abnormally, kernel cannot
> send notifications to any listening processes.


I really don't think this should be going via networking.
We can help review the netlink bits, if any, but otherwise we are far
outside of our comfort zone. 


IOW when you repost please drop the net-next designation and you'll
need to find someone else to merge these patches, or send a PR directly
to Linus himself during the merge window.

ANJALI> Thank you! However, looking at the MAINTAINERS file, drivers/connector/ is listed under NETWORKING DRIVERS. Hence sending on net-next is the most appropriate place?
Jakub Kicinski Oct. 15, 2024, 4:48 p.m. UTC | #3 
On Tue, 15 Oct 2024 16:22:24 +0000 Anjali Kulkarni wrote:
> Thank you! However, looking at the MAINTAINERS file,
> drivers/connector/ is listed under NETWORKING DRIVERS.
> Hence sending on net-next is the most appropriate place?   

Hm. It was done relatively recently in

commit 46cf789b68b25744be3dc99efd4afce4a5381b5c
Author: David S. Miller <davem@davemloft.net>
Date:   Thu Sep 10 08:40:13 2020 -0700

    connector: Move maintainence under networking drivers umbrella.
    
    Evgeniy does not have the time nor capacity to maintain the
    connector subsystem any longer, so just move it under networking
    as that is effectively what has been happening lately.
    
    Signed-off-by: David S. Miller <davem@davemloft.net>

There wasn't much development in this code for a long while,
so I guess it was done just to make sure patches don't fall
thru the cracks. But you seem to be trying to develop it more
actively.

Okay, if nobody else who want to take these patches into their tree
raises their hand - we can try to continue to process the patches,
as long as Peter or someone else with the expertise acks them.
Anjali Kulkarni Oct. 15, 2024, 4:53 p.m. UTC | #4 
On 10/15/24, 9:48 AM, "Jakub Kicinski" <kuba@kernel.org <mailto:kuba@kernel.org>> wrote:


On Tue, 15 Oct 2024 16:22:24 +0000 Anjali Kulkarni wrote:
> Thank you! However, looking at the MAINTAINERS file,
> drivers/connector/ is listed under NETWORKING DRIVERS.
> Hence sending on net-next is the most appropriate place? 


Hm. It was done relatively recently in


commit 46cf789b68b25744be3dc99efd4afce4a5381b5c
Author: David S. Miller <davem@davemloft.net <mailto:davem@davemloft.net>>
Date: Thu Sep 10 08:40:13 2020 -0700


connector: Move maintainence under networking drivers umbrella.


Evgeniy does not have the time nor capacity to maintain the
connector subsystem any longer, so just move it under networking
as that is effectively what has been happening lately.


Signed-off-by: David S. Miller <davem@davemloft.net <mailto:davem@davemloft.net>>


There wasn't much development in this code for a long while,
so I guess it was done just to make sure patches don't fall
thru the cracks. But you seem to be trying to develop it more
actively.


Okay, if nobody else who want to take these patches into their tree
raises their hand - we can try to continue to process the patches,
as long as Peter or someone else with the expertise acks them.

ANJALI> Ok, thanks very much!

Anjali