On Thu, 30 Jul 2020 22:43:10 +0300
Vitaliy Makkoveev <[email protected]> wrote:
> On Thu, Jul 30, 2020 at 10:05:13PM +0900, YASUOKA Masahiko wrote:
>> On Thu, 30 Jul 2020 15:34:09 +0300
>> Vitaliy Makkoveev <[email protected]> wrote:
>> > On Thu, Jul 30, 2020 at 09:13:46PM +0900, YASUOKA Masahiko wrote:
>> >> Hi,
>> >>
>> >> sys/net/if_ethersubr.c:
>> >> 372 void
>> >> 373 ether_input(struct ifnet *ifp, struct mbuf *m)
>> >> (snip)
>> >> 519 #if NPPPOE > 0 || defined(PIPEX)
>> >> 520 case ETHERTYPE_PPPOEDISC:
>> >> 521 case ETHERTYPE_PPPOE:
>> >> 522 if (m->m_flags & (M_MCAST | M_BCAST))
>> >> 523 goto dropanyway;
>> >> 524 #ifdef PIPEX
>> >> 525 if (pipex_enable) {
>> >> 526 struct pipex_session *session;
>> >> 527
>> >> 528 if ((session = pipex_pppoe_lookup_session(m))
>> >> != NULL) {
>> >> 529 pipex_pppoe_input(m, session);
>> >> 530 return;
>> >> 531 }
>> >> 532 }
>> >> 533 #endif
>> >>
>> >> previously a packet which branchces to #529 is enqueued.
>> >>
>> >> If the diff removes the queue, then the pipex input routine is
>> >> executed by the NIC's interrupt handler.
>> >>
>> >> The queues had been made to avoid that kind of situations.
>> >
>> > It's not enqueued in pppoe case. According pipex_pppoe_input() code we
>> > call pipex_common_input() with `useq' argument set to '0', so we don't
>> > enqueue mbuf(9) but pass it to pipex_ppp_input() which will pass it to
>> > ipv{4,6}_input().
>>
>> You are right. Sorry, I forgot about this which I did that by myself.
>>
>
> I'm interesting the reason why you did that.
>
>> >> Also I don't see a relation of the use-after-free problem and killing
>> >> queues. Can't we fix the problem unless we kill the queues?
>> >
>> > Yes we can. Reference counters allow us to keep orphan sessions in these
>> > queues without use after free issue.
>> >
>> > I will wait your commentaries current enqueuing before to do something.
>>
>> I have another concern.
>>
>> You might know, when L2TP/IPsec is used heavily, the crypto thread
>> uses 100% of 1 CPU core. In that case, that thread becomes like
>> below:
>>
>> crypto thread -> udp_userreq -> pipex_l2tp_input
>>
>> some clients are using MPPE(RC4 encryption) on CCP. It's not so
>> light.
>>
>> How do we offload this for CPUs? I am thinking that "pipex" can have
>> a dedicated thread. Do we have another scenario?
>>
>
> I suppose you mean udp_input(). What is you call "crypto thread"? I did
> a little backtrace but I didn't find this thread.
>
> ether_resolve
> if_input_local
> ipv4_input
> ip_input_if
> ip_ours
> ip_deliver
> udp_input (through pr_input)
> pipex_l2tp_input
>
> ipi{,6}_mloopback
> if_input_local
> ipv4_input
> ...
> udp_input (through pr_input)
> pipex_l2tp_input
>
> loinput
> if_input_local
> ipv4_input
> ...
> udp_input (through pr_input)
> pipex_l2tp_input
>
> Also various pseudo drivers call ipv{4,6}_input() and underlay
> udp_unput() too.
>
> Except nfs, we call udp_usrreq() through socket layer only. Do you mean
> userland as "crypto thread"?
Sorry, udp_usrreq() should be usr_input() and crypto thread meant a
kthread for crypto_taskq_mp_safe, whose name is "crynlk" (see
crypto_init()).
A packet of L2TP/IPsec (encapsulated IP/PPP/L2TP/UDP/ESP/UDP/IP) is
processed like:
ipv4_input
...
udp_input
ipsec_common_input
esp_input
crypto_dispatch
=> crypto_taskq_mp_safe
kthread "crynlk"
crypto_invoke
... (*1)
crypto_done
esp_input_cb
ipsec_common_input_cb
ip_deliver
udp_input
pipex_l2tp_input
pipex_common_input
(*2)
pipex_ppp_input
pipex_mppe_input (*3)
pipex_ppp_input
pipex_ip_input
ipv4_input
...
At *2 there was a queue. "crynlk" is a busy thread, since it is doing
decryption at *1. I think it's better pipex input is be done by
another thread than crypto since it also has decryption at *3.
> But upd_input(), udp_usrreq() and pipexintr() are serialized by
> NET_LOCK(). We should move pipex(4) under it's own rwlock to allow them
> simultaneous execution. Also, should we move outgoing pppx(4) mbufs to
> queue too?
pppx has a dedicated queue for each pppx interface. I think that is a
reason why it skips pipexoutq.
Also I noticed there is a generic queue for IP.
pipex_ppp_output
pipex_l2tp_output
ipsend()
=> net_tq
For PPPoE,
pipex_ppp_output
pipex_pppoe_output
ipsend()
=> ifp->if_output
Currently it seems there is enough queues for output side.
--yasuoka
