Re: [PATCH net-next v10 08/15] quic: add path management

On 2/25/26 3:34 AM, Xin Long wrote:
> +/* Binds a QUIC path to a local port and sets up a UDP socket. */
> +int quic_path_bind(struct sock *sk, struct quic_path_group *paths, u8 path)
> +{
> +     union quic_addr *a = quic_path_saddr(paths, path);
> +     int rover, low, high, remaining;
> +     struct net *net = sock_net(sk);
> +     struct quic_uhash_head *head;
> +     struct quic_udp_sock *us;
> +     u16 port;
> +
> +     port = ntohs(a->v4.sin_port);
> +     if (port) {
> +             head = quic_udp_sock_head(net, port);
> +             mutex_lock(&head->lock);
> +             us = quic_udp_sock_lookup(sk, a, port);
> +             if (us) {

When the quick socket is already bound to a local port, reusing an
existing udp tunnel sock is allowed, but when the quick socket is not
bound, UDP tunnel sock reused is prevented. This looks confusing and not
documented, please clarify the behavior and/or make it consistent.


> +                     if (!quic_udp_sock_get(us)) { /* Releasing in workqueue; retry later. */
> +                             mutex_unlock(&head->lock);
> +                             return -EAGAIN;

Why not -EADDRINUSE here?

> +                     }
> +             } else {
> +                     us = quic_udp_sock_create(sk, a);
> +                     if (!us) {
> +                             mutex_unlock(&head->lock);
> +                             return -EINVAL;

It's probably better to propagate an error code (PTR_ERR) from
quic_udp_sock_create(), or use -ENOMEM

[...]
> @@ -332,6 +333,12 @@ static __init int quic_init(void)
>       if (err)
>               goto err_hash;
>
> +     quic_wq = create_workqueue("quic_workqueue");
> +     if (!quic_wq) {
> +             err = -ENOMEM;
> +             goto err_wq;
> +     }

AI review noted that:

This isn't a bug, but create_workqueue() is a legacy API marked with
__WQ_LEGACY in include/linux/workqueue.h. Should new subsystem code use
alloc_workqueue() with explicit flags instead?

Looking at include/linux/workqueue.h, create_workqueue() implicitly sets
WQ_PERCPU, creating per-CPU worker threads. Since quic_wq only handles
infrequent UDP socket cleanup via quic_udp_sock_put_work() in path.c, is
per-CPU allocation necessary here? Would alloc_workqueue("quic_workqueue",
WQ_MEM_RECLAIM, 0) be more appropriate, or could this simply use system_wq
if memory reclaim safety is not required?


/P