Re: pipex(4): introduce mutexes to protect 'pipex_session' context
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(>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!
Re: pipex(4): introduce mutexes to protect 'pipex_session' context
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(>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.
> > + mtx_enter(>pxs_mtx);
> > + ccp_id=session->ccp_id;
> > + session->ccp_id++;
> > + mtx_leave(>pxs_mtx);
ccp_id=session->ccp_id, there must be spaces around =
> > 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.
anyway OK bluhm@
Re: pipex(4): introduce mutexes to protect 'pipex_session' context
ping
> On 22 Jul 2021, at 01:56, Vitaliy Makkoveev wrote:
>
> With bluhm@'s diff for parallel forwarding pipex(4) could be accessed in
> parallel through (*ifp->if_input)() -> ether_input() ->
> pipex_pppoe_input(). PPPOE pipex(4) sessions are mostly immutable except
> MPPE crypt.
>
> The diff below makes pipex(4) a little bit more parallelization
> reliable.
>
> The new per-session `pxs_mtx' mutex(9) used to protect session's
> `ccp-id' which is incremented each time we send CCP reset-request.
>
> The new per-MPPE context `pxm_mtx' mutex(9) used to protect MPPE
> context. We have two of them: one for the input and one for output path.
> With bluhm@'s diff those paths are mostly serialized, except the case
> when we send CCP reset-request. I don't see the reason to other
> pipex_pppoe_input() threads spin while we perform pipex_ccp_output().
>
> Where is no lock order limitations because those new mutex(9)'es newer
> held together.
>
> I'm not PPPOE user and I'll be happy if such user tests this diff in
> real workflow.
>
> Index: sys/net/pipex.c
> ===
> RCS file: /cvs/src/sys/net/pipex.c,v
> retrieving revision 1.134
> diff -u -p -r1.134 pipex.c
> --- sys/net/pipex.c 20 Jul 2021 16:44:55 - 1.134
> +++ sys/net/pipex.c 21 Jul 2021 22:27:47 -
> @@ -263,6 +263,7 @@ pipex_init_session(struct pipex_session
>
> /* prepare a new session */
> session = pool_get(_session_pool, PR_WAITOK | PR_ZERO);
> + mtx_init(>pxs_mtx, IPL_SOFTNET);
> session->state = PIPEX_STATE_INITIAL;
> session->protocol = req->pr_protocol;
> session->session_id = req->pr_session_id;
> @@ -2099,6 +2100,7 @@ pipex_mppe_init(struct pipex_mppe *mppe,
> u_char *master_key, int has_oldkey)
> {
> memset(mppe, 0, sizeof(struct pipex_mppe));
> + mtx_init(>pxm_mtx, IPL_SOFTNET);
> if (stateless)
> mppe->stateless = 1;
> if (has_oldkey)
> @@ -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(>pxm_mtx);
> +
> + PIPEX_MPPE_DBG((session, LOG_DEBUG, "in coher_cnt=%03x %s%s",
> + mppe->coher_cnt, (flushed) ? "[flushed]" : "",
> + (encrypt) ? "[encrypt]" : ""));
> +
> /*
>* L2TP data session may be used without sequencing, PPP frames may
>* arrive in disorder. The 'coherency counter' of MPPE detects such
> @@ -2274,6 +2278,7 @@ pipex_mppe_input(struct mbuf *m0, struct
> pipex_session_log(session, LOG_DEBUG,
> "Workaround the out-of-sequence PPP framing
> problem: "
> "%d => %d", mppe->coher_cnt, coher_cnt);
> + mtx_leave(>pxm_mtx);
> goto drop;
> }
> rewind = 1;
> @@ -2305,10 +2310,19 @@ pipex_mppe_input(struct mbuf *m0, struct
> coher_cnt &= PIPEX_COHERENCY_CNT_MASK;
> mppe->coher_cnt = coher_cnt;
> } else if (mppe->coher_cnt != coher_cnt) {
> + int ccp_id;
> +
> + mtx_leave(>pxm_mtx);
> +
> /* Send CCP ResetReq */
> PIPEX_DBG((session, LOG_DEBUG, "CCP SendResetReq"));
> - pipex_ccp_output(session, CCP_RESETREQ,
> - session->ccp_id++);
> +
> + mtx_enter(>pxs_mtx);
> + ccp_id=session->ccp_id;
> + session->ccp_id++;
> + mtx_leave(>pxs_mtx);
> +
> + pipex_ccp_output(session, CCP_RESETREQ, ccp_id);
> goto drop;
> }
> if ((coher_cnt & 0xff) == 0xff) {
> @@ -2336,6 +2350,9 @@ pipex_mppe_input(struct mbuf *m0, struct
> mppe->coher_cnt++;
> mppe->coher_cnt &= PIPEX_COHERENCY_CNT_MASK;
> }
> +
> + mtx_leave(>pxm_mtx);
> +
> if (m0->m_pkthdr.len < PIPEX_PPPMINLEN)
> goto drop;
>
> @@ -2387,6 +2404,8 @@ pipex_mppe_output(struct mbuf *m0, struc
> flushed = 0;
> encrypt = 1;
>
> + mtx_enter(>pxm_mtx);
> +
> if (mppe->stateless != 0) {
> flushed = 1;
> mppe_key_change(mppe);
> @@ -2429,6 +2448,8 @@ pipex_mppe_output(struct mbuf *m0, struc
> pipex_mppe_crypt(mppe, len, cp, cp);
> }
>
> + mtx_leave(>pxm_mtx);
>
