Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1637276 > unrolled thread

Re: [RFC v5 8/9] sched/deadline: base GRUB reclaiming on the inactive utilization

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC v5 8/9] sched/deadline: base GRUB reclaiming on the  inactive utilization Luca Abeni <luca.abeni@santannapisa.it> - 2017-05-08 09:50 +0200
    Re: [RFC v5 8/9] sched/deadline: base GRUB reclaiming on the  inactive utilization Peter Zijlstra <peterz@infradead.org> - 2017-05-08 10:20 +0200
      Re: [RFC v5 8/9] sched/deadline: base GRUB reclaiming on the  inactive utilization Luca Abeni <luca.abeni@santannapisa.it> - 2017-05-09 11:40 +0200

#1637276 — Re: [RFC v5 8/9] sched/deadline: base GRUB reclaiming on the inactive utilization

Hi Peter,

sorry for the delay; anyway, I am working on fixing the patchset
according to the comments I received....

When working on one of your comments, I have a doubt:

On Mon, 27 Mar 2017 16:26:33 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
[...]
> 
> 
> #define BW_SHIFT	20
> #define BW_UNIT		(1 << BW_SHIFT)
> 
> static inline
> u64 grub_reclaim(u64 delta, struct rq *rq, struct sched_dl_entity
> *dl_se) {
> u64 u_inact = rq->dl.this_bw - rq->dl.running_bw; /* Utot -
> Uact */ u64 u_act;
[...]

I think introducing the BW_SHIFT and BW_UNIT defines can be more useful
in a previous patch (patch 4, where I introduce the "grub_reclaim()"
function, and use ">> 20" for the first time.

Moreover, the "20" magic number is already used in core.c... Should I
introduce the defines in sched/sched.h, and change the existing core.c
code too? Is it ok to embed this change in patch 4 (sched/deadline:
implement GRUB accounting), or should it go in a separate patch?


			Thanks,
				Luca

> 
> 	/*
>          * What we want to write is:
> 	 *
> 	 *   max(BW_UNIT - u_inact, dl_se->dl_bw)
> 	 *
> 	 * but we cannot do that since Utot can be larger than 1,
> 	 * which means u_inact can be larger than 1, which would
> 	 * have the above result in negative values.
> 	 */
> 	if (u_inact > (BW_UNIT - dl_se->dl_bw))
> 		u_act = dl_se->dl_bw;
> 	else
> 		u_act = BW_UNIT - u_inact;
> 
> 	return (delta * u_act) >> BW_SHIFT;
> }
> 
> Hmm?

[toc] | [next] | [standalone]

#1637296

On Mon, May 08, 2017 at 09:41:08AM +0200, Luca Abeni wrote:
> Hi Peter,
> 
> sorry for the delay; anyway, I am working on fixing the patchset
> according to the comments I received....
> 
> When working on one of your comments, I have a doubt:
> 
> On Mon, 27 Mar 2017 16:26:33 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> [...]
> > 
> > 
> > #define BW_SHIFT	20
> > #define BW_UNIT		(1 << BW_SHIFT)
> > 
> > static inline
> > u64 grub_reclaim(u64 delta, struct rq *rq, struct sched_dl_entity
> > *dl_se) {
> > u64 u_inact = rq->dl.this_bw - rq->dl.running_bw; /* Utot -
> > Uact */ u64 u_act;
> [...]
> 
> I think introducing the BW_SHIFT and BW_UNIT defines can be more useful
> in a previous patch (patch 4, where I introduce the "grub_reclaim()"
> function, and use ">> 20" for the first time.

Sure..

> Moreover, the "20" magic number is already used in core.c... Should I
> introduce the defines in sched/sched.h, and change the existing core.c
> code too?

Yes please.

> Is it ok to embed this change in patch 4 (sched/deadline:
> implement GRUB accounting), or should it go in a separate patch?

Whatever you feel is nicest. Currently the thing is fully contained in
the one to_ratio() function (afaict), so the first patch where you make
it escape would be fine.

[toc] | [prev] | [next] | [standalone]

#1638009

On Mon, 8 May 2017 10:06:13 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
[...]
> > > #define BW_SHIFT	20
> > > #define BW_UNIT		(1 << BW_SHIFT)
> > > 
> > > static inline
> > > u64 grub_reclaim(u64 delta, struct rq *rq, struct sched_dl_entity
> > > *dl_se) {
> > > u64 u_inact = rq->dl.this_bw - rq->dl.running_bw; /* Utot -
> > > Uact */ u64 u_act;  
> > [...]
> > 
> > I think introducing the BW_SHIFT and BW_UNIT defines can be more
> > useful in a previous patch (patch 4, where I introduce the
> > "grub_reclaim()" function, and use ">> 20" for the first time.  
> 
> Sure..
> 
> > Moreover, the "20" magic number is already used in core.c... Should
> > I introduce the defines in sched/sched.h, and change the existing
> > core.c code too?  
> 
> Yes please.
> 
> > Is it ok to embed this change in patch 4 (sched/deadline:
> > implement GRUB accounting), or should it go in a separate patch?  
> 
> Whatever you feel is nicest. Currently the thing is fully contained in
> the one to_ratio() function (afaict), so the first patch where you
> make it escape would be fine.

Ok, thanks. I included this change in patch 4 (the first one using the
"20") and I put the two defines in sched.h, immediately before the
prototype of to_ratio().

I am finishing with the requested changes, and after some days of
testing I'll post a new patchset.


			Thanks,
				Luca

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web