mbox series

[00/12] unify the interface of the proportional-share policy in blkio/io

Message ID 20181112095632.69114-1-paolo.valente@linaro.org
Headers show
Series unify the interface of the proportional-share policy in blkio/io | expand

Message

Paolo Valente Nov. 12, 2018, 9:56 a.m. UTC
Hi Jens, Tejun, all,
about nine months ago, we agreed on a solution for unifying the
interface of the proportional-share policy in blkio/io [1].  Angelo
and I finally completed it.  Let me briefly recall the problem and the
solution.

The current implementation of cgroups doesn't allow two or more
entities, e.g., I/O schedulers, to share the same files.  So, if CFQ
creates its files for the proportional-share policy, such as, e.g,
weight files for blkio/io groups, BFQ cannot attach somehow to them.
Thus, to enable people to set group weights with BFQ, I resorted to
making BFQ create its own version of these common files, by prepending
a bfq prefix.

Actually, no legacy code uses these different names, or is likely to
do so.  Having these two sets of names is simply a source of
confusion, as pointed out also, e.g., by Lennart Poettering (CCed
here), and acknowledged by Tejun [2].

In [1] we agreed on a solution that solves this problem, by actually
making it possible to share cgroups files.  Both writing to and
reading from a shared file trigger the appropriate operation for each
of the entities that share the file.  In particular, in case of
reading,
- if all entities produce the same output, the this common output is
  shown only once;
- if the outputs differ, then every per-entity output is shown,
  preceded by the name of the entity that produced that output.

With this solution, legacy code that, e.g., sets group weights, just
works, regardless of the I/O scheduler actually implementing
proportional share.

But note that this extension is not restricted to only blkio/io.  The
general group interface now enables files to be shared among multiple
entities of any kind.

(I have also added a patch to fix some clerical errors in bfq doc,
which I found while making the latter consistent with the new
interface.)

Thanks,
Paolo

[1] https://lkml.org/lkml/2018/1/4/667
[2] https://github.com/systemd/systemd/issues/7057

Angelo Ruocco (7):
  kernfs: add function to find kernfs_node without increasing ref
    counter
  cgroup: link cftypes of the same subsystem with the same name
  cgroup: add owner name to cftypes
  block, bfq: align min and default weights with cfq
  cgroup: make all functions of all cftypes be invoked
  block, cfq: allow cgroup files to be shared
  block, throttle: allow sharing cgroup statistic files

Paolo Valente (5):
  cgroup: add hook seq_show_cft with also the owning cftype as parameter
  block, cgroup: pass cftype to functions that need to use it
  block, bfq: use standard file names for the proportional-share policy
  doc, bfq-iosched: fix a few clerical errors
  doc, bfq-iosched: make it consistent with the new cgroup interface

 Documentation/block/bfq-iosched.txt |  31 +++--
 block/bfq-cgroup.c                  | 148 +++++++++++++-------
 block/bfq-iosched.h                 |   4 +-
 block/blk-cgroup.c                  |  22 +--
 block/blk-throttle.c                |  24 ++--
 block/cfq-iosched.c                 | 105 +++++++++++----
 fs/kernfs/dir.c                     |  13 ++
 include/linux/blk-cgroup.h          |  10 +-
 include/linux/cgroup-defs.h         |  14 +-
 include/linux/cgroup.h              |  13 ++
 include/linux/kernfs.h              |   7 +
 kernel/cgroup/cgroup.c              | 262 +++++++++++++++++++++++++++++-------
 12 files changed, 483 insertions(+), 170 deletions(-)

--
2.16.1

Comments

Paolo Valente Nov. 12, 2018, 9:58 a.m. UTC | #1
Forgot to CC Lennart, sorry.

> Il giorno 12 nov 2018, alle ore 10:56, Paolo Valente <paolo.valente@linaro.org> ha scritto:

> 

> Hi Jens, Tejun, all,

> about nine months ago, we agreed on a solution for unifying the

> interface of the proportional-share policy in blkio/io [1].  Angelo

> and I finally completed it.  Let me briefly recall the problem and the

> solution.

> 

> The current implementation of cgroups doesn't allow two or more

> entities, e.g., I/O schedulers, to share the same files.  So, if CFQ

> creates its files for the proportional-share policy, such as, e.g,

> weight files for blkio/io groups, BFQ cannot attach somehow to them.

> Thus, to enable people to set group weights with BFQ, I resorted to

> making BFQ create its own version of these common files, by prepending

> a bfq prefix.

> 

> Actually, no legacy code uses these different names, or is likely to

> do so.  Having these two sets of names is simply a source of

> confusion, as pointed out also, e.g., by Lennart Poettering (CCed

> here), and acknowledged by Tejun [2].

> 

> In [1] we agreed on a solution that solves this problem, by actually

> making it possible to share cgroups files.  Both writing to and

> reading from a shared file trigger the appropriate operation for each

> of the entities that share the file.  In particular, in case of

> reading,

> - if all entities produce the same output, the this common output is

>  shown only once;

> - if the outputs differ, then every per-entity output is shown,

>  preceded by the name of the entity that produced that output.

> 

> With this solution, legacy code that, e.g., sets group weights, just

> works, regardless of the I/O scheduler actually implementing

> proportional share.

> 

> But note that this extension is not restricted to only blkio/io.  The

> general group interface now enables files to be shared among multiple

> entities of any kind.

> 

> (I have also added a patch to fix some clerical errors in bfq doc,

> which I found while making the latter consistent with the new

> interface.)

> 

> Thanks,

> Paolo

> 

> [1] https://lkml.org/lkml/2018/1/4/667

> [2] https://github.com/systemd/systemd/issues/7057

> 

> Angelo Ruocco (7):

>  kernfs: add function to find kernfs_node without increasing ref

>    counter

>  cgroup: link cftypes of the same subsystem with the same name

>  cgroup: add owner name to cftypes

>  block, bfq: align min and default weights with cfq

>  cgroup: make all functions of all cftypes be invoked

>  block, cfq: allow cgroup files to be shared

>  block, throttle: allow sharing cgroup statistic files

> 

> Paolo Valente (5):

>  cgroup: add hook seq_show_cft with also the owning cftype as parameter

>  block, cgroup: pass cftype to functions that need to use it

>  block, bfq: use standard file names for the proportional-share policy

>  doc, bfq-iosched: fix a few clerical errors

>  doc, bfq-iosched: make it consistent with the new cgroup interface

> 

> Documentation/block/bfq-iosched.txt |  31 +++--

> block/bfq-cgroup.c                  | 148 +++++++++++++-------

> block/bfq-iosched.h                 |   4 +-

> block/blk-cgroup.c                  |  22 +--

> block/blk-throttle.c                |  24 ++--

> block/cfq-iosched.c                 | 105 +++++++++++----

> fs/kernfs/dir.c                     |  13 ++

> include/linux/blk-cgroup.h          |  10 +-

> include/linux/cgroup-defs.h         |  14 +-

> include/linux/cgroup.h              |  13 ++

> include/linux/kernfs.h              |   7 +

> kernel/cgroup/cgroup.c              | 262 +++++++++++++++++++++++++++++-------

> 12 files changed, 483 insertions(+), 170 deletions(-)

> 

> --

> 2.16.1
Paolo Valente Nov. 12, 2018, 10:17 a.m. UTC | #2
> Il giorno 12 nov 2018, alle ore 11:00, Oleksandr Natalenko <oleksandr@natalenko.name> ha scritto:

> 

> On 12.11.2018 10:56, Paolo Valente wrote:

>> Hi Jens, Tejun, all,

>> about nine months ago, we agreed on a solution for unifying the

>> interface of the proportional-share policy in blkio/io [1].  Angelo

>> and I finally completed it.  Let me briefly recall the problem and the

>> solution.

>> The current implementation of cgroups doesn't allow two or more

>> entities, e.g., I/O schedulers, to share the same files.  So, if CFQ

>> creates its files for the proportional-share policy, such as, e.g,

>> weight files for blkio/io groups, BFQ cannot attach somehow to them.

>> Thus, to enable people to set group weights with BFQ, I resorted to

>> making BFQ create its own version of these common files, by prepending

>> a bfq prefix.

>> Actually, no legacy code uses these different names, or is likely to

>> do so.  Having these two sets of names is simply a source of

>> confusion, as pointed out also, e.g., by Lennart Poettering (CCed

>> here), and acknowledged by Tejun [2].

>> In [1] we agreed on a solution that solves this problem, by actually

>> making it possible to share cgroups files.  Both writing to and

>> reading from a shared file trigger the appropriate operation for each

>> of the entities that share the file.  In particular, in case of

>> reading,

>> - if all entities produce the same output, the this common output is

>>  shown only once;

>> - if the outputs differ, then every per-entity output is shown,

>>  preceded by the name of the entity that produced that output.

>> With this solution, legacy code that, e.g., sets group weights, just

>> works, regardless of the I/O scheduler actually implementing

>> proportional share.

>> But note that this extension is not restricted to only blkio/io.  The

>> general group interface now enables files to be shared among multiple

>> entities of any kind.

>> (I have also added a patch to fix some clerical errors in bfq doc,

>> which I found while making the latter consistent with the new

>> interface.)

>> Thanks,

>> Paolo

>> [1] https://lkml.org/lkml/2018/1/4/667

>> [2] https://github.com/systemd/systemd/issues/7057

>> Angelo Ruocco (7):

>>  kernfs: add function to find kernfs_node without increasing ref

>>    counter

>>  cgroup: link cftypes of the same subsystem with the same name

>>  cgroup: add owner name to cftypes

>>  block, bfq: align min and default weights with cfq

>>  cgroup: make all functions of all cftypes be invoked

>>  block, cfq: allow cgroup files to be shared

>>  block, throttle: allow sharing cgroup statistic files

>> Paolo Valente (5):

>>  cgroup: add hook seq_show_cft with also the owning cftype as parameter

>>  block, cgroup: pass cftype to functions that need to use it

>>  block, bfq: use standard file names for the proportional-share policy

>>  doc, bfq-iosched: fix a few clerical errors

>>  doc, bfq-iosched: make it consistent with the new cgroup interface

>> Documentation/block/bfq-iosched.txt |  31 +++--

>> block/bfq-cgroup.c                  | 148 +++++++++++++-------

>> block/bfq-iosched.h                 |   4 +-

>> block/blk-cgroup.c                  |  22 +--

>> block/blk-throttle.c                |  24 ++--

>> block/cfq-iosched.c                 | 105 +++++++++++----

>> fs/kernfs/dir.c                     |  13 ++

>> include/linux/blk-cgroup.h          |  10 +-

>> include/linux/cgroup-defs.h         |  14 +-

>> include/linux/cgroup.h              |  13 ++

>> include/linux/kernfs.h              |   7 +

>> kernel/cgroup/cgroup.c              | 262 +++++++++++++++++++++++++++++-------

>> 12 files changed, 483 insertions(+), 170 deletions(-)

>> --

>> 2.16.1

> 

> I thought all the legacy stuff including CFS et al. is going to be removed in v4.21 completely…

> 


Thanks for pointing this out.

People with a lower kernel version than the future 4.21 just cannot
and will not be able to use the proportional share policy on blk-mq
(with legacy code), because of the name issue highlighted in this
email.  If this patch series gets accepted, a backport will solve the
problem.  In this respect, such a backport might even happen
'automatically', as most bfq commit seem to get backported to older,
stable kernels.

In addition, this extension
- extends the whole cgroups interface, in a seamless and
  backward-compatible way, to prevent future issues like these;
- solves a similar issue with throttle (which AFAIK won't go away
  with 4.21).

Thanks,
Paolo

> -- 

>  Oleksandr Natalenko (post-factum)
Jens Axboe Nov. 12, 2018, 3:35 p.m. UTC | #3
On 11/12/18 3:17 AM, Paolo Valente wrote:
> 

> 

>> Il giorno 12 nov 2018, alle ore 11:00, Oleksandr Natalenko <oleksandr@natalenko.name> ha scritto:

>>

>> On 12.11.2018 10:56, Paolo Valente wrote:

>>> Hi Jens, Tejun, all,

>>> about nine months ago, we agreed on a solution for unifying the

>>> interface of the proportional-share policy in blkio/io [1].  Angelo

>>> and I finally completed it.  Let me briefly recall the problem and the

>>> solution.

>>> The current implementation of cgroups doesn't allow two or more

>>> entities, e.g., I/O schedulers, to share the same files.  So, if CFQ

>>> creates its files for the proportional-share policy, such as, e.g,

>>> weight files for blkio/io groups, BFQ cannot attach somehow to them.

>>> Thus, to enable people to set group weights with BFQ, I resorted to

>>> making BFQ create its own version of these common files, by prepending

>>> a bfq prefix.

>>> Actually, no legacy code uses these different names, or is likely to

>>> do so.  Having these two sets of names is simply a source of

>>> confusion, as pointed out also, e.g., by Lennart Poettering (CCed

>>> here), and acknowledged by Tejun [2].

>>> In [1] we agreed on a solution that solves this problem, by actually

>>> making it possible to share cgroups files.  Both writing to and

>>> reading from a shared file trigger the appropriate operation for each

>>> of the entities that share the file.  In particular, in case of

>>> reading,

>>> - if all entities produce the same output, the this common output is

>>>  shown only once;

>>> - if the outputs differ, then every per-entity output is shown,

>>>  preceded by the name of the entity that produced that output.

>>> With this solution, legacy code that, e.g., sets group weights, just

>>> works, regardless of the I/O scheduler actually implementing

>>> proportional share.

>>> But note that this extension is not restricted to only blkio/io.  The

>>> general group interface now enables files to be shared among multiple

>>> entities of any kind.

>>> (I have also added a patch to fix some clerical errors in bfq doc,

>>> which I found while making the latter consistent with the new

>>> interface.)

>>> Thanks,

>>> Paolo

>>> [1] https://lkml.org/lkml/2018/1/4/667

>>> [2] https://github.com/systemd/systemd/issues/7057

>>> Angelo Ruocco (7):

>>>  kernfs: add function to find kernfs_node without increasing ref

>>>    counter

>>>  cgroup: link cftypes of the same subsystem with the same name

>>>  cgroup: add owner name to cftypes

>>>  block, bfq: align min and default weights with cfq

>>>  cgroup: make all functions of all cftypes be invoked

>>>  block, cfq: allow cgroup files to be shared

>>>  block, throttle: allow sharing cgroup statistic files

>>> Paolo Valente (5):

>>>  cgroup: add hook seq_show_cft with also the owning cftype as parameter

>>>  block, cgroup: pass cftype to functions that need to use it

>>>  block, bfq: use standard file names for the proportional-share policy

>>>  doc, bfq-iosched: fix a few clerical errors

>>>  doc, bfq-iosched: make it consistent with the new cgroup interface

>>> Documentation/block/bfq-iosched.txt |  31 +++--

>>> block/bfq-cgroup.c                  | 148 +++++++++++++-------

>>> block/bfq-iosched.h                 |   4 +-

>>> block/blk-cgroup.c                  |  22 +--

>>> block/blk-throttle.c                |  24 ++--

>>> block/cfq-iosched.c                 | 105 +++++++++++----

>>> fs/kernfs/dir.c                     |  13 ++

>>> include/linux/blk-cgroup.h          |  10 +-

>>> include/linux/cgroup-defs.h         |  14 +-

>>> include/linux/cgroup.h              |  13 ++

>>> include/linux/kernfs.h              |   7 +

>>> kernel/cgroup/cgroup.c              | 262 +++++++++++++++++++++++++++++-------

>>> 12 files changed, 483 insertions(+), 170 deletions(-)

>>> --

>>> 2.16.1

>>

>> I thought all the legacy stuff including CFS et al. is going to be removed in v4.21 completely…

>>

> 

> Thanks for pointing this out.

> 

> People with a lower kernel version than the future 4.21 just cannot

> and will not be able to use the proportional share policy on blk-mq

> (with legacy code), because of the name issue highlighted in this

> email.  If this patch series gets accepted, a backport will solve the

> problem.  In this respect, such a backport might even happen

> 'automatically', as most bfq commit seem to get backported to older,

> stable kernels.

> 

> In addition, this extension

> - extends the whole cgroups interface, in a seamless and

>   backward-compatible way, to prevent future issues like these;

> - solves a similar issue with throttle (which AFAIK won't go away

>   with 4.21).


There's no way this series can get accepted, since you've made the
mistake of basing it on something that won't apply to the block
tree for 4.21. I've outlined these rules before, but here they are
again:

1) Patches destined for the CURRENT kernel version should be
   against my for-linus branch. That means that right now, any
   patches that should to into 4.20 should be against that.

2) Patches destined for the NEXT kernel version should be against
   my for-x.y/block branch, where x.y is the next version. As of
   right now, patches for 4.21 should be against my for-4.21/bloc
   branch.

I'd encourage you to respin against that, particularly in this case
since we've both got a lot of churn, and also removal of various
items that you are patching here.

-- 
Jens Axboe
Paolo Valente Nov. 12, 2018, 3:45 p.m. UTC | #4
> Il giorno 12 nov 2018, alle ore 16:35, Jens Axboe <axboe@kernel.dk> ha scritto:

> 

> On 11/12/18 3:17 AM, Paolo Valente wrote:

>> 

>> 

>>> Il giorno 12 nov 2018, alle ore 11:00, Oleksandr Natalenko <oleksandr@natalenko.name> ha scritto:

>>> 

>>> On 12.11.2018 10:56, Paolo Valente wrote:

>>>> Hi Jens, Tejun, all,

>>>> about nine months ago, we agreed on a solution for unifying the

>>>> interface of the proportional-share policy in blkio/io [1].  Angelo

>>>> and I finally completed it.  Let me briefly recall the problem and the

>>>> solution.

>>>> The current implementation of cgroups doesn't allow two or more

>>>> entities, e.g., I/O schedulers, to share the same files.  So, if CFQ

>>>> creates its files for the proportional-share policy, such as, e.g,

>>>> weight files for blkio/io groups, BFQ cannot attach somehow to them.

>>>> Thus, to enable people to set group weights with BFQ, I resorted to

>>>> making BFQ create its own version of these common files, by prepending

>>>> a bfq prefix.

>>>> Actually, no legacy code uses these different names, or is likely to

>>>> do so.  Having these two sets of names is simply a source of

>>>> confusion, as pointed out also, e.g., by Lennart Poettering (CCed

>>>> here), and acknowledged by Tejun [2].

>>>> In [1] we agreed on a solution that solves this problem, by actually

>>>> making it possible to share cgroups files.  Both writing to and

>>>> reading from a shared file trigger the appropriate operation for each

>>>> of the entities that share the file.  In particular, in case of

>>>> reading,

>>>> - if all entities produce the same output, the this common output is

>>>> shown only once;

>>>> - if the outputs differ, then every per-entity output is shown,

>>>> preceded by the name of the entity that produced that output.

>>>> With this solution, legacy code that, e.g., sets group weights, just

>>>> works, regardless of the I/O scheduler actually implementing

>>>> proportional share.

>>>> But note that this extension is not restricted to only blkio/io.  The

>>>> general group interface now enables files to be shared among multiple

>>>> entities of any kind.

>>>> (I have also added a patch to fix some clerical errors in bfq doc,

>>>> which I found while making the latter consistent with the new

>>>> interface.)

>>>> Thanks,

>>>> Paolo

>>>> [1] https://lkml.org/lkml/2018/1/4/667

>>>> [2] https://github.com/systemd/systemd/issues/7057

>>>> Angelo Ruocco (7):

>>>> kernfs: add function to find kernfs_node without increasing ref

>>>>   counter

>>>> cgroup: link cftypes of the same subsystem with the same name

>>>> cgroup: add owner name to cftypes

>>>> block, bfq: align min and default weights with cfq

>>>> cgroup: make all functions of all cftypes be invoked

>>>> block, cfq: allow cgroup files to be shared

>>>> block, throttle: allow sharing cgroup statistic files

>>>> Paolo Valente (5):

>>>> cgroup: add hook seq_show_cft with also the owning cftype as parameter

>>>> block, cgroup: pass cftype to functions that need to use it

>>>> block, bfq: use standard file names for the proportional-share policy

>>>> doc, bfq-iosched: fix a few clerical errors

>>>> doc, bfq-iosched: make it consistent with the new cgroup interface

>>>> Documentation/block/bfq-iosched.txt |  31 +++--

>>>> block/bfq-cgroup.c                  | 148 +++++++++++++-------

>>>> block/bfq-iosched.h                 |   4 +-

>>>> block/blk-cgroup.c                  |  22 +--

>>>> block/blk-throttle.c                |  24 ++--

>>>> block/cfq-iosched.c                 | 105 +++++++++++----

>>>> fs/kernfs/dir.c                     |  13 ++

>>>> include/linux/blk-cgroup.h          |  10 +-

>>>> include/linux/cgroup-defs.h         |  14 +-

>>>> include/linux/cgroup.h              |  13 ++

>>>> include/linux/kernfs.h              |   7 +

>>>> kernel/cgroup/cgroup.c              | 262 +++++++++++++++++++++++++++++-------

>>>> 12 files changed, 483 insertions(+), 170 deletions(-)

>>>> --

>>>> 2.16.1

>>> 

>>> I thought all the legacy stuff including CFS et al. is going to be removed in v4.21 completely…

>>> 

>> 

>> Thanks for pointing this out.

>> 

>> People with a lower kernel version than the future 4.21 just cannot

>> and will not be able to use the proportional share policy on blk-mq

>> (with legacy code), because of the name issue highlighted in this

>> email.  If this patch series gets accepted, a backport will solve the

>> problem.  In this respect, such a backport might even happen

>> 'automatically', as most bfq commit seem to get backported to older,

>> stable kernels.

>> 

>> In addition, this extension

>> - extends the whole cgroups interface, in a seamless and

>>  backward-compatible way, to prevent future issues like these;

>> - solves a similar issue with throttle (which AFAIK won't go away

>>  with 4.21).

> 

> There's no way this series can get accepted, since you've made the

> mistake of basing it on something that won't apply to the block

> tree for 4.21.


Of course, sorry :(

We'll rebase V2.

BTW, since this patch series is probably even more useful for older
than for future kernels, might it make sense to also propose it for
stable/longterm kernels (provided that such a possibility exists)?

Thanks,
Paolo

> I've outlined these rules before, but here they are

> again:

> 

> 1) Patches destined for the CURRENT kernel version should be

>   against my for-linus branch. That means that right now, any

>   patches that should to into 4.20 should be against that.

> 

> 2) Patches destined for the NEXT kernel version should be against

>   my for-x.y/block branch, where x.y is the next version. As of

>   right now, patches for 4.21 should be against my for-4.21/bloc

>   branch.

> 

> I'd encourage you to respin against that, particularly in this case

> since we've both got a lot of churn, and also removal of various

> items that you are patching here.

> 

> -- 

> Jens Axboe
Jens Axboe Nov. 12, 2018, 3:48 p.m. UTC | #5
On 11/12/18 8:45 AM, Paolo Valente wrote:
> BTW, since this patch series is probably even more useful for older

> than for future kernels, might it make sense to also propose it for

> stable/longterm kernels (provided that such a possibility exists)?


That just not how things work, we don't put different things in
older/stable kernels, it's strictly backports of what we have in
current/newer kernels. Hence it appears to be a dead end right now.

-- 
Jens Axboe
Josef Bacik Nov. 12, 2018, 3:54 p.m. UTC | #6
On Mon, Nov 12, 2018 at 08:48:35AM -0700, Jens Axboe wrote:
> On 11/12/18 8:45 AM, Paolo Valente wrote:

> > BTW, since this patch series is probably even more useful for older

> > than for future kernels, might it make sense to also propose it for

> > stable/longterm kernels (provided that such a possibility exists)?

> 

> That just not how things work, we don't put different things in

> older/stable kernels, it's strictly backports of what we have in

> current/newer kernels. Hence it appears to be a dead end right now.

> 


It may not be useful currently, but my plans are to do a scheduler agnostic
proportional io controller next, so having these interfaces unified would be
nice so I don't have to do a rqos.io.weight or something similar.  Thanks,

Josef
Jens Axboe Nov. 12, 2018, 4:05 p.m. UTC | #7
On 11/12/18 8:54 AM, Josef Bacik wrote:
> On Mon, Nov 12, 2018 at 08:48:35AM -0700, Jens Axboe wrote:

>> On 11/12/18 8:45 AM, Paolo Valente wrote:

>>> BTW, since this patch series is probably even more useful for older

>>> than for future kernels, might it make sense to also propose it for

>>> stable/longterm kernels (provided that such a possibility exists)?

>>

>> That just not how things work, we don't put different things in

>> older/stable kernels, it's strictly backports of what we have in

>> current/newer kernels. Hence it appears to be a dead end right now.

>>

> 

> It may not be useful currently, but my plans are to do a scheduler agnostic

> proportional io controller next, so having these interfaces unified would be

> nice so I don't have to do a rqos.io.weight or something similar.  Thanks,


I'm not saying the work isn't useful, I'm saying that we can't go adding
different interfaces to stable kernels than what we currently have in
tip. I'm all for unified interfaces for this kind of thing, it's much
better than having something that's specific to any given
implementation.

-- 
Jens Axboe
Angelo Ruocco Nov. 15, 2018, 11:54 a.m. UTC | #8
Hi Jens,

I have rebased the patchset against the for-4.21/block branch, but I
can't test them properly because the compiling process has an error on
a different file. In particular:

include/net/xfrm.h:1465:3 error: unknown type 'spruct'
include/net/xfrm.h:1465:30 error: expected ':', ',', ';', '}' or
'__attribute__' before 'auth'

To be clear, and so that you can check I haven't made some trivial
mistakes: I have added/fetched the remote [1] and then simply rebased
against the for-4.21/block branch.

[1] git://git.kernel.dk/linux-block

Angelo

2018-11-12 16:35 GMT+01:00, Jens Axboe <axboe@kernel.dk>:
> On 11/12/18 3:17 AM, Paolo Valente wrote:

>>

>>

>>> Il giorno 12 nov 2018, alle ore 11:00, Oleksandr Natalenko

>>> <oleksandr@natalenko.name> ha scritto:

>>>

>>> On 12.11.2018 10:56, Paolo Valente wrote:

>>>> Hi Jens, Tejun, all,

>>>> about nine months ago, we agreed on a solution for unifying the

>>>> interface of the proportional-share policy in blkio/io [1].  Angelo

>>>> and I finally completed it.  Let me briefly recall the problem and the

>>>> solution.

>>>> The current implementation of cgroups doesn't allow two or more

>>>> entities, e.g., I/O schedulers, to share the same files.  So, if CFQ

>>>> creates its files for the proportional-share policy, such as, e.g,

>>>> weight files for blkio/io groups, BFQ cannot attach somehow to them.

>>>> Thus, to enable people to set group weights with BFQ, I resorted to

>>>> making BFQ create its own version of these common files, by prepending

>>>> a bfq prefix.

>>>> Actually, no legacy code uses these different names, or is likely to

>>>> do so.  Having these two sets of names is simply a source of

>>>> confusion, as pointed out also, e.g., by Lennart Poettering (CCed

>>>> here), and acknowledged by Tejun [2].

>>>> In [1] we agreed on a solution that solves this problem, by actually

>>>> making it possible to share cgroups files.  Both writing to and

>>>> reading from a shared file trigger the appropriate operation for each

>>>> of the entities that share the file.  In particular, in case of

>>>> reading,

>>>> - if all entities produce the same output, the this common output is

>>>>  shown only once;

>>>> - if the outputs differ, then every per-entity output is shown,

>>>>  preceded by the name of the entity that produced that output.

>>>> With this solution, legacy code that, e.g., sets group weights, just

>>>> works, regardless of the I/O scheduler actually implementing

>>>> proportional share.

>>>> But note that this extension is not restricted to only blkio/io.  The

>>>> general group interface now enables files to be shared among multiple

>>>> entities of any kind.

>>>> (I have also added a patch to fix some clerical errors in bfq doc,

>>>> which I found while making the latter consistent with the new

>>>> interface.)

>>>> Thanks,

>>>> Paolo

>>>> [1] https://lkml.org/lkml/2018/1/4/667

>>>> [2] https://github.com/systemd/systemd/issues/7057

>>>> Angelo Ruocco (7):

>>>>  kernfs: add function to find kernfs_node without increasing ref

>>>>    counter

>>>>  cgroup: link cftypes of the same subsystem with the same name

>>>>  cgroup: add owner name to cftypes

>>>>  block, bfq: align min and default weights with cfq

>>>>  cgroup: make all functions of all cftypes be invoked

>>>>  block, cfq: allow cgroup files to be shared

>>>>  block, throttle: allow sharing cgroup statistic files

>>>> Paolo Valente (5):

>>>>  cgroup: add hook seq_show_cft with also the owning cftype as parameter

>>>>  block, cgroup: pass cftype to functions that need to use it

>>>>  block, bfq: use standard file names for the proportional-share policy

>>>>  doc, bfq-iosched: fix a few clerical errors

>>>>  doc, bfq-iosched: make it consistent with the new cgroup interface

>>>> Documentation/block/bfq-iosched.txt |  31 +++--

>>>> block/bfq-cgroup.c                  | 148 +++++++++++++-------

>>>> block/bfq-iosched.h                 |   4 +-

>>>> block/blk-cgroup.c                  |  22 +--

>>>> block/blk-throttle.c                |  24 ++--

>>>> block/cfq-iosched.c                 | 105 +++++++++++----

>>>> fs/kernfs/dir.c                     |  13 ++

>>>> include/linux/blk-cgroup.h          |  10 +-

>>>> include/linux/cgroup-defs.h         |  14 +-

>>>> include/linux/cgroup.h              |  13 ++

>>>> include/linux/kernfs.h              |   7 +

>>>> kernel/cgroup/cgroup.c              | 262

>>>> +++++++++++++++++++++++++++++-------

>>>> 12 files changed, 483 insertions(+), 170 deletions(-)

>>>> --

>>>> 2.16.1

>>>

>>> I thought all the legacy stuff including CFS et al. is going to be

>>> removed in v4.21 completely…

>>>

>>

>> Thanks for pointing this out.

>>

>> People with a lower kernel version than the future 4.21 just cannot

>> and will not be able to use the proportional share policy on blk-mq

>> (with legacy code), because of the name issue highlighted in this

>> email.  If this patch series gets accepted, a backport will solve the

>> problem.  In this respect, such a backport might even happen

>> 'automatically', as most bfq commit seem to get backported to older,

>> stable kernels.

>>

>> In addition, this extension

>> - extends the whole cgroups interface, in a seamless and

>>   backward-compatible way, to prevent future issues like these;

>> - solves a similar issue with throttle (which AFAIK won't go away

>>   with 4.21).

>

> There's no way this series can get accepted, since you've made the

> mistake of basing it on something that won't apply to the block

> tree for 4.21. I've outlined these rules before, but here they are

> again:

>

> 1) Patches destined for the CURRENT kernel version should be

>    against my for-linus branch. That means that right now, any

>    patches that should to into 4.20 should be against that.

>

> 2) Patches destined for the NEXT kernel version should be against

>    my for-x.y/block branch, where x.y is the next version. As of

>    right now, patches for 4.21 should be against my for-4.21/bloc

>    branch.

>

> I'd encourage you to respin against that, particularly in this case

> since we've both got a lot of churn, and also removal of various

> items that you are patching here.

>

> --

> Jens Axboe

>

>
Jens Axboe Nov. 15, 2018, 4:30 p.m. UTC | #9
On 11/15/18 4:54 AM, Angelo Ruocco wrote:
> Hi Jens,

> 

> I have rebased the patchset against the for-4.21/block branch, but I

> can't test them properly because the compiling process has an error on

> a different file. In particular:

> 

> include/net/xfrm.h:1465:3 error: unknown type 'spruct'

> include/net/xfrm.h:1465:30 error: expected ':', ',', ';', '}' or

> '__attribute__' before 'auth'

> 

> To be clear, and so that you can check I haven't made some trivial

> mistakes: I have added/fetched the remote [1] and then simply rebased

> against the for-4.21/block branch.


I think you have memory issues, a 'p' is just one bit away from a 't'.

-- 
Jens Axboe
Angelo Ruocco Nov. 19, 2018, 9:46 a.m. UTC | #10
2018-11-15 17:30 GMT+01:00, Jens Axboe <axboe@kernel.dk>:
> On 11/15/18 4:54 AM, Angelo Ruocco wrote:

>> Hi Jens,

>>

>> I have rebased the patchset against the for-4.21/block branch, but I

>> can't test them properly because the compiling process has an error on

>> a different file. In particular:

>>

>> include/net/xfrm.h:1465:3 error: unknown type 'spruct'

>> include/net/xfrm.h:1465:30 error: expected ':', ',', ';', '}' or

>> '__attribute__' before 'auth'

>>

>> To be clear, and so that you can check I haven't made some trivial

>> mistakes: I have added/fetched the remote [1] and then simply rebased

>> against the for-4.21/block branch.

>

> I think you have memory issues, a 'p' is just one bit away from a 't'.


That was indeed the problem.

Thank you,
Angelo

>

> --

> Jens Axboe

>