Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Liam Howlett <liam.howlett@oracle.com>
To:     Vlastimil Babka <vbabka@suse.cz>
CC:     =?iso-8859-2?Q?Jakub_Mat=ECna?= <matenajakub@gmail.com>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "patches@lists.linux.dev" <patches@lists.linux.dev>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "mhocko@kernel.org" <mhocko@kernel.org>,
        "mgorman@techsingularity.net" <mgorman@techsingularity.net>,
        "willy@infradead.org" <willy@infradead.org>,
        "hughd@google.com" <hughd@google.com>,
        "kirill@shutemov.name" <kirill@shutemov.name>,
        "riel@surriel.com" <riel@surriel.com>,
        "rostedt@goodmis.org" <rostedt@goodmis.org>,
        "peterz@infradead.org" <peterz@infradead.org>
Subject: Re: [RFC PATCH 4/4] [PATCH 4/4] mm: add tracing for VMA merges
Thread-Topic: [RFC PATCH 4/4] [PATCH 4/4] mm: add tracing for VMA merges
Thread-Index: AQHYJMIWwUhFsrN60kOTfTd4C42Vf6yZugmAgApkeACAADntAA==
Date:   Fri, 25 Feb 2022 14:07:14 +0000
Message-ID: <20220225140708.onog34i7xp2d7geh@revolver>
References: <20220218122019.130274-1-matenajakub@gmail.com>
 <20220218122019.130274-5-matenajakub@gmail.com>
 <20220218195729.oa5olrcsq6yox7hp@revolver>
 <d7f56188-5512-1365-243a-1e70acddf5c1@suse.cz>
In-Reply-To: <d7f56188-5512-1365-243a-1e70acddf5c1@suse.cz>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?iso-8859-2?Q?Tk9yTZXK7bA+531din1GEQzke3Xays3rqOqaE5UEC6SfQlD8yRlSM7phmN?=
 =?iso-8859-2?Q?rL/pPuW2/3Y64oWInMYeC+U55YN+Yy3rV4krEG18QD5UsgcQmGEpRxP4KL?=
 =?iso-8859-2?Q?PfNsgmSE8jW0LAzXarJI3wIAnJ1AoXu7DR1CKGBs/w1pbbPB7AdGev/b5n?=
 =?iso-8859-2?Q?Oi86NLc5h2VRKN2mMUzAsdnM0XQSxzjmyHJH1CNCH8LXR8TbwR3IrteXev?=
 =?iso-8859-2?Q?G92iYS5mJzLaNmQACLgoiWufCY96P3OLxBvtk41JMcPIIX7XKQF0OOzDVk?=
 =?iso-8859-2?Q?G9onTHalavxIgq6+JCNEqf7yMKUo77dyYV4hOos1Z9zuU6NWZwfBfhFiIc?=
 =?iso-8859-2?Q?Lq3DPhxUqPfXePnrvIsGwG0RDm/NOb2Af+cmsggZYXuBO+BMEjEeQgqx6H?=
 =?iso-8859-2?Q?uztQ8rjbNTGpIuG2tn5Xvdrf/Dn94ieRCN31ARaDcbGh4Xrp7pjuWXkUhY?=
 =?iso-8859-2?Q?8LCxlzbbO21W7zgjlwBl/b/Z0mzmL42vpYp6GzmYs0U+tdl/aEnRYamY0C?=
 =?iso-8859-2?Q?WB+v19blSg2MlZ0wYIlDansCcxRPV4m1psssGPx0MBPWXv1fwiqGxWUX/v?=
 =?iso-8859-2?Q?RITxDA9aPmjsQYYlz4hOf0q7QzwvEeEX/q6UgNwXjUv6cT3x5bi3UNN2Cl?=
 =?iso-8859-2?Q?3ogTDzn/D9MD2Tmg2VpdAZapGwMs8Mf7/3EFpOueIgIaeI4/3Y/zkJO8Qw?=
 =?iso-8859-2?Q?KBIil5D0vm6+41dCi0Kmh+V8xkjwTattWM0Fvmdqbhfolpc5LRAktV97sS?=
 =?iso-8859-2?Q?rrOMaRuORGZ2e+gbwNhZmEjGFsvmQtie2Y6u1EmkF5MbjJQYEYz2AQPFO/?=
 =?iso-8859-2?Q?gnr2ErjlcWqpjdaiEvOokErqRBnz02zS8aQIEcO8sONoCAlLqglrW+eE4B?=
 =?iso-8859-2?Q?h49GSXBzEuIc+QXQaed6960tVTdXowfOc+AqH7PRPoU6VOXMjVSUUwVZZJ?=
 =?iso-8859-2?Q?Wu/I/iZ7zpCtpt+SL2JQyQmH6PumDJqo2ShCN0HWAnVFZl0l2XVuFXVjFl?=
 =?iso-8859-2?Q?InXIsDFiwbpyyCM3D6wRvSJ8DrfeJRHWo7yDv3AH4SWZDYzB4+BSN1FCth?=
 =?iso-8859-2?Q?/gvo8CI+WgTZNWAeyu3tf8assnugKL4FAgfehOOaQr57dmLi+vSgRCVTi5?=
 =?iso-8859-2?Q?/mmya45YBYFlUdT7tjMCdljXwEmdD2PCzjeRfP/T3hAfVi6+hekD7f4Sru?=
 =?iso-8859-2?Q?epUYlPVWYQZsN8fYdRzk1iNS+CjBPBGcw2zYU0JiLpc6sFRqEwkQWoiPt3?=
 =?iso-8859-2?Q?rKX7HJAg2g6Bf3OMree1wQTO7hqRuRDbIhHPEk84LIqdaR5tUXcouWP/ak?=
 =?iso-8859-2?Q?WPVQvwFxyhxxVP0o1esUp6Y5GvgspX4pP4+CEuuK2Jt4G3HWAngh9gp3wD?=
 =?iso-8859-2?Q?NSLjFO/BODEUUPQDN1gynAhyELm4T5dB/XIWW6EV2d/1g+FZLxA6aVG8IX?=
 =?iso-8859-2?Q?caoSC33oBz0UDiJVU2Q/DPfI2ZSCjtFx0fs/Du+zRmzea+d6wXefSyUpAj?=
 =?iso-8859-2?Q?FArpt6oWOkKopG3phKe74IFFqQDQ3JMekf4vubTtonXYifQct06hCO22rz?=
 =?iso-8859-2?Q?HHGMTqD2CAD+gE9T57xempwBsKlH/cOpv3c5a/rJRE78uFxjbr0fkameRx?=
 =?iso-8859-2?Q?mV94sww5QTjMKhtv68tJaqoC/J5c8Srci7m+9yUI8X1a+JEGRCGXtZND2S?=
 =?iso-8859-2?Q?CZyJXsmeEaq4HF0GbhU=3D?=
Content-Type: text/plain; charset="iso-8859-2"
Content-ID: <4696D9A8FA429C4B8C28F1F3F1C801CA@namprd10.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN6PR10MB3022.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a16bee20-bd26-48e6-0e75-08d9f8682035
X-MS-Exchange-CrossTenant-originalarrivaltime: 25 Feb 2022 14:07:14.9333
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: icWc0wlO4uaqtNxmXxsqxiwR6nCro5xtcQAdu476Au9p2Dq9gfG4t6ZiACgRr86bKnp37VlYK8Yp5UhZ8AQtnA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR10MB4640
X-Proofpoint-Virus-Version: vendor=nai engine=6300 definitions=10268 signatures=684655
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 suspectscore=0 mlxscore=0
 spamscore=0 mlxlogscore=620 adultscore=0 phishscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2201110000
 definitions=main-2202250082
X-Proofpoint-GUID: R7t6jOj3xsZoOzvOVE4P-0pRFN7EuFkj
X-Proofpoint-ORIG-GUID: R7t6jOj3xsZoOzvOVE4P-0pRFN7EuFkj
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H5,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

* Vlastimil Babka <vbabka@suse.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 >=3D MERGE_OK && merge_next >=3D MERGE_OK) {
> >=20
> > 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 lik=
e
>=20
> It's similar to e.g. enum compact_priority where specific values are defi=
ned
> as well as more abstract aliases.
>=20
> > 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..
>=20
> 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 singl=
e
> 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?

>=20
> Note that the tracepoint is being provided as part of the series mainly t=
o
> allow evaluation of the series. If it's deemed too specific to be include=
d
> 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.

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