bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/2] bpf: add bpf_send_signal_thread() helper
@ 2020-01-10  1:15 Yonghong Song
  2020-01-10  1:15 ` [PATCH bpf-next 1/2] " Yonghong Song
  2020-01-10  1:15 ` [PATCH bpf-next 2/2] tools/bpf: add a selftest for bpf_send_signal_thread() Yonghong Song
  0 siblings, 2 replies; 7+ messages in thread
From: Yonghong Song @ 2020-01-10  1:15 UTC (permalink / raw)
  To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

Commit 8b401f9ed244 ("bpf: implement bpf_send_signal() helper")
added helper bpf_send_signal() which permits bpf program to
send a signal to the current process.

This patch implemented a new helper bpf_send_signal_thread()
to send a signal to the current thread. This helper can simplify
user space code if the thread context of bpf sending signal
is needed in user space. Please see Patch #1 for details of
use case and kernel implementation.

Patch #2 added some bpf self tests for the new helper.

Yonghong Song (2):
  bpf: add bpf_send_signal_thread() helper
  tools/bpf: add a selftest for bpf_send_signal_thread()

 include/uapi/linux/bpf.h                      | 18 +++++++++--
 kernel/trace/bpf_trace.c                      | 27 +++++++++++++++--
 tools/include/uapi/linux/bpf.h                | 18 +++++++++--
 .../selftests/bpf/prog_tests/send_signal.c    | 30 ++++++++++++-------
 .../bpf/progs/test_send_signal_kern.c         |  9 ++++--
 5 files changed, 82 insertions(+), 20 deletions(-)

-- 
2.17.1


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH bpf-next 1/2] bpf: add bpf_send_signal_thread() helper
  2020-01-10  1:15 [PATCH bpf-next 0/2] bpf: add bpf_send_signal_thread() helper Yonghong Song
@ 2020-01-10  1:15 ` Yonghong Song
  2020-01-14  0:58   ` Andrii Nakryiko
  2020-01-10  1:15 ` [PATCH bpf-next 2/2] tools/bpf: add a selftest for bpf_send_signal_thread() Yonghong Song
  1 sibling, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2020-01-10  1:15 UTC (permalink / raw)
  To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

Commit 8b401f9ed244 ("bpf: implement bpf_send_signal() helper")
added helper bpf_send_signal() which permits bpf program to
send a signal to the current process.

We found a use case where sending the signal to the current
thread is more preferable.
  - A bpf program will collect the stack trace and then
    send signal to the user application.
  - The user application will add some thread specific
    information to the just collected stack trace for
    later analysis.

If bpf_send_signal() is used, user application will need
to check whether the thread receiving the signal matches
the thread collecting the stack by checking thread id.
If not, it will need to send signal to another thread
through pthread_kill().

This patch proposed a new helper bpf_send_signal_thread(),
which sends the signal to the current thread. This way,
user space is guaranteed that bpf_program execution context
and user space signal handling context are the same thread.

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 include/uapi/linux/bpf.h | 18 ++++++++++++++++--
 kernel/trace/bpf_trace.c | 27 ++++++++++++++++++++++++---
 2 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 52966e758fe5..3320f8bdfe7e 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -2714,7 +2714,7 @@ union bpf_attr {
  *
  * int bpf_send_signal(u32 sig)
  *	Description
- *		Send signal *sig* to the current task.
+ *		Send signal *sig* to the process of the current task.
  *	Return
  *		0 on success or successfully queued.
  *
@@ -2850,6 +2850,19 @@ union bpf_attr {
  *	Return
  *		0 on success, or a negative error in case of failure.
  *
+ * int bpf_send_signal_thread(u32 sig)
+ *	Description
+ *		Send signal *sig* to the current task.
+ *	Return
+ *		0 on success or successfully queued.
+ *
+ *		**-EBUSY** if work queue under nmi is full.
+ *
+ *		**-EINVAL** if *sig* is invalid.
+ *
+ *		**-EPERM** if no permission to send the *sig*.
+ *
+ *		**-EAGAIN** if bpf program can try again.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2968,7 +2981,8 @@ union bpf_attr {
 	FN(probe_read_kernel),		\
 	FN(probe_read_user_str),	\
 	FN(probe_read_kernel_str),	\
-	FN(tcp_send_ack),
+	FN(tcp_send_ack),		\
+	FN(send_signal_thread),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
index e5ef4ae9edb5..19e793aa441a 100644
--- a/kernel/trace/bpf_trace.c
+++ b/kernel/trace/bpf_trace.c
@@ -703,6 +703,7 @@ struct send_signal_irq_work {
 	struct irq_work irq_work;
 	struct task_struct *task;
 	u32 sig;
+	enum pid_type type;
 };
 
 static DEFINE_PER_CPU(struct send_signal_irq_work, send_signal_work);
@@ -712,10 +713,10 @@ static void do_bpf_send_signal(struct irq_work *entry)
 	struct send_signal_irq_work *work;
 
 	work = container_of(entry, struct send_signal_irq_work, irq_work);
-	group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, PIDTYPE_TGID);
+	group_send_sig_info(work->sig, SEND_SIG_PRIV, work->task, work->type);
 }
 
-BPF_CALL_1(bpf_send_signal, u32, sig)
+static int bpf_send_signal_common(u32 sig, enum pid_type type)
 {
 	struct send_signal_irq_work *work = NULL;
 
@@ -748,11 +749,17 @@ BPF_CALL_1(bpf_send_signal, u32, sig)
 		 */
 		work->task = current;
 		work->sig = sig;
+		work->type = type;
 		irq_work_queue(&work->irq_work);
 		return 0;
 	}
 
-	return group_send_sig_info(sig, SEND_SIG_PRIV, current, PIDTYPE_TGID);
+	return group_send_sig_info(sig, SEND_SIG_PRIV, current, type);
+}
+
+BPF_CALL_1(bpf_send_signal, u32, sig)
+{
+	return bpf_send_signal_common(sig, PIDTYPE_TGID);
 }
 
 static const struct bpf_func_proto bpf_send_signal_proto = {
@@ -762,6 +769,18 @@ static const struct bpf_func_proto bpf_send_signal_proto = {
 	.arg1_type	= ARG_ANYTHING,
 };
 
+BPF_CALL_1(bpf_send_signal_thread, u32, sig)
+{
+	return bpf_send_signal_common(sig, PIDTYPE_PID);
+}
+
+static const struct bpf_func_proto bpf_send_signal_thread_proto = {
+	.func		= bpf_send_signal_thread,
+	.gpl_only	= false,
+	.ret_type	= RET_INTEGER,
+	.arg1_type	= ARG_ANYTHING,
+};
+
 static const struct bpf_func_proto *
 tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 {
@@ -822,6 +841,8 @@ tracing_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
 #endif
 	case BPF_FUNC_send_signal:
 		return &bpf_send_signal_proto;
+	case BPF_FUNC_send_signal_thread:
+		return &bpf_send_signal_thread_proto;
 	default:
 		return NULL;
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH bpf-next 2/2] tools/bpf: add a selftest for bpf_send_signal_thread()
  2020-01-10  1:15 [PATCH bpf-next 0/2] bpf: add bpf_send_signal_thread() helper Yonghong Song
  2020-01-10  1:15 ` [PATCH bpf-next 1/2] " Yonghong Song
@ 2020-01-10  1:15 ` Yonghong Song
  2020-01-14  1:19   ` Andrii Nakryiko
  1 sibling, 1 reply; 7+ messages in thread
From: Yonghong Song @ 2020-01-10  1:15 UTC (permalink / raw)
  To: bpf; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team

The test_progs send_signal() is amended to test
bpf_send_signal_thread() as well.

  $ ./test_progs -n 40
  #40/1 send_signal_tracepoint:OK
  #40/2 send_signal_perf:OK
  #40/3 send_signal_nmi:OK
  #40/4 send_signal_tracepoint_thread:OK
  #40/5 send_signal_perf_thread:OK
  #40/6 send_signal_nmi_thread:OK
  #40 send_signal:OK
  Summary: 1/6 PASSED, 0 SKIPPED, 0 FAILED

Signed-off-by: Yonghong Song <yhs@fb.com>
---
 tools/include/uapi/linux/bpf.h                | 18 +++++++++--
 .../selftests/bpf/prog_tests/send_signal.c    | 30 ++++++++++++-------
 .../bpf/progs/test_send_signal_kern.c         |  9 ++++--
 3 files changed, 42 insertions(+), 15 deletions(-)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 52966e758fe5..3320f8bdfe7e 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -2714,7 +2714,7 @@ union bpf_attr {
  *
  * int bpf_send_signal(u32 sig)
  *	Description
- *		Send signal *sig* to the current task.
+ *		Send signal *sig* to the process of the current task.
  *	Return
  *		0 on success or successfully queued.
  *
@@ -2850,6 +2850,19 @@ union bpf_attr {
  *	Return
  *		0 on success, or a negative error in case of failure.
  *
+ * int bpf_send_signal_thread(u32 sig)
+ *	Description
+ *		Send signal *sig* to the current task.
+ *	Return
+ *		0 on success or successfully queued.
+ *
+ *		**-EBUSY** if work queue under nmi is full.
+ *
+ *		**-EINVAL** if *sig* is invalid.
+ *
+ *		**-EPERM** if no permission to send the *sig*.
+ *
+ *		**-EAGAIN** if bpf program can try again.
  */
 #define __BPF_FUNC_MAPPER(FN)		\
 	FN(unspec),			\
@@ -2968,7 +2981,8 @@ union bpf_attr {
 	FN(probe_read_kernel),		\
 	FN(probe_read_user_str),	\
 	FN(probe_read_kernel_str),	\
-	FN(tcp_send_ack),
+	FN(tcp_send_ack),		\
+	FN(send_signal_thread),
 
 /* integer value in 'imm' field of BPF_CALL instruction selects which helper
  * function eBPF program intends to call
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index b607112c64e7..b12350832be3 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -10,7 +10,8 @@ static void sigusr1_handler(int signum)
 
 static void test_send_signal_common(struct perf_event_attr *attr,
 				    int prog_type,
-				    const char *test_name)
+				    const char *test_name,
+				    bool send_thread)
 {
 	int err = -1, pmu_fd, prog_fd, info_map_fd, status_map_fd;
 	const char *file = "./test_send_signal_kern.o";
@@ -110,7 +111,7 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 
 	/* trigger the bpf send_signal */
 	key = 0;
-	val = (((__u64)(SIGUSR1)) << 32) | pid;
+	val = (((__u64)send_thread) << 48) | (((__u64)(SIGUSR1)) << 32) | pid;
 	bpf_map_update_elem(info_map_fd, &key, &val, 0);
 
 	/* notify child that bpf program can send_signal now */
@@ -140,7 +141,7 @@ static void test_send_signal_common(struct perf_event_attr *attr,
 	wait(NULL);
 }
 
-static void test_send_signal_tracepoint(void)
+static void test_send_signal_tracepoint(bool send_thread)
 {
 	const char *id_path = "/sys/kernel/debug/tracing/events/syscalls/sys_enter_nanosleep/id";
 	struct perf_event_attr attr = {
@@ -168,10 +169,11 @@ static void test_send_signal_tracepoint(void)
 
 	attr.config = strtol(buf, NULL, 0);
 
-	test_send_signal_common(&attr, BPF_PROG_TYPE_TRACEPOINT, "tracepoint");
+	test_send_signal_common(&attr, BPF_PROG_TYPE_TRACEPOINT, "tracepoint",
+				send_thread);
 }
 
-static void test_send_signal_perf(void)
+static void test_send_signal_perf(bool send_thread)
 {
 	struct perf_event_attr attr = {
 		.sample_period = 1,
@@ -180,10 +182,10 @@ static void test_send_signal_perf(void)
 	};
 
 	test_send_signal_common(&attr, BPF_PROG_TYPE_PERF_EVENT,
-				"perf_sw_event");
+				"perf_sw_event", send_thread);
 }
 
-static void test_send_signal_nmi(void)
+static void test_send_signal_nmi(bool send_thread)
 {
 	struct perf_event_attr attr = {
 		.sample_freq = 50,
@@ -211,15 +213,21 @@ static void test_send_signal_nmi(void)
 	}
 
 	test_send_signal_common(&attr, BPF_PROG_TYPE_PERF_EVENT,
-				"perf_hw_event");
+				"perf_hw_event", send_thread);
 }
 
 void test_send_signal(void)
 {
 	if (test__start_subtest("send_signal_tracepoint"))
-		test_send_signal_tracepoint();
+		test_send_signal_tracepoint(false);
 	if (test__start_subtest("send_signal_perf"))
-		test_send_signal_perf();
+		test_send_signal_perf(false);
 	if (test__start_subtest("send_signal_nmi"))
-		test_send_signal_nmi();
+		test_send_signal_nmi(false);
+	if (test__start_subtest("send_signal_tracepoint_thread"))
+		test_send_signal_tracepoint(true);
+	if (test__start_subtest("send_signal_perf_thread"))
+		test_send_signal_perf(true);
+	if (test__start_subtest("send_signal_nmi_thread"))
+		test_send_signal_nmi(true);
 }
diff --git a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
index 0e6be01157e6..4a722024c32b 100644
--- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
+++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
@@ -23,6 +23,7 @@ int bpf_send_signal_test(void *ctx)
 {
 	__u64 *info_val, *status_val;
 	__u32 key = 0, pid, sig;
+	int use_signal_thread;
 	int ret;
 
 	status_val = bpf_map_lookup_elem(&status_map, &key);
@@ -33,11 +34,15 @@ int bpf_send_signal_test(void *ctx)
 	if (!info_val || *info_val == 0)
 		return 0;
 
-	sig = *info_val >> 32;
+	use_signal_thread = *info_val >> 48;
+	sig = *info_val >> 32 & 0xFFFF;
 	pid = *info_val & 0xffffFFFF;
 
 	if ((bpf_get_current_pid_tgid() >> 32) == pid) {
-		ret = bpf_send_signal(sig);
+		if (use_signal_thread)
+			ret = bpf_send_signal_thread(sig);
+		else
+			ret = bpf_send_signal(sig);
 		if (ret == 0)
 			*status_val = 1;
 	}
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next 1/2] bpf: add bpf_send_signal_thread() helper
  2020-01-10  1:15 ` [PATCH bpf-next 1/2] " Yonghong Song
@ 2020-01-14  0:58   ` Andrii Nakryiko
  2020-01-14  1:15     ` Yonghong Song
  0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2020-01-14  0:58 UTC (permalink / raw)
  To: Yonghong Song; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Thu, Jan 9, 2020 at 5:16 PM Yonghong Song <yhs@fb.com> wrote:
>
> Commit 8b401f9ed244 ("bpf: implement bpf_send_signal() helper")
> added helper bpf_send_signal() which permits bpf program to
> send a signal to the current process.
>
> We found a use case where sending the signal to the current
> thread is more preferable.
>   - A bpf program will collect the stack trace and then
>     send signal to the user application.
>   - The user application will add some thread specific
>     information to the just collected stack trace for
>     later analysis.
>
> If bpf_send_signal() is used, user application will need
> to check whether the thread receiving the signal matches
> the thread collecting the stack by checking thread id.
> If not, it will need to send signal to another thread
> through pthread_kill().
>
> This patch proposed a new helper bpf_send_signal_thread(),
> which sends the signal to the current thread. This way,
> user space is guaranteed that bpf_program execution context
> and user space signal handling context are the same thread.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  include/uapi/linux/bpf.h | 18 ++++++++++++++++--
>  kernel/trace/bpf_trace.c | 27 ++++++++++++++++++++++++---
>  2 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 52966e758fe5..3320f8bdfe7e 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -2714,7 +2714,7 @@ union bpf_attr {
>   *
>   * int bpf_send_signal(u32 sig)
>   *     Description
> - *             Send signal *sig* to the current task.
> + *             Send signal *sig* to the process of the current task.
>   *     Return
>   *             0 on success or successfully queued.
>   *
> @@ -2850,6 +2850,19 @@ union bpf_attr {
>   *     Return
>   *             0 on success, or a negative error in case of failure.
>   *
> + * int bpf_send_signal_thread(u32 sig)
> + *     Description
> + *             Send signal *sig* to the current task.


This all makes sense and looks good, but I think it's very unclear why
the distinction between sending signal to process vs thread. Could you
extend bpf_send_signal and bpf_send_signal_thread descriptions
explaining the difference (e.g., that, according to POSIX, when
sending signal to a process, any thread within that process can get
signal delivered, while sending to a specific thread will ensure that
that specific thread will receive desired signal).

[...]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next 1/2] bpf: add bpf_send_signal_thread() helper
  2020-01-14  0:58   ` Andrii Nakryiko
@ 2020-01-14  1:15     ` Yonghong Song
  0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2020-01-14  1:15 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team



On 1/13/20 4:58 PM, Andrii Nakryiko wrote:
> On Thu, Jan 9, 2020 at 5:16 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> Commit 8b401f9ed244 ("bpf: implement bpf_send_signal() helper")
>> added helper bpf_send_signal() which permits bpf program to
>> send a signal to the current process.
>>
>> We found a use case where sending the signal to the current
>> thread is more preferable.
>>    - A bpf program will collect the stack trace and then
>>      send signal to the user application.
>>    - The user application will add some thread specific
>>      information to the just collected stack trace for
>>      later analysis.
>>
>> If bpf_send_signal() is used, user application will need
>> to check whether the thread receiving the signal matches
>> the thread collecting the stack by checking thread id.
>> If not, it will need to send signal to another thread
>> through pthread_kill().
>>
>> This patch proposed a new helper bpf_send_signal_thread(),
>> which sends the signal to the current thread. This way,
>> user space is guaranteed that bpf_program execution context
>> and user space signal handling context are the same thread.
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   include/uapi/linux/bpf.h | 18 ++++++++++++++++--
>>   kernel/trace/bpf_trace.c | 27 ++++++++++++++++++++++++---
>>   2 files changed, 40 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
>> index 52966e758fe5..3320f8bdfe7e 100644
>> --- a/include/uapi/linux/bpf.h
>> +++ b/include/uapi/linux/bpf.h
>> @@ -2714,7 +2714,7 @@ union bpf_attr {
>>    *
>>    * int bpf_send_signal(u32 sig)
>>    *     Description
>> - *             Send signal *sig* to the current task.
>> + *             Send signal *sig* to the process of the current task.
>>    *     Return
>>    *             0 on success or successfully queued.
>>    *
>> @@ -2850,6 +2850,19 @@ union bpf_attr {
>>    *     Return
>>    *             0 on success, or a negative error in case of failure.
>>    *
>> + * int bpf_send_signal_thread(u32 sig)
>> + *     Description
>> + *             Send signal *sig* to the current task.
> 
> 
> This all makes sense and looks good, but I think it's very unclear why
> the distinction between sending signal to process vs thread. Could you
> extend bpf_send_signal and bpf_send_signal_thread descriptions
> explaining the difference (e.g., that, according to POSIX, when
> sending signal to a process, any thread within that process can get
> signal delivered, while sending to a specific thread will ensure that
> that specific thread will receive desired signal).

Sounds good. Will send v2 with better descriptions later.

> 
> [...]
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next 2/2] tools/bpf: add a selftest for bpf_send_signal_thread()
  2020-01-10  1:15 ` [PATCH bpf-next 2/2] tools/bpf: add a selftest for bpf_send_signal_thread() Yonghong Song
@ 2020-01-14  1:19   ` Andrii Nakryiko
  2020-01-14  1:22     ` Yonghong Song
  0 siblings, 1 reply; 7+ messages in thread
From: Andrii Nakryiko @ 2020-01-14  1:19 UTC (permalink / raw)
  To: Yonghong Song; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team

On Thu, Jan 9, 2020 at 5:16 PM Yonghong Song <yhs@fb.com> wrote:
>
> The test_progs send_signal() is amended to test
> bpf_send_signal_thread() as well.
>
>   $ ./test_progs -n 40
>   #40/1 send_signal_tracepoint:OK
>   #40/2 send_signal_perf:OK
>   #40/3 send_signal_nmi:OK
>   #40/4 send_signal_tracepoint_thread:OK
>   #40/5 send_signal_perf_thread:OK
>   #40/6 send_signal_nmi_thread:OK
>   #40 send_signal:OK
>   Summary: 1/6 PASSED, 0 SKIPPED, 0 FAILED
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
>  tools/include/uapi/linux/bpf.h                | 18 +++++++++--

maybe do tools/uapi header sync in a first patch, along the original change?

>  .../selftests/bpf/prog_tests/send_signal.c    | 30 ++++++++++++-------
>  .../bpf/progs/test_send_signal_kern.c         |  9 ++++--
>  3 files changed, 42 insertions(+), 15 deletions(-)
>

[...]

> diff --git a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
> index 0e6be01157e6..4a722024c32b 100644
> --- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
> +++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
> @@ -23,6 +23,7 @@ int bpf_send_signal_test(void *ctx)
>  {
>         __u64 *info_val, *status_val;
>         __u32 key = 0, pid, sig;
> +       int use_signal_thread;
>         int ret;
>
>         status_val = bpf_map_lookup_elem(&status_map, &key);
> @@ -33,11 +34,15 @@ int bpf_send_signal_test(void *ctx)
>         if (!info_val || *info_val == 0)
>                 return 0;
>
> -       sig = *info_val >> 32;
> +       use_signal_thread = *info_val >> 48;
> +       sig = *info_val >> 32 & 0xFFFF;
>         pid = *info_val & 0xffffFFFF;

Would you mind rewriting this test w/ BPF skeleton and global data? It
would make it cleaner without all this masking stuff?

>
>         if ((bpf_get_current_pid_tgid() >> 32) == pid) {
> -               ret = bpf_send_signal(sig);
> +               if (use_signal_thread)
> +                       ret = bpf_send_signal_thread(sig);
> +               else
> +                       ret = bpf_send_signal(sig);
>                 if (ret == 0)
>                         *status_val = 1;
>         }
> --
> 2.17.1
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH bpf-next 2/2] tools/bpf: add a selftest for bpf_send_signal_thread()
  2020-01-14  1:19   ` Andrii Nakryiko
@ 2020-01-14  1:22     ` Yonghong Song
  0 siblings, 0 replies; 7+ messages in thread
From: Yonghong Song @ 2020-01-14  1:22 UTC (permalink / raw)
  To: Andrii Nakryiko; +Cc: bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team



On 1/13/20 5:19 PM, Andrii Nakryiko wrote:
> On Thu, Jan 9, 2020 at 5:16 PM Yonghong Song <yhs@fb.com> wrote:
>>
>> The test_progs send_signal() is amended to test
>> bpf_send_signal_thread() as well.
>>
>>    $ ./test_progs -n 40
>>    #40/1 send_signal_tracepoint:OK
>>    #40/2 send_signal_perf:OK
>>    #40/3 send_signal_nmi:OK
>>    #40/4 send_signal_tracepoint_thread:OK
>>    #40/5 send_signal_perf_thread:OK
>>    #40/6 send_signal_nmi_thread:OK
>>    #40 send_signal:OK
>>    Summary: 1/6 PASSED, 0 SKIPPED, 0 FAILED
>>
>> Signed-off-by: Yonghong Song <yhs@fb.com>
>> ---
>>   tools/include/uapi/linux/bpf.h                | 18 +++++++++--
> 
> maybe do tools/uapi header sync in a first patch, along the original change?

Will do.

> 
>>   .../selftests/bpf/prog_tests/send_signal.c    | 30 ++++++++++++-------
>>   .../bpf/progs/test_send_signal_kern.c         |  9 ++++--
>>   3 files changed, 42 insertions(+), 15 deletions(-)
>>
> 
> [...]
> 
>> diff --git a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
>> index 0e6be01157e6..4a722024c32b 100644
>> --- a/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
>> +++ b/tools/testing/selftests/bpf/progs/test_send_signal_kern.c
>> @@ -23,6 +23,7 @@ int bpf_send_signal_test(void *ctx)
>>   {
>>          __u64 *info_val, *status_val;
>>          __u32 key = 0, pid, sig;
>> +       int use_signal_thread;
>>          int ret;
>>
>>          status_val = bpf_map_lookup_elem(&status_map, &key);
>> @@ -33,11 +34,15 @@ int bpf_send_signal_test(void *ctx)
>>          if (!info_val || *info_val == 0)
>>                  return 0;
>>
>> -       sig = *info_val >> 32;
>> +       use_signal_thread = *info_val >> 48;
>> +       sig = *info_val >> 32 & 0xFFFF;
>>          pid = *info_val & 0xffffFFFF;
> 
> Would you mind rewriting this test w/ BPF skeleton and global data? It
> would make it cleaner without all this masking stuff?

Previously I made the change to minimize the number of changed lines.
But since you are mentioning rewriting here, I will do it in v2.

> 
>>
>>          if ((bpf_get_current_pid_tgid() >> 32) == pid) {
>> -               ret = bpf_send_signal(sig);
>> +               if (use_signal_thread)
>> +                       ret = bpf_send_signal_thread(sig);
>> +               else
>> +                       ret = bpf_send_signal(sig);
>>                  if (ret == 0)
>>                          *status_val = 1;
>>          }
>> --
>> 2.17.1
>>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-01-14  1:22 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-10  1:15 [PATCH bpf-next 0/2] bpf: add bpf_send_signal_thread() helper Yonghong Song
2020-01-10  1:15 ` [PATCH bpf-next 1/2] " Yonghong Song
2020-01-14  0:58   ` Andrii Nakryiko
2020-01-14  1:15     ` Yonghong Song
2020-01-10  1:15 ` [PATCH bpf-next 2/2] tools/bpf: add a selftest for bpf_send_signal_thread() Yonghong Song
2020-01-14  1:19   ` Andrii Nakryiko
2020-01-14  1:22     ` Yonghong Song

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).