Re: [PATCH] netclassid: on modifying netclassid, only consider the main process.

[Simon Horman]

On Sun, Oct 08, 2023 at 11:04:42AM +0800, zhailiansen wrote:

Hi,

thanks for your patch.
Some minor comments from my side.

> When modifying netclassid, the command("echo 0x100001 > net_cls.classid") will
> take more time on muti threads of one process, because the process create many

nit: muti -> multiple ?

> fds.
> for example, one process exists 28000 fds and 60000 threads, echo command will
> task 45 seconds.
> Now, we only consider the main process when exec "iterate_fd", and the time is
> 52 milliseconds.

Please consider line-wrapping the patch description at 75 bytes.

This patch seems best targeted at net-next, this information should be
included in the patch. So if post a v2 please consider using:

	Subject: [PATCH v2] ...

> Signed-off-by: zhailiansen <zhailiansen@xxxxxxxxxxxx>

Please consider signing off using your real name.

e.g. Signed-off-by: Firstname Lastname <...>

> ---
>  net/core/netclassid_cgroup.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/net/core/netclassid_cgroup.c b/net/core/netclassid_cgroup.c
> index d6a70ae..78c903c 100644
> --- a/net/core/netclassid_cgroup.c
> +++ b/net/core/netclassid_cgroup.c
> @@ -88,6 +88,12 @@ static void update_classid_task(struct task_struct *p, u32 classid)
>  	};
>  	unsigned int fd = 0;
>  
> +	/* Only update the leader task, when multi threads in this task,
> +	 * so it can avoid the useless traversal.
> +	 */
> +	if (p && p != p->group_leader)
> +		return;

Can p ever be NULL?
It is dereferenced unconditionally by the existing call to task_lock() below.

> +
>  	do {
>  		task_lock(p);
>  		fd = iterate_fd(p->files, fd, update_classid_sock, &ctx);
> -- 
> 1.8.3.1
> 
>