On Wed, 18 Nov 2020 02:47:24 +0000 Vadim Fedorenko wrote: > >>>> This behavior differs from plain TCP socket and > >>>> leads to special treating in user-space. Patch unpauses parser > >>>> directly if we have unparsed data in tcp receive queue. > >>> Sure, but why is the parser paused? Does it pause itself on FIN? > >> No, it doesn't start even once. The trace looks like: > >> > >> tcp_recvmsg is called > >> tcp_recvmsg returns 1 (Change Cipher Spec record data) > >> tls_setsockopt is called > >> tls_setsockopt returns > >> tls_recvmsg is called > >> tls_recvmsg returns 0 > >> __strp_recv is called > >> stack > >> __strp_recv+1 > >> tcp_read_sock+169 > >> strp_read_sock+104 > >> strp_work+68 > >> process_one_work+436 > >> worker_thread+80 > >> kthread+276 > >> ret_from_fork+34tls_read_size called > >> > >> So it looks like strp_work was scheduled after tls_recvmsg and > >> nothing triggered parser because all the data was received before > >> tls_setsockopt ended the configuration process. > > Um. That makes me think we need to flush_work() on the strparser after > > we configure rx tls, no? Or __unpause at the right time instead of > > dealing with the async nature of strp_check_rcv()? > I'm not sure that flush_work() will do right way in this case. Because: > 1. The work is idle after tls_sw_strparser_arm()
Not sure what you mean by idle, it queues the work via strp_check_rcv(). > 2. The strparser needs socket lock to do it's work - that could be a > deadlock because setsockopt_conf already holds socket lock. I'm not > sure that it's a good idea to unlock socket just to wait the strparser. Ack, I meant in do_tls_setsockopt() after the lock is released. > The async nature of parser is OK for classic HTTPS server/client case > because it's very good to have parsed record before actual call to recvmsg > or splice_read is done. The code inside the loop in tls_wait_data is slow > path - maybe just move the check and the __unpause in this slow path? Yeah, looking closer this problem can arise after we start as well :/ How about we move the __unparse code into the loop tho? Seems like this could help with latency. Right now AFAICT we get a TCP socket ready notification, which wakes the waiter for no good reason and makes strparser queue its work, the work then will call tls_queue() -> data_ready waking the waiting thread second time this time with the record actually parsed. Did I get that right? Should the waiter not cancel the work on the first wake up and just kick of the parsing itself?
