From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Gleixner Subject: Re: [RFC v3 net-next 13/18] net/sched: Introduce the TBS Qdisc Date: Tue, 24 Apr 2018 10:50:04 +0200 (CEST) Message-ID: References: <20180307011230.24001-1-jesus.sanchez-palencia@intel.com> <20180307011230.24001-14-jesus.sanchez-palencia@intel.com> <768e8da5-502e-d36f-0f32-9324eaca4a1d@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Cc: netdev@vger.kernel.org, jhs@mojatatu.com, xiyou.wangcong@gmail.com, jiri@resnulli.us, vinicius.gomes@intel.com, richardcochran@gmail.com, intel-wired-lan@lists.osuosl.org, anna-maria@linutronix.de, henrik@austad.us, john.stultz@linaro.org, levi.pearson@harman.com, edumazet@google.com, willemb@google.com, mlichvar@redhat.com To: Jesus Sanchez-Palencia Return-path: Received: from Galois.linutronix.de ([146.0.238.70]:34501 "EHLO Galois.linutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755957AbeDXIuT (ORCPT ); Tue, 24 Apr 2018 04:50:19 -0400 In-Reply-To: <768e8da5-502e-d36f-0f32-9324eaca4a1d@intel.com> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, 23 Apr 2018, Jesus Sanchez-Palencia wrote: > On 03/21/2018 06:46 AM, Thomas Gleixner wrote: > > On Tue, 6 Mar 2018, Jesus Sanchez-Palencia wrote: > >> +struct tbs_sched_data { > >> + bool sorting; > >> + int clockid; > >> + int queue; > >> + s32 delta; /* in ns */ > >> + ktime_t last; /* The txtime of the last skb sent to the netdevice. */ > >> + struct rb_root head; > > > > Hmm. You are reimplementing timerqueue open coded. Have you checked whether > > you could reuse the timerqueue implementation? > > > > That requires to add a timerqueue node to struct skbuff > > > > @@ -671,7 +671,8 @@ struct sk_buff { > > unsigned long dev_scratch; > > }; > > }; > > - struct rb_node rbnode; /* used in netem & tcp stack */ > > + struct rb_node rbnode; /* used in netem & tcp stack */ > > + struct timerqueue_node tqnode; > > }; > > struct sock *sk; > > > > Then you can use timerqueue_head in your scheduler data and all the open > > coded rbtree handling goes away. > > > I just noticed that doing the above increases the size of struct sk_buff by 8 > bytes - struct timerqueue_node is 32bytes long while struct rb_node is only > 24bytes long. > > Given the feedback we got here before against touching struct sk_buff at all for > non-generic use cases, I will keep the implementation of sch_tbs.c as is, thus > keeping the open-coded version for now, ok? The size of sk_buff is 216 and the size of sk_buff_fclones is 440 bytes. The sk_buff and sk_buff_fclones kmem_caches use objects sized 256 and 512 bytes because the kmem_caches are created with SLAB_HWCACHE_ALIGN. So adding 8 bytes to spare duplicated code will not change the kmem_cache object size and I really doubt that anyone will notice. Thanks, tglx From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Gleixner Date: Tue, 24 Apr 2018 10:50:04 +0200 (CEST) Subject: [Intel-wired-lan] [RFC v3 net-next 13/18] net/sched: Introduce the TBS Qdisc In-Reply-To: <768e8da5-502e-d36f-0f32-9324eaca4a1d@intel.com> References: <20180307011230.24001-1-jesus.sanchez-palencia@intel.com> <20180307011230.24001-14-jesus.sanchez-palencia@intel.com> <768e8da5-502e-d36f-0f32-9324eaca4a1d@intel.com> Message-ID: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: intel-wired-lan@osuosl.org List-ID: On Mon, 23 Apr 2018, Jesus Sanchez-Palencia wrote: > On 03/21/2018 06:46 AM, Thomas Gleixner wrote: > > On Tue, 6 Mar 2018, Jesus Sanchez-Palencia wrote: > >> +struct tbs_sched_data { > >> + bool sorting; > >> + int clockid; > >> + int queue; > >> + s32 delta; /* in ns */ > >> + ktime_t last; /* The txtime of the last skb sent to the netdevice. */ > >> + struct rb_root head; > > > > Hmm. You are reimplementing timerqueue open coded. Have you checked whether > > you could reuse the timerqueue implementation? > > > > That requires to add a timerqueue node to struct skbuff > > > > @@ -671,7 +671,8 @@ struct sk_buff { > > unsigned long dev_scratch; > > }; > > }; > > - struct rb_node rbnode; /* used in netem & tcp stack */ > > + struct rb_node rbnode; /* used in netem & tcp stack */ > > + struct timerqueue_node tqnode; > > }; > > struct sock *sk; > > > > Then you can use timerqueue_head in your scheduler data and all the open > > coded rbtree handling goes away. > > > I just noticed that doing the above increases the size of struct sk_buff by 8 > bytes - struct timerqueue_node is 32bytes long while struct rb_node is only > 24bytes long. > > Given the feedback we got here before against touching struct sk_buff at all for > non-generic use cases, I will keep the implementation of sch_tbs.c as is, thus > keeping the open-coded version for now, ok? The size of sk_buff is 216 and the size of sk_buff_fclones is 440 bytes. The sk_buff and sk_buff_fclones kmem_caches use objects sized 256 and 512 bytes because the kmem_caches are created with SLAB_HWCACHE_ALIGN. So adding 8 bytes to spare duplicated code will not change the kmem_cache object size and I really doubt that anyone will notice. Thanks, tglx