From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751476AbbCSUWN (ORCPT ); Thu, 19 Mar 2015 16:22:13 -0400 Received: from mailout2.w1.samsung.com ([210.118.77.12]:48527 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751127AbbCSUWM (ORCPT ); Thu, 19 Mar 2015 16:22:12 -0400 X-AuditID: cbfec7f5-b7fc86d0000066b7-c3-550b2f5b7da9 Message-id: <550B2FEF.7060204@partner.samsung.com> Date: Thu, 19 Mar 2015 23:22:07 +0300 From: Stefan Strogin User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0 MIME-version: 1.0 To: Ingo Molnar Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org, Joonsoo Kim , Andrew Morton , Marek Szyprowski , Michal Nazarewicz , aneesh.kumar@linux.vnet.ibm.com, Laurent Pinchart , Dmitry Safonov , Pintu Kumar , Weijie Yang , Laura Abbott , SeongJae Park , Hui Zhu , Minchan Kim , Dyasly Sergey , Vyacheslav Tyrtov , Aleksei Mateosian , gregory.0xf0@gmail.com, sasha.levin@oracle.com, gioh.kim@lge.com, pavel@ucw.cz, stefan.strogin@gmail.com, Steven Rostedt , Ingo Molnar Subject: Re: [PATCH v4 1/5] mm: cma: add trace events to debug physically-contiguous memory allocations References: <550741BD.9080109@partner.samsung.com> <20150317074025.GA27548@gmail.com> In-reply-to: <20150317074025.GA27548@gmail.com> Content-type: text/plain; charset=utf-8 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA02SXUhTYRzGe885O2ebDo6m8aZQMJVKUGtIvEpJeVHnsgsFsaCWDhWcylmK 1o1flVv4kTpdm5mkRjptcwr5UWJLnR8E2pzgZLoyNVZm85s0bXMXeffj+T/8nps/F/e1EAHc tIx7EjZDnC4k+cT4vskSdjPCK/78+0cC9KW+kkJ1ujYSLfyoJ9BayyKJlgo3AKqp1XNQy5Mi Er2Vqygkf9pEIXNvHYnm2g44qH3QRqEGUzEHvdpcpZB2fwygzwMNGLKNDZGobPsjhfpL7Biq 0gSgrdE6HDUOzuJourscQ+afBxRSqnYoNKr/xEH6Iv6VQMZcVooxPWobxRT3WylGU/KMwxha 5SRjWKukGMXKFMaMqHYJxm7sJJg3ew6McS5aCebDr3cYs9pvIZmyrlbAvNZuU8y64dQNn0T+ pWRJelqOhI2IucNPnZwcwLIUMNdcrsTzgdNXAXhcSEdCtbKP8PAJODGnI93sSzcD+MfKKgDf xU4Af2u+AvdBQItg14wKczNBh8DdihWOm0mXaHii1tXhcv3pBDhr5nnqPnCnau7Q70cLYeOG k3I7cfqAhN/3tIfO43QaXB1w4p6xJQCLHc2HUh4dAR3jJtItxekzsLpa6o5x+jTsbFvBKwCt PrKh/t9SH2k1ALwV+Euyk7Jkd1OkonCZWCrLzkgJT8qUGoDnFTa7QfNwtBHQXCD0FhyDXvG+ HHGOLE9qBJCLC/0EhnOuSJAszrsvYTNvs9npEpkRYFxeQD4YCZ2/9UDZESv7Fh00KlrfLBhI FEXPaNhCx3ZX+OOX7EY7Ya3p1wf2/e3dt0QGUeb5QJ/glpWElNnry9ZrmLe+I7g6Ji5m2k+o 4xxE1TbFDl3WaS3P+a22kNwFNs6uFYVVLOdOmZxRBSfzr5YXBcrPGi/a0cOeUkvni/LdLSEh SxVfCMVZmfgfonFQnegCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 17/03/15 10:40, Ingo Molnar wrote: > > * Stefan Strogin wrote: > >>> +TRACE_EVENT(cma_alloc, >>> + >>> + TP_PROTO(struct cma *cma, struct page *page, int count), >>> + >>> + TP_ARGS(cma, page, count), >>> + >>> + TP_STRUCT__entry( >>> + __field(struct page *, page) >>> + __field(unsigned long, count) >>> + ), >>> + >>> + TP_fast_assign( >>> + __entry->page = page; >>> + __entry->count = count; >>> + ), >>> + >>> + TP_printk("page=%p pfn=%lu count=%lu", >>> + __entry->page, >>> + __entry->page ? page_to_pfn(__entry->page) : 0, >>> + __entry->count) > > So I'm wondering, the fast-assign side is not equivalent to the > TP_printk() side: > >>> + __entry->page = page; >>> + __entry->page ? page_to_pfn(__entry->page) : 0, > > to me it seems it would be useful if MM tracing standardized on pfn > printing. Just like you did for trace_cma_release(). > Hello Ingo, thank you for the reply. I afraid there is no special sense in printing both struct page * and pfn. But cma_alloc() returns struct page *, cma_release receives struct page *, and pr_debugs in these functions print struct page *. Maybe it would be better to print the same here too? > Also: > >>> + __entry->page ? page_to_pfn(__entry->page) : 0, > > pfn 0 should probably be reserved for the true 0th pfn - those exist > in some machines. Returning -1ll could be the 'no such pfn' condition? > I took this from trace_mm_page_alloc() and other trace events from trace/events/kmem.h. If we return -1 here to indicate "no such pfn", should we change do this in kmem.h too? >>> + TP_STRUCT__entry( >>> + __field(unsigned long, pfn) > > Btw., does pfn always fit into 32 bits on 32-bit platforms? > Well, I think it does. cma_release() uses 'unsigned long' on all platforms. >>> + __field(unsigned long, count) > > Does this have to be 64-bit on 64-bit platforms? > Oops! I'm terribly wrong. + __field(unsigned int, count) I guess it shouldn't be 64-bit on 64-bit platforms. It's the number of pages being freed, and in cma_release() 'unsigned int' is used for it. >>> + ), >>> + >>> + TP_fast_assign( >>> + __entry->pfn = pfn; >>> + __entry->count = count; >>> + ), >>> + >>> + TP_printk("pfn=%lu page=%p count=%lu", >>> + __entry->pfn, >>> + pfn_to_page(__entry->pfn), >>> + __entry->count) > > So here you print more in the TP_printk() line than in the fast-assign > side. > See above, I think it's the same case as in trace_cma_alloc() TP_printk(). > Again I'd double check the various boundary conditions. > Sorry, I don't quite understand. Boundary conditions are already [should be] checked in cma_alloc()/cma_release, we should only pass to a trace event the information we want to be known, isn't it so? I again terribly sorry, I also completely forgot about struct cma * being passed to trace event. I think either it should be used somehow (e.g. to print the number of CMA region) or shouldn't be passed...