2017-11-28 18:09 GMT+08:00 Jan Kara <jack@xxxxxxx>: > On Tue 28-11-17 15:52:50, Yafang Shao wrote: >> 2017-11-28 15:45 GMT+08:00 Michal Hocko <mhocko@xxxxxxxx>: >> > On Tue 28-11-17 14:12:15, Yafang Shao wrote: >> >> 2017-11-28 11:11 GMT+08:00 Yafang Shao <laoar.shao@xxxxxxxxx>: >> >> > Hi Michal, >> >> > >> >> > What about bellow change ? >> >> > It makes the function domain_dirty_limits() more clear. >> >> > And the result will have a higher precision. >> >> > >> >> > >> >> > diff --git a/mm/page-writeback.c b/mm/page-writeback.c >> >> > index 8a15511..2b5e507 100644 >> >> > --- a/mm/page-writeback.c >> >> > +++ b/mm/page-writeback.c >> >> > @@ -397,8 +397,8 @@ static void domain_dirty_limits(struct >> >> > dirty_throttle_control *dtc) >> >> > unsigned long bytes = vm_dirty_bytes; >> >> > unsigned long bg_bytes = dirty_background_bytes; >> >> > /* convert ratios to per-PAGE_SIZE for higher precision */ >> >> > - unsigned long ratio = (vm_dirty_ratio * PAGE_SIZE) / 100; >> >> > - unsigned long bg_ratio = (dirty_background_ratio * PAGE_SIZE) / 100; >> >> > + unsigned long ratio = vm_dirty_ratio; >> >> > + unsigned long bg_ratio = dirty_background_ratio; >> >> > unsigned long thresh; >> >> > unsigned long bg_thresh; >> >> > struct task_struct *tsk; >> >> > @@ -416,28 +416,33 @@ static void domain_dirty_limits(struct >> >> > dirty_throttle_control *dtc) >> >> > */ >> >> > if (bytes) >> >> > ratio = min(DIV_ROUND_UP(bytes, global_avail), >> >> > - PAGE_SIZE); >> >> > + 100); >> >> > if (bg_bytes) >> >> > bg_ratio = min(DIV_ROUND_UP(bg_bytes, global_avail), >> >> > - PAGE_SIZE); >> >> > + 99); /* bg_ratio should less than ratio */ >> >> > bytes = bg_bytes = 0; >> >> > } >> >> >> >> >> >> Errata: >> >> >> >> if (bytes) >> >> - ratio = min(DIV_ROUND_UP(bytes, global_avail), >> >> - PAGE_SIZE); >> >> + ratio = min(DIV_ROUND_UP(bytes / PAGE_SIZE, global_avail), >> >> + 100); >> >> if (bg_bytes) >> >> - bg_ratio = min(DIV_ROUND_UP(bg_bytes, global_avail), >> >> - PAGE_SIZE); >> >> + bg_ratio = min(DIV_ROUND_UP(bg_bytes / PAGE_SIZE, global_avail), >> >> + 100 - 1); /* bg_ratio should be less than ratio */ >> >> bytes = bg_bytes = 0; >> > >> > And you really think this makes code easier to follow? I am somehow not >> > conviced... >> > >> >> There's hidden bug in the original code, because it is too complex to >> clearly understand. >> See bellow, >> >> ratio = min(DIV_ROUND_UP(bytes, global_avail), >> PAGE_SIZE) >> >> Suppose the vm_dirty_bytes is set to 512M (this is a reasonable >> value), and the global_avail is only 10000 pages (this is not low), >> then DIV_ROUND_UP(bytes, global_avail) is 53688, which is bigger than >> 4096, so the ratio will be 4096. >> That's unreasonable. > > But that's not a bug in domain_dirty_limits(). It is more a design issue of > the dirty_bytes interface - i.e., if you tell the system that 512M of dirty > pages is fine, then it is fine even if you have only 400M of page cache - > i.e., 100% of page cache can be dirty and that's what the function > computes. Bad luck if you don't like that but that's how the interface was > (mis)designed. We can talk about changes to what dirty_bytes mean under a > situation when there is low amount of page cache but that will be a > userspace visible change and we will have to be *very* careful not to break > current users. > Thanks for your suggestion. I will submit a patch for that. Thanks Yafang -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
