[patch] quic PTO counter fixes

[Vladimir Homutov]

On Thu, Oct 26, 2023 at 03:08:55AM +0400, Sergey Kandaurov wrote:
> On Wed, Oct 11, 2023 at 04:58:47PM +0300, Vladimir Homutov via nginx-devel wrote:
[..]

> > diff --git a/src/event/quic/ngx_event_quic_output.c b/src/event/quic/ngx_event_quic_output.c
> > --- a/src/event/quic/ngx_event_quic_output.c
> > +++ b/src/event/quic/ngx_event_quic_output.c
> > @@ -563,8 +563,6 @@ ngx_quic_output_packet(ngx_connection_t
> >              pkt.need_ack = 1;
> >          }
> >
> > -        ngx_quic_log_frame(c->log, f, 1);
> > -
> >          flen = ngx_quic_create_frame(p, f);
> >          if (flen == -1) {
> >              return NGX_ERROR;
> > @@ -578,6 +576,8 @@ ngx_quic_output_packet(ngx_connection_t
> >          f->last = now;
> >          f->plen = 0;
> >
> > +        ngx_quic_log_frame(c->log, f, 1);
> > +
> >          nframes++;
>
> I'd rather move setting frame fields before calling
> ngx_quic_log_frame()/ngx_quic_create_frame()
> to preserve consistency with other places, i.e.:
> - set fields
> - log frame
> - create frame
>
> To look as follows:
>
> :        f->pnum = ctx->pnum;
> :        f->first = now;
> :        f->last = now;
> :        f->plen = 0;
> :
> :        ngx_quic_log_frame(c->log, f, 1);
> :
> :        flen = ngx_quic_create_frame(p, f);
> :

agreed

> >      }
> >
> > @@ -925,6 +925,13 @@ ngx_quic_send_early_cc(ngx_connection_t
> >
> >      res.data = dst;
> >
> > +    ngx_log_debug7(NGX_LOG_DEBUG_EVENT, c->log, 0,
> > +                   "quic packet tx %s bytes:%ui need_ack:%d"
> > +                   " number:%L encoded nl:%d trunc:0x%xD frame:%ui]",
>
> typo: closing square bracket

thanks, removed

> Not sure we need logging for a (particular) frame in packet logging,
> not to say that it looks like a layering violation.
> Anyway, it is logged nearby, for example:
>
>  quic frame tx init:0 CONNECTION_CLOSE err:11 invalid address validation token ft:0
>  quic packet tx init bytes:36 need_ack:0 number:0 encoded nl:1 trunc:0x0
>
> So I'd remove this part.

agreed, frame logging removed

> > +                   ngx_quic_level_name(pkt.level), pkt.payload.len,
> > +                   pkt.need_ack, pkt.number, pkt.num_len, pkt.trunc,
> > +                   frame->type);
> > +
>
> BTW, it would make sense to get a new macro / inline function
> for packet tx logging, similar to ngx_quic_log_frame(),
> since we will have three places with identical ngx_log_debug7().

actually, four (we have also retry), so having a macro is a good idea

updated patch attached
-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbg_frames_2.diff
Type: text/x-diff
Size: 5317 bytes
Desc: not available
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20231026/23c7f77f/attachment.bin>