On Tue, Jul 27, 2021 at 02:44:51AM +0200, Alexander Bluhm wrote:
> On Tue, Jul 27, 2021 at 12:30:06AM +0300, Vitaliy Makkoveev wrote:
> > > The diff below makes pipex(4) a little bit more parallelization
> > > reliable.
>
> > > @@ -2238,10 +2240,6 @@ pipex_mppe_input(struct mbuf *m0, struct
> > > coher_cnt &= PIPEX_COHERENCY_CNT_MASK;
> > > pktloss = 0;
> > >
> > > - PIPEX_MPPE_DBG((session, LOG_DEBUG, "in coher_cnt=%03x %s%s",
> > > - mppe->coher_cnt, (flushed) ? "[flushed]" : "",
> > > - (encrypt) ? "[encrypt]" : ""));
> > > -
> > > if (encrypt == 0) {
> > > pipex_session_log(session, LOG_DEBUG,
> > > "Received unexpected MPPE packet.(no ecrypt)");
> > > @@ -2251,6 +2249,12 @@ pipex_mppe_input(struct mbuf *m0, struct
> > > /* adjust mbuf */
> > > m_adj(m0, sizeof(coher_cnt));
> > >
> > > + mtx_enter(&mppe->pxm_mtx);
> > > +
> > > + PIPEX_MPPE_DBG((session, LOG_DEBUG, "in coher_cnt=%03x %s%s",
> > > + mppe->coher_cnt, (flushed) ? "[flushed]" : "",
> > > + (encrypt) ? "[encrypt]" : ""));
> > > +
>
> Is is really necessary to move the debug print? encrypt is always
> 0 now. I guess the author of this line also wanted to see the
> messages with encrypt == 1. It is only a reading access to
> mppe->coher_cnt in a debug print that is not MP safe.
>
I like to have this debug 'mppe->coher_cnt' access consistent with debug
'mppe->coher_cnt' access below. PIPEX_MPPE_DBG() is not compiled so if
we keep it as is we perform "encrypt == 0" with useless `pxm_mtx' held
and we should release it in error path. For me this is also ugly. In
other hand we don't loose this debug messages when "encrypt == 0" is
true. I moved it back.
>
> > > int16_t stateless:1, /* [I] key change mode */
> > > - resetreq:1, /* [N] */
> > > + resetreq:1, /* [m] */
> > > reserved:14;
>
> Can we have differnet locks on bit fields? I thought the minimum
> granulatrity of locked access is an int.
I guess this case should be identical to `*_flags' struct members which
contain immutable bits. Mutable bits are protected by lock but we
perform read-only access to immutable bits without lock held.
It's obvious all mutable bits of bit field should be protected by the
same lock. Here the only `resetreq' is the such bit.
I'm not the fun of bit fields and I like them to be rewritten as
`*_flags'. This should make the code consistent to other places and
make us sure the compiler's behaviour is the same.
>
> anyway OK bluhm@
>
Committed, thanks!
