[patch] quic PTO counter fixes

[Vladimir Homutov]

On Fri, Oct 27, 2023 at 12:27:22AM +0400, Sergey Kandaurov wrote:
> On Thu, Oct 26, 2023 at 05:20:39PM +0300, Vladimir Homutov wrote:
> > 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
>
> Well, I don't think retry needs logging, because this is not a real
> packet, it carries a token and is used to construct a Retry packet
> (which is also a special packet) later in ngx_quic_encrypt().
> Logging such a construct is bogus, because nearly all fields aren't
> initialized to sensible values, personally I've got the following:
>
>  quic packet tx init bytes:0 need_ack:0 number:0 encoded nl:0 trunc:0x0

yes, this makes sense, removed.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: dbg_frames_3.diff
Type: text/x-diff
Size: 5102 bytes
Desc: not available
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20231026/2f59b3dc/attachment-0001.bin>