From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jason Wang Subject: Re: [PATCH v2] net-tun: restructure tun_do_read for better sleep/wakeup efficiency Date: Tue, 20 May 2014 14:55:13 +0800 Message-ID: <537AFC51.30103@redhat.com> References: <537AF036.7010005@redhat.com> <20140520063403.GB6653@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Eric Dumazet , Xi Wang , "David S. Miller" , netdev@vger.kernel.org, Maxim Krasnyansky , Neal Cardwell , Eric Dumazet To: "Michael S. Tsirkin" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:33435 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751685AbaETGzU (ORCPT ); Tue, 20 May 2014 02:55:20 -0400 In-Reply-To: <20140520063403.GB6653@redhat.com> Sender: netdev-owner@vger.kernel.org List-ID: On 05/20/2014 02:34 PM, Michael S. Tsirkin wrote: > On Tue, May 20, 2014 at 02:03:34PM +0800, Jason Wang wrote: >> On 05/20/2014 01:11 PM, Eric Dumazet wrote: >>> On Tue, 2014-05-20 at 12:44 +0800, Jason Wang wrote: >>>> On 05/19/2014 10:09 PM, Eric Dumazet wrote: >>>>> About the sk_data_ready() and wake_up_all(), you missed the whole part >>>>> of the patch I think. >>>>> >>>>> Check how sock_def_readable() does everything properly and efficiently, >>>>> including the async part. >>>> But this changes (sk_data_ready()) has nothing related to switching to >>>> use __skb_recv_datagram() >>>> >>> This is totally related. >>> >>> I think you did not yet understood this patch >> Sorry for being unclear, but I think you misunderstand my meaning. >>> Compare wake_up_all() and sk_data_ready() speeds, you'll be surprised. >>> >>> You should ask to yourself : Why do we use wq_has_sleeper() in >>> networking stacks ? >> See my first reply, I don't have objection that uses sk_data_ready() in >> tun_net_xmit(). My only concern is using sk_data_ready() in >> tun_detach_all(): >> >> - It was only called during tun destroying, so I believe we don't care >> about the performance in this condition. >> - sk_data_ready() was usually called when there's something new to be >> processed which is not case in tun_detach_all() > OK so what does userspace do to notice change in behaviour? > I don't ask that you write a test but can you show us in > pseudo-code? > > Not currently but possible in the future. >> - Not sure it was a problem but sock_def_readable() will not wake up >> uninterruptible task during tun destroying. > But task_uninterruptible here would really mean some in-kernel caller > hooking into this function? Yes. > is there a way to create this from > userspace? If not we don't care. I think not. >> - If we make sock_fasync() works for tun in the future, it may send >> SIGIO to user process during tun destroying which is not expected. >> >> Thanks > I don't get this last comment. The patch does not touch fasync paths > at all. How can it break them? sock_def_readable() will call sk_wake_async() which may send SIGIO to userspace if SOCK_FASYNC were set for socket. But currently tun has its own implementation of fasync, so this can not happen. But if we want to switch to use sock_fasync() for tun in the future, this issue may appear.