Messages in this thread | | | Date | Wed, 19 Oct 2022 12:18:35 +0500 | Subject | Re: [PATCH v3 0/4] Implement IOCTL to get and clear soft dirty PTE | From | Muhammad Usama Anjum <> |
| |
On 10/18/22 10:17 PM, Michał Mirosław wrote: > On Tue, 18 Oct 2022 at 15:23, Muhammad Usama Anjum > <usama.anjum@collabora.com> wrote: >> >> On 10/18/22 4:11 PM, Michał Mirosław wrote: >>> On Tue, 18 Oct 2022 at 12:36, Muhammad Usama Anjum >>> <usama.anjum@collabora.com> wrote: > [...] >>>> * @start: Starting address >>>> * @len: Length of the region >>>> * @vec: Output page_region struct array >>>> * @vec_len: Length of the page_region struct array >>>> * @max_out_page: Optional max output pages (It must be less than >>>> vec_len if specified) >>> >>> Why is it required to be less than vec_len? vec_len effectively >>> specifies max number of ranges to find, and this new additional field >>> counts pages, I suppose? >>> BTW, if we count pages, then what size of them? Maybe using bytes >>> (matching start/len fields) would be more consistent? >> Yes, it if for counting pages. As the regions can have multiple pages, >> user cannot specify through the number of regions that how many pages >> does he need. Page size is used here as well like the start and len. >> This is optional argument as this is only needed to emulate the Windows >> syscall getWriteWatch. > > I'm wondering about the condition that max_out_page < vec_len. Since > both count different things (pages vs ranges) I would expect there is > no strict relation between them and information returned is as much as > fits both (IOW: at most vec_len ranges spanning not more than > max_out_page pages). The field's name and description I'd suggest > improving: maybe 'max_pages' with a comment that 0 = unlimited? Correct, max_pages with this comment is what I want. I'll update. vec_len represents the total number of the page_range array elements. If the pages which we want to return are sparse or the consective pages have different flags, we'll only return one page in one page_range struct. In this case if someone has specified max_pages to be 10, vec_len must be at least 10 to keep store the 10 pages. So max_pages <= vec_len.
> > [...] >>>> /* Special flags */ >>>> #define PAGEMAP_NO_REUSED_REGIONS 0x1 >>> >>> What does this flag do? >> Some non-dirty pages get marked as dirty because of the kernel's >> internal activity. The dirty bit of the pages is stored in the VMA flags >> and in the per page flags. If any of these two bits are set, the page is >> considered to be dirty. Suppose you have cleared the dirty bit of half >> of VMA which will be done by splitting the VMA and clearing dirty flag >> in the half VMA and the pages in it. Now kernel may decide to merge the >> VMAs again as dirty bit of VMAs isn't considered if the VMAs should be >> merged. So the half VMA becomes dirty again. This splitting/merging >> costs performance. The application receives a lot of pages which aren't >> dirty in reality but marked as dirty. Performance is lost again here. >> >> This PAGEMAP_NO_REUSED_REGIONS flag is used to don't depend on the dirty >> flag in the VMA flags. It only depends on the individual page dirty bit. >> With doing this, the new memory regions which are just created, doesn't >> look like dirty when seen with the IOCTL, but look dirty when seen from >> pagemap. This seems okay as the user of this flag know the implication >> of using it. > > Thanks for explaining! Could you include this as a comment in the patch? Will do.
> > Best Regards > Michał Mirosław
| |