Welcome! Log In Create A New Profile

Advanced

Re: [PATCH 4 of 6] QUIC: never disable QUIC socket events

Roman Arutyunyan
January 17, 2023 04:56AM
Hi,

On Tue, Dec 13, 2022 at 08:57:12PM +0300, Maxim Dounin wrote:
> Hello!
>
> On Fri, Dec 09, 2022 at 09:38:50AM +0000, Roman Arutyunyan wrote:
>
> > # HG changeset patch
> > # User Roman Arutyunyan <arut@nginx.com>
> > # Date 1670256830 0
> > # Mon Dec 05 16:13:50 2022 +0000
> > # Branch quic
> > # Node ID de8bcaea559d151f5945d0a2e06c61b56a26a52b
> > # Parent b5c30f16ec8ba3ace2f58d77d294d9b355bf3267
> > QUIC: never disable QUIC socket events.
> >
> > Unlike TCP accept(), current QUIC implementation does not require new file
> > descriptors for new clients. Also, it does not work with accept mutex since
> > it normally requires reuseport option.
> >
> > diff --git a/src/event/ngx_event_accept.c b/src/event/ngx_event_accept.c
> > --- a/src/event/ngx_event_accept.c
> > +++ b/src/event/ngx_event_accept.c
> > @@ -416,6 +416,12 @@ ngx_disable_accept_events(ngx_cycle_t *c
> >
> > #endif
> >
> > +#if (NGX_QUIC)
> > + if (ls[i].quic) {
> > + continue;
> > + }
> > +#endif
> > +
>
> As long as the reuseport option is used, this should happen
> automatically. If it's not, it might be a bad idea to do not
> disable accepting new QUIC connections, given that QUIC
> connections can still use file descriptors for other tasks, such
> as opening files and connecting to backends.
>
> On the other hand, existing QUIC implementation cannot receive
> packets from previously created connections if socket events are
> disabled. And this might be a good reason for the change.
> If this is the reason, shouldn't it be in the commit log?

Exactly. With current QUIC implementation, QUIC listen sockets aren't used
only for "listening", but also for receiving QUIC packets for existing
connections.

> Also, it looks like with client sockets, as introduced later in
> this patch series, it would be appropriate to actually disable
> events on QUIC listening sockets.

IMHO, even when client sockets are committed, the current approach will still
be available as an option. Client sockets approach has a number of issues,
which the current implementation does not have.

> > if (ngx_del_event(c->read, NGX_READ_EVENT, NGX_DISABLE_EVENT)
> > == NGX_ERROR)
> > {

Also, it looks like the problem is not specific to QUIC, but also exists for
regular Stream/UDP and should be addressed separately as a fix for mainline.
Probably we need a listen flag which would signal if the listen socket should
never be disabled, or just a more sophisticated condition in
ngx_disable_accept_events(). Anyway, I'll remove the patch from this series.

> >
> > _______________________________________________
> > nginx-devel mailing list -- nginx-devel@nginx.org
> > To unsubscribe send an email to nginx-devel-leave@nginx.org
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Roman Arutyunyan
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

[PATCH 4 of 6] QUIC: never disable QUIC socket events

Roman Arutyunyan 375 December 09, 2022 04:40AM

Re: [PATCH 4 of 6] QUIC: never disable QUIC socket events

Maxim Dounin 45 December 13, 2022 12:58PM

Re: [PATCH 4 of 6] QUIC: never disable QUIC socket events

Roman Arutyunyan 20 January 17, 2023 04:56AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 267
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready