linux-kernel - Re: [RFC PATCH 4/4] [PATCH 4/4] mm: add tracing for VMA merges

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220225140708.onog34i7xp2d7geh@revolver>
Date:   Fri, 25 Feb 2022 14:07:14 +0000
From:   Liam Howlett <liam.howlett@...cle.com>
To:     Vlastimil Babka <vbabka@...e.cz>
CC:     Jakub Matěna <matenajakub@...il.com>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "patches@...ts.linux.dev" <patches@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "mhocko@...nel.org" <mhocko@...nel.org>,
        "mgorman@...hsingularity.net" <mgorman@...hsingularity.net>,
        "willy@...radead.org" <willy@...radead.org>,
        "hughd@...gle.com" <hughd@...gle.com>,
        "kirill@...temov.name" <kirill@...temov.name>,
        "riel@...riel.com" <riel@...riel.com>,
        "rostedt@...dmis.org" <rostedt@...dmis.org>,
        "peterz@...radead.org" <peterz@...radead.org>
Subject: Re: [RFC PATCH 4/4] [PATCH 4/4] mm: add tracing for VMA merges

* Vlastimil Babka <vbabka@...e.cz> [220225 05:39]:
> On 2/18/22 20:57, Liam Howlett wrote:
> >>  	/*
> >>  	 * Can we merge both predecessor and successor?
> >>  	 */
> >> -	if (merge_prev && merge_next)
> >> +	if (merge_prev >= MERGE_OK && merge_next >= MERGE_OK) {
> > 
> > What happened to making vma_merge easier to read?  What does > MERGE_OK
> > mean?  I guess this answers why booleans were not used, but I don't like
> 
> It's similar to e.g. enum compact_priority where specific values are defined
> as well as more abstract aliases.
> 
> > it.   Couldn't you just log the err/success and the value of
> > merge_prev/merge_next?  It's not like the code tries more than one way
> > of merging on failure..
> 
> An initial version had the "log" (trace point really) at multiple places and
> it was uglier than collecting details in the variables and having a single
> tracepoint call site.

Wouldn't the success/failure, merge_prev, and merge_next be all we need
to know what happened?  This could be places near the end of the
function.  It doesn't say why merge_prev or merge_next was set to false
but I think that's enough for most uses?

> 
> Note that the tracepoint is being provided as part of the series mainly to
> allow evaluation of the series. If it's deemed too specific to be included
> in mainline in the end, so be it.

Ah, in that case use bool until we arrive at this patch, then change
merge_* to int.

> 
> >>  		merge_both = is_mergeable_anon_vma(prev->anon_vma, next->anon_vma, NULL);
> >> +	}
> >>  
> >> -	if (merge_both) {	 /* cases 1, 6 */
> >> +	if (merge_both >= MERGE_OK) {	 /* cases 1, 6 */
> >>  		err = __vma_adjust(prev, prev->vm_start,
> >>  					next->vm_end, prev->vm_pgoff, NULL,
> >>  					prev);
> >>  		area = prev;
> >> -	} else if (merge_prev) {			/* cases 2, 5, 7 */
> >> +	} else if (merge_prev >= MERGE_OK) {			/* cases 2, 5, 7 */
> >>  		err = __vma_adjust(prev, prev->vm_start,
> >>  					end, prev->vm_pgoff, NULL, prev);
> >>  		area = prev;
> >> -	} else if (merge_next) {
> >> +	} else if (merge_next >= MERGE_OK) {
> >>  		if (prev && addr < prev->vm_end)	/* case 4 */
> >>  			err = __vma_adjust(prev, prev->vm_start,
> >>  					addr, prev->vm_pgoff, NULL, next);
> >> @@ -1252,7 +1255,7 @@ struct vm_area_struct *vma_merge(struct mm_struct *mm,
> >>  	} else {
> >>  		err = -1;
> >>  	}
> >> -
> >> +	trace_vm_av_merge(err, merge_prev, merge_next, merge_both);
> >>  	/*
> >>  	 * Cannot merge with predecessor or successor or error in __vma_adjust?
> >>  	 */
> >> @@ -3359,6 +3362,8 @@ struct vm_area_struct *copy_vma(struct vm_area_struct **vmap,
> >>  		/*
> >>  		 * Source vma may have been merged into new_vma
> >>  		 */
> >> +		trace_vm_pgoff_merge(vma, anon_pgoff_updated);
> >> +
> >>  		if (unlikely(vma_start >= new_vma->vm_start &&
> >>  			     vma_start < new_vma->vm_end)) {
> >>  			/*
> >> -- 
> >> 2.34.1
> >> 
>