[LSF/MM TOPIC] mm patches review bandwidth

[LSF/MM TOPIC] mm patches review bandwidth

[Michal Hocko]

Hi,
I have a very bad feeling that we are running out of the patch review
bandwidth for quite some time. Quite often it is really hard to get
any feedback at all. This leaves Andrew in an unfortunate position when
he is pushed to merge changes which are not reviewed.

A quick check shows that around 40% of patches is not tagged with
neither Acked-by nor Reviewed-by. While this is not any hard number it
should give us at least some idea...

$ git rev-list --no-merges v4.8..v4.9 -- mm/ | wc -l 
150
$ git rev-list --no-merges v4.8..v4.9 -- mm/ | while read sha1; do git show $sha1 | grep "Acked-by\|Reviewed-by" >/dev/null&& echo $sha1; done | wc -l
87

The overall trend since 4.0 shows that this is quite a consistent number

123 commits in 4.0..4.1 range 47 % unreviewed
170 commits in 4.1..4.2 range 56 % unreviewed
187 commits in 4.2..4.3 range 35 % unreviewed
176 commits in 4.3..4.4 range 34 % unreviewed
220 commits in 4.4..4.5 range 32 % unreviewed
199 commits in 4.5..4.6 range 42 % unreviewed
217 commits in 4.6..4.7 range 41 % unreviewed
247 commits in 4.7..4.8 range 39 % unreviewed
150 commits in 4.8..4.9 range 42 % unreviewed

I am worried that the number of patches posted to linux-mm grows over
time while the number of reviewers doesn't scale up with that trend. I
believe we need to do something about that and aim to increase both the
number of reviewers as well as the number of patches which are really
reviewed. I am not really sure how to achieve that, though. Requiring
Acked-by resp. Reviewed-by on each patch sounds like the right approach
but I am just worried that even useful changes could get stuck without
any forward progress that way.

Another problem, somehow related, is that there are areas which have
evolved into a really bad shape because nobody has really payed
attention to them from the architectural POV when they were merged. To
name one the memory hotplug doesn't seem very healthy, full of kludges,
random hacks and fixes for fixes working for a particualr usecase
without any longterm vision. We have allowed to (ab)use concepts like
ZONE_MOVABLE which are finding new users because that seems to be the
simplest way forward. Now we are left with fixing the code which has
some fundamental issues because it is used out there. Are we going to do
anything about those? E.g. generate a list of them, discuss how to make
that code healthy again and do not allow new features until we sort that
out?
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [LSF/MM TOPIC] mm patches review bandwidth

[Mike Kravetz]

On 01/05/2017 07:37 AM, Michal Hocko wrote:
> Hi,
> I have a very bad feeling that we are running out of the patch review
> bandwidth for quite some time. Quite often it is really hard to get
> any feedback at all. This leaves Andrew in an unfortunate position when
> he is pushed to merge changes which are not reviewed.
> 
> A quick check shows that around 40% of patches is not tagged with
> neither Acked-by nor Reviewed-by. While this is not any hard number it
> should give us at least some idea...
> 
> $ git rev-list --no-merges v4.8..v4.9 -- mm/ | wc -l 
> 150
> $ git rev-list --no-merges v4.8..v4.9 -- mm/ | while read sha1; do git show $sha1 | grep "Acked-by\|Reviewed-by" >/dev/null&& echo $sha1; done | wc -l
> 87
> 
> The overall trend since 4.0 shows that this is quite a consistent number
> 
> 123 commits in 4.0..4.1 range 47 % unreviewed
> 170 commits in 4.1..4.2 range 56 % unreviewed
> 187 commits in 4.2..4.3 range 35 % unreviewed
> 176 commits in 4.3..4.4 range 34 % unreviewed
> 220 commits in 4.4..4.5 range 32 % unreviewed
> 199 commits in 4.5..4.6 range 42 % unreviewed
> 217 commits in 4.6..4.7 range 41 % unreviewed
> 247 commits in 4.7..4.8 range 39 % unreviewed
> 150 commits in 4.8..4.9 range 42 % unreviewed
> 
> I am worried that the number of patches posted to linux-mm grows over
> time while the number of reviewers doesn't scale up with that trend. I
> believe we need to do something about that and aim to increase both the
> number of reviewers as well as the number of patches which are really
> reviewed. I am not really sure how to achieve that, though. Requiring
> Acked-by resp. Reviewed-by on each patch sounds like the right approach
> but I am just worried that even useful changes could get stuck without
> any forward progress that way.
> 
> Another problem, somehow related, is that there are areas which have
> evolved into a really bad shape because nobody has really payed
> attention to them from the architectural POV when they were merged. To
> name one the memory hotplug doesn't seem very healthy, full of kludges,
> random hacks and fixes for fixes working for a particualr usecase
> without any longterm vision. We have allowed to (ab)use concepts like
> ZONE_MOVABLE which are finding new users because that seems to be the
> simplest way forward. Now we are left with fixing the code which has
> some fundamental issues because it is used out there. Are we going to do
> anything about those? E.g. generate a list of them, discuss how to make
> that code healthy again and do not allow new features until we sort that
> out?

hugetlb reservation processing seems to be one of those areas.  I certainly
have been guilty of stretching the limits of the current code to meet the
demands of new functionality.  It has been my desire to do some rewrite or
rearchitecture in this area.

-- 
Mike Kravetz

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [LSF/MM TOPIC] mm patches review bandwidth

[Michal Hocko]

On Thu 05-01-17 17:43:38, Mike Kravetz wrote:
> On 01/05/2017 07:37 AM, Michal Hocko wrote:
[...]
> > Another problem, somehow related, is that there are areas which have
> > evolved into a really bad shape because nobody has really payed
> > attention to them from the architectural POV when they were merged. To
> > name one the memory hotplug doesn't seem very healthy, full of kludges,
> > random hacks and fixes for fixes working for a particualr usecase
> > without any longterm vision. We have allowed to (ab)use concepts like
> > ZONE_MOVABLE which are finding new users because that seems to be the
> > simplest way forward. Now we are left with fixing the code which has
> > some fundamental issues because it is used out there. Are we going to do
> > anything about those? E.g. generate a list of them, discuss how to make
> > that code healthy again and do not allow new features until we sort that
> > out?
> 
> hugetlb reservation processing seems to be one of those areas.  I certainly
> have been guilty of stretching the limits of the current code to meet the
> demands of new functionality.  It has been my desire to do some rewrite or
> rearchitecture in this area.

I think that it would be really useful to start by a throughout design
documentation of the current code before any rewrite. I believe this
will already tell us a lot of the design complexities and shortcomings
already.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [LSF/MM TOPIC] mm patches review bandwidth

[Vlastimil Babka]

On 01/05/2017 04:37 PM, Michal Hocko wrote:
> Hi,
> I have a very bad feeling that we are running out of the patch review
> bandwidth for quite some time. Quite often it is really hard to get
> any feedback at all. This leaves Andrew in an unfortunate position when
> he is pushed to merge changes which are not reviewed.
> 
> A quick check shows that around 40% of patches is not tagged with
> neither Acked-by nor Reviewed-by. While this is not any hard number it
> should give us at least some idea...
> 
> $ git rev-list --no-merges v4.8..v4.9 -- mm/ | wc -l 
> 150
> $ git rev-list --no-merges v4.8..v4.9 -- mm/ | while read sha1; do git show $sha1 | grep "Acked-by\|Reviewed-by" >/dev/null&& echo $sha1; done | wc -l
> 87
> 
> The overall trend since 4.0 shows that this is quite a consistent number
> 
> 123 commits in 4.0..4.1 range 47 % unreviewed
> 170 commits in 4.1..4.2 range 56 % unreviewed
> 187 commits in 4.2..4.3 range 35 % unreviewed
> 176 commits in 4.3..4.4 range 34 % unreviewed
> 220 commits in 4.4..4.5 range 32 % unreviewed
> 199 commits in 4.5..4.6 range 42 % unreviewed
> 217 commits in 4.6..4.7 range 41 % unreviewed
> 247 commits in 4.7..4.8 range 39 % unreviewed
> 150 commits in 4.8..4.9 range 42 % unreviewed

That's better than I pessimistically expected, but still far from great,
yeah.

> I am worried that the number of patches posted to linux-mm grows over
> time while the number of reviewers doesn't scale up with that trend. I
> believe we need to do something about that and aim to increase both the
> number of reviewers as well as the number of patches which are really
> reviewed. I am not really sure how to achieve that, though. Requiring
> Acked-by resp. Reviewed-by on each patch sounds like the right approach
> but I am just worried that even useful changes could get stuck without
> any forward progress that way.

It does work for some other subsystems, so I would be for trying this.
mm is core enough to deserve being careful IMHO.

> Another problem, somehow related, is that there are areas which have
> evolved into a really bad shape because nobody has really payed
> attention to them from the architectural POV when they were merged. To
> name one the memory hotplug doesn't seem very healthy, full of kludges,
> random hacks and fixes for fixes working for a particualr usecase
> without any longterm vision. We have allowed to (ab)use concepts like
> ZONE_MOVABLE which are finding new users because that seems to be the
> simplest way forward. Now we are left with fixing the code which has
> some fundamental issues because it is used out there. Are we going to do
> anything about those? E.g. generate a list of them, discuss how to make
> that code healthy again and do not allow new features until we sort that
> out?

Sounds like we could at least try.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [LSF/MM TOPIC] mm patches review bandwidth

[Yasuaki Ishimatsu]

On 01/05/2017 10:37 AM, Michal Hocko wrote:
> Hi,
> I have a very bad feeling that we are running out of the patch review
> bandwidth for quite some time. Quite often it is really hard to get
> any feedback at all. This leaves Andrew in an unfortunate position when
> he is pushed to merge changes which are not reviewed.
>
> A quick check shows that around 40% of patches is not tagged with
> neither Acked-by nor Reviewed-by. While this is not any hard number it
> should give us at least some idea...
>
> $ git rev-list --no-merges v4.8..v4.9 -- mm/ | wc -l
> 150
> $ git rev-list --no-merges v4.8..v4.9 -- mm/ | while read sha1; do git show $sha1 | grep "Acked-by\|Reviewed-by" >/dev/null&& echo $sha1; done | wc -l
> 87
>
> The overall trend since 4.0 shows that this is quite a consistent number
>
> 123 commits in 4.0..4.1 range 47 % unreviewed
> 170 commits in 4.1..4.2 range 56 % unreviewed
> 187 commits in 4.2..4.3 range 35 % unreviewed
> 176 commits in 4.3..4.4 range 34 % unreviewed
> 220 commits in 4.4..4.5 range 32 % unreviewed
> 199 commits in 4.5..4.6 range 42 % unreviewed
> 217 commits in 4.6..4.7 range 41 % unreviewed
> 247 commits in 4.7..4.8 range 39 % unreviewed
> 150 commits in 4.8..4.9 range 42 % unreviewed
>
> I am worried that the number of patches posted to linux-mm grows over
> time while the number of reviewers doesn't scale up with that trend. I
> believe we need to do something about that and aim to increase both the
> number of reviewers as well as the number of patches which are really
> reviewed. I am not really sure how to achieve that, though. Requiring
> Acked-by resp. Reviewed-by on each patch sounds like the right approach
> but I am just worried that even useful changes could get stuck without
> any forward progress that way.


On 01/05/2017 10:37 AM, Michal Hocko wrote:
 > Hi,
 > I have a very bad feeling that we are running out of the patch review
 > bandwidth for quite some time. Quite often it is really hard to get
 > any feedback at all. This leaves Andrew in an unfortunate position when
 > he is pushed to merge changes which are not reviewed.
 >
 > A quick check shows that around 40% of patches is not tagged with
 > neither Acked-by nor Reviewed-by. While this is not any hard number it
 > should give us at least some idea...
 >
 > $ git rev-list --no-merges v4.8..v4.9 -- mm/ | wc -l
 > 150
 > $ git rev-list --no-merges v4.8..v4.9 -- mm/ | while read sha1; do git show $sha1 | grep "Acked-by\|Reviewed-by" >/dev/null&& echo $sha1; done | wc -l
 > 87
 >
 > The overall trend since 4.0 shows that this is quite a consistent number
 >
 > 123 commits in 4.0..4.1 range 47 % unreviewed
 > 170 commits in 4.1..4.2 range 56 % unreviewed
 > 187 commits in 4.2..4.3 range 35 % unreviewed
 > 176 commits in 4.3..4.4 range 34 % unreviewed
 > 220 commits in 4.4..4.5 range 32 % unreviewed
 > 199 commits in 4.5..4.6 range 42 % unreviewed
 > 217 commits in 4.6..4.7 range 41 % unreviewed
 > 247 commits in 4.7..4.8 range 39 % unreviewed
 > 150 commits in 4.8..4.9 range 42 % unreviewed
 >
 > I am worried that the number of patches posted to linux-mm grows over
 > time while the number of reviewers doesn't scale up with that trend. I
 > believe we need to do something about that and aim to increase both the
 > number of reviewers as well as the number of patches which are really
 > reviewed. I am not really sure how to achieve that, though. Requiring
 > Acked-by resp. Reviewed-by on each patch sounds like the right approach
 > but I am just worried that even useful changes could get stuck without
 > any forward progress that way.

Here are additional information.

Rate of unreviewed patch (Unreviewed patch/Total patch))
  Version     : mm           : arch/x86     : fs/ext4      : fs/xfs       : fs/btrfs     :
  v3.0..v3.1  : 56 ( 86/151) : 79 (277/349) : 90 ( 75/ 83) : 29 ( 28/ 96) : 96 (103/107) :
  v3.1..v3.2  : 45 ( 61/135) : 88 (227/256) : 95 (105/110) : 48 ( 42/ 86) : 98 (182/184) :
  v3.2..v3.3  : 42 ( 94/221) : 85 (281/329) : 97 ( 45/ 46) : 23 ( 12/ 51) : 96 (127/131) :
  v3.3..v3.4  : 42 ( 55/130) : 76 (259/339) : 84 ( 50/ 59) : 12 (  7/ 57) : 99 (117/118) :
  v3.4..v3.5  : 38 ( 70/182) : 81 (293/358) : 88 ( 38/ 43) : 18 ( 13/ 71) : 91 ( 99/108) :
  v3.5..v3.6  : 37 ( 65/173) : 76 (234/305) : 78 ( 32/ 41) : 16 (  9/ 53) : 96 (100/104) :
  v3.6..v3.7  : 54 (112/205) : 73 (296/404) : 81 ( 64/ 79) : 27 ( 11/ 40) : 98 (148/151) :
  v3.7..v3.8  : 51 (124/239) : 77 (175/225) : 70 ( 54/ 77) :  1 (  1/ 89) : 91 (147/161) :
  v3.8..v3.9  : 52 ( 91/173) : 65 (279/428) : 67 ( 64/ 95) :  2 (  1/ 44) : 93 (152/162) :
  v3.9..v3.10 : 51 ( 68/132) : 68 (204/300) : 78 ( 40/ 51) :  1 (  1/ 81) : 85 (114/134) :
v3.10..v3.11 : 51 ( 83/162) : 64 (128/200) : 61 ( 52/ 85) :  4 (  3/ 74) : 93 ( 93/100) :
v3.11..v3.12 : 40 ( 91/222) : 51 (109/211) : 70 ( 22/ 31) :  6 ( 10/144) : 82 (130/157) :
v3.12..v3.13 : 33 ( 54/160) : 69 (174/252) : 62 ( 15/ 24) :  0 (  0/ 66) : 83 (104/125) :
v3.13..v3.14 : 41 ( 71/173) : 70 (181/258) : 66 ( 16/ 24) :  6 (  4/ 64) : 81 (140/171) :
v3.14..v3.15 : 47 ( 91/191) : 71 (209/294) : 78 ( 55/ 70) :  8 (  5/ 61) : 91 (139/152) :
v3.15..v3.16 : 48 (103/212) : 64 (163/253) : 78 ( 36/ 46) : 13 ( 14/101) : 78 (107/137) :
v3.16..v3.17 : 30 ( 45/147) : 82 (246/299) : 72 ( 16/ 22) :  1 (  1/ 51) : 68 ( 30/ 44) :
v3.17..v3.18 : 53 ( 82/154) : 77 (221/286) : 73 ( 50/ 68) :  1 (  1/ 66) : 80 (120/149) :
v3.18..v3.19 : 45 ( 74/161) : 73 (267/363) : 96 ( 29/ 30) :  3 (  1/ 26) : 85 ( 70/ 82) :
v3.19..v4.1  : 50 (153/304) : 71 (513/716) : 82 ( 60/ 73) : 13 ( 16/118) : 80 (173/216) :
  v4.1..v4.2  : 56 ( 96/170) : 63 (475/752) : 84 ( 50/ 59) : 10 (  6/ 56) : 69 ( 83/119) :
  v4.2..v4.3  : 35 ( 67/187) : 75 (279/368) : 72 ( 21/ 29) :  6 (  3/ 48) : 63 ( 47/ 74) :
  v4.3..v4.4  : 34 ( 60/176) : 68 (231/336) : 76 ( 26/ 34) :  4 (  2/ 42) : 73 (102/138) :
  v4.4..v4.5  : 32 ( 72/220) : 57 (156/273) : 74 ( 29/ 39) : 25 ( 12/ 47) : 83 (106/127) :
  v4.5..v4.6  : 42 ( 84/199) : 65 (324/495) : 89 ( 42/ 47) :  4 (  3/ 68) : 69 ( 69/ 99) :
  v4.6..v4.7  : 41 ( 91/217) : 70 (229/327) : 81 ( 26/ 32) : 25 ( 13/ 52) : 63 (101/158) :
  v4.7..v4.8  : 39 ( 98/247) : 64 (272/425) : 66 ( 20/ 30) :  2 (  3/125) : 60 ( 68/112) :
  v4.8..v4.9  : 42 ( 63/150) : 58 (189/321) : 80 ( 34/ 42) : 13 ( 17/129) : 48 ( 31/ 64) :

xfs seems to be good. So there may be hints to improve the problem.


> Another problem, somehow related, is that there are areas which have
> evolved into a really bad shape because nobody has really payed
> attention to them from the architectural POV when they were merged. To
> name one the memory hotplug doesn't seem very healthy, full of kludges,
> random hacks and fixes for fixes working for a particualr usecase
> without any longterm vision. We have allowed to (ab)use concepts like
> ZONE_MOVABLE which are finding new users because that seems to be the
> simplest way forward. Now we are left with fixing the code which has
> some fundamental issues because it is used out there. Are we going to do
> anything about those? E.g. generate a list of them, discuss how to make
> that code healthy again and do not allow new features until we sort that
> out?
>

I've joined memory hotplug development since 2011. To make enhancement
of memory hotplug healthy, I'd like to join the discussion.

Thanks,
Yasuaki Ishimatsu

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [LSF/MM TOPIC] mm patches review bandwidth

[Vlastimil Babka]

On 01/06/2017 02:43 AM, Mike Kravetz wrote:
> On 01/05/2017 07:37 AM, Michal Hocko wrote:
>> Another problem, somehow related, is that there are areas which have
>> evolved into a really bad shape because nobody has really payed
>> attention to them from the architectural POV when they were merged. To
>> name one the memory hotplug doesn't seem very healthy, full of kludges,
>> random hacks and fixes for fixes working for a particualr usecase
>> without any longterm vision. We have allowed to (ab)use concepts like
>> ZONE_MOVABLE which are finding new users because that seems to be the
>> simplest way forward. Now we are left with fixing the code which has
>> some fundamental issues because it is used out there. Are we going to do
>> anything about those? E.g. generate a list of them, discuss how to make
>> that code healthy again and do not allow new features until we sort that
>> out?
>
> hugetlb reservation processing seems to be one of those areas.  I certainly
> have been guilty of stretching the limits of the current code to meet the
> demands of new functionality.  It has been my desire to do some rewrite or
> rearchitecture in this area.

Since this is now a list, let me add cpuset's mems_allowed handling there... See 
[1] for some details.

[1] https://lkml.kernel.org/r/20170117221610.22505-1-vbabka@suse.cz

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>