* Re: [RFC PATCH v2 1/7] mm: page_alloc: avoid merging non-fallbackable pageblocks with others.
From: Eric Ren @ 2021-12-10 7:43 UTC (permalink / raw)
To: Zi Yan, David Hildenbrand, linux-mm
Cc: Mel Gorman, Robin Murphy, linux-kernel, iommu, virtualization,
linuxppc-dev, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
In-Reply-To: <20211209230414.2766515-2-zi.yan@sent.com>
Hi,
On 2021/12/10 07:04, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
>
> This is done in addition to MIGRATE_ISOLATE pageblock merge avoidance.
> It prepares for the upcoming removal of the MAX_ORDER-1 alignment
> requirement for CMA and alloc_contig_range().
>
> MIGRARTE_HIGHATOMIC should not merge with other migratetypes like
> MIGRATE_ISOLATE and MIGRARTE_CMA[1], so this commit prevents that too.
> Also add MIGRARTE_HIGHATOMIC to fallbacks array for completeness.
>
> [1] https://lore.kernel.org/linux-mm/20211130100853.GP3366@techsingularity.net/
>
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
> include/linux/mmzone.h | 6 ++++++
> mm/page_alloc.c | 28 ++++++++++++++++++----------
> 2 files changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 58e744b78c2c..b925431b0123 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -83,6 +83,12 @@ static inline bool is_migrate_movable(int mt)
> return is_migrate_cma(mt) || mt == MIGRATE_MOVABLE;
> }
>
> +/* See fallbacks[MIGRATE_TYPES][3] in page_alloc.c */
> +static inline bool migratetype_has_fallback(int mt)
> +{
> + return mt < MIGRATE_PCPTYPES;
> +}
> +
I would suggest spliting the patch into 2 parts. The first part: no
functioning change, just introduce migratetype_has_fallback()
and replace where it applys to.
> #define for_each_migratetype_order(order, type) \
> for (order = 0; order < MAX_ORDER; order++) \
> for (type = 0; type < MIGRATE_TYPES; type++)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index edfd6c81af82..107a5f186d3b 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -1041,6 +1041,12 @@ buddy_merge_likely(unsigned long pfn, unsigned long buddy_pfn,
> return page_is_buddy(higher_page, higher_buddy, order + 1);
> }
>
> +static inline bool has_non_fallback_pageblock(struct zone *zone)
> +{
> + return has_isolate_pageblock(zone) || zone_cma_pages(zone) != 0 ||
> + zone->nr_reserved_highatomic != 0;
Make zone->nr_reserved_highatomic != 0 a helper as zone_cma_pages()?
> +}
> +
> /*
> * Freeing function for a buddy system allocator.
> *
> @@ -1116,14 +1122,15 @@ static inline void __free_one_page(struct page *page,
> }
> if (order < MAX_ORDER - 1) {
> /* If we are here, it means order is >= pageblock_order.
> - * We want to prevent merge between freepages on isolate
> - * pageblock and normal pageblock. Without this, pageblock
> - * isolation could cause incorrect freepage or CMA accounting.
> + * We want to prevent merge between freepages on pageblock
> + * without fallbacks and normal pageblock. Without this,
> + * pageblock isolation could cause incorrect freepage or CMA
> + * accounting or HIGHATOMIC accounting.
> *
> * We don't want to hit this code for the more frequent
> * low-order merging.
> */
> - if (unlikely(has_isolate_pageblock(zone))) {
> + if (unlikely(has_non_fallback_pageblock(zone))) {
I'm not familiar with the code details, just wondering if this change
would has side effects on cma
pageblock merging as it the condition stronger?
Thanks,
Eric
> int buddy_mt;
>
> buddy_pfn = __find_buddy_pfn(pfn, order);
> @@ -1131,8 +1138,8 @@ static inline void __free_one_page(struct page *page,
> buddy_mt = get_pageblock_migratetype(buddy);
>
> if (migratetype != buddy_mt
> - && (is_migrate_isolate(migratetype) ||
> - is_migrate_isolate(buddy_mt)))
> + && (!migratetype_has_fallback(migratetype) ||
> + !migratetype_has_fallback(buddy_mt)))
> goto done_merging;
> }
> max_order = order + 1;
> @@ -2483,6 +2490,7 @@ static int fallbacks[MIGRATE_TYPES][3] = {
> [MIGRATE_UNMOVABLE] = { MIGRATE_RECLAIMABLE, MIGRATE_MOVABLE, MIGRATE_TYPES },
> [MIGRATE_MOVABLE] = { MIGRATE_RECLAIMABLE, MIGRATE_UNMOVABLE, MIGRATE_TYPES },
> [MIGRATE_RECLAIMABLE] = { MIGRATE_UNMOVABLE, MIGRATE_MOVABLE, MIGRATE_TYPES },
> + [MIGRATE_HIGHATOMIC] = { MIGRATE_TYPES }, /* Never used */
> #ifdef CONFIG_CMA
> [MIGRATE_CMA] = { MIGRATE_TYPES }, /* Never used */
> #endif
> @@ -2794,8 +2802,8 @@ static void reserve_highatomic_pageblock(struct page *page, struct zone *zone,
>
> /* Yoink! */
> mt = get_pageblock_migratetype(page);
> - if (!is_migrate_highatomic(mt) && !is_migrate_isolate(mt)
> - && !is_migrate_cma(mt)) {
> + /* Only reserve normal pageblock */
> + if (migratetype_has_fallback(mt)) {
> zone->nr_reserved_highatomic += pageblock_nr_pages;
> set_pageblock_migratetype(page, MIGRATE_HIGHATOMIC);
> move_freepages_block(zone, page, MIGRATE_HIGHATOMIC, NULL);
> @@ -3544,8 +3552,8 @@ int __isolate_free_page(struct page *page, unsigned int order)
> struct page *endpage = page + (1 << order) - 1;
> for (; page < endpage; page += pageblock_nr_pages) {
> int mt = get_pageblock_migratetype(page);
> - if (!is_migrate_isolate(mt) && !is_migrate_cma(mt)
> - && !is_migrate_highatomic(mt))
> + /* Only change normal pageblock */
> + if (migratetype_has_fallback(mt))
> set_pageblock_migratetype(page,
> MIGRATE_MOVABLE);
> }
^ permalink raw reply
* Re: [RFC PATCH v2 3/7] mm: migrate: allocate the right size of non hugetlb or THP compound pages.
From: Eric Ren @ 2021-12-10 7:53 UTC (permalink / raw)
To: Zi Yan, David Hildenbrand, linux-mm
Cc: Mel Gorman, Robin Murphy, linux-kernel, iommu, virtualization,
linuxppc-dev, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
In-Reply-To: <20211209230414.2766515-4-zi.yan@sent.com>
Hi,
On 2021/12/10 07:04, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
>
> alloc_migration_target() is used by alloc_contig_range() and non-LRU
> movable compound pages can be migrated. Current code does not allocate the
> right page size for such pages. Check THP precisely using
> is_transparent_huge() and add allocation support for non-LRU compound
> pages.
Could you elaborate on why the current code doesn't get the right size?
how this patch fixes it.
The description sounds like it's an existing bug, if so, the patch
subject should be changed to
"Fixes ..."?
>
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
> mm/migrate.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/mm/migrate.c b/mm/migrate.c
> index d487a399253b..2ce3c771b1de 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -1563,7 +1563,7 @@ struct page *alloc_migration_target(struct page *page, unsigned long private)
> return alloc_huge_page_nodemask(h, nid, mtc->nmask, gfp_mask);
> }
>
> - if (PageTransHuge(page)) {
> + if (is_transparent_hugepage(page)) {
> /*
> * clear __GFP_RECLAIM to make the migration callback
> * consistent with regular THP allocations.
> @@ -1572,13 +1572,17 @@ struct page *alloc_migration_target(struct page *page, unsigned long private)
if (PageTransHuge(page)) { // just give more code context
...
> gfp_mask |= GFP_TRANSHUGE;
> order = HPAGE_PMD_ORDER;
order assigned here
> }
> + if (PageCompound(page)) {
> + gfp_mask |= __GFP_COMP;
> + order = compound_order(page);
re-assinged again as THP is a compound page?
Thanks,
Eric
> + }
> zidx = zone_idx(page_zone(page));
> if (is_highmem_idx(zidx) || zidx == ZONE_MOVABLE)
> gfp_mask |= __GFP_HIGHMEM;
>
> new_page = __alloc_pages(gfp_mask, order, nid, mtc->nmask);
>
> - if (new_page && PageTransHuge(new_page))
> + if (new_page && is_transparent_hugepage(page))
> prep_transhuge_page(new_page);
>
> return new_page;
^ permalink raw reply
* Re: [RFC PATCH v2 4/7] mm: make alloc_contig_range work at pageblock granularity
From: Eric Ren @ 2021-12-10 8:12 UTC (permalink / raw)
To: Zi Yan, David Hildenbrand, linux-mm
Cc: Mel Gorman, Robin Murphy, linux-kernel, iommu, virtualization,
linuxppc-dev, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
In-Reply-To: <20211209230414.2766515-5-zi.yan@sent.com>
Hi,
On 2021/12/10 07:04, Zi Yan wrote:
> From: Zi Yan <ziy@nvidia.com>
>
> alloc_contig_range() worked at MAX_ORDER-1 granularity to avoid merging
> pageblocks with different migratetypes. It might unnecessarily convert
> extra pageblocks at the beginning and at the end of the range. Change
> alloc_contig_range() to work at pageblock granularity.
>
> It is done by restoring pageblock types and split >pageblock_order free
> pages after isolating at MAX_ORDER-1 granularity and migrating pages
> away at pageblock granularity. The reason for this process is that
> during isolation, some pages, either free or in-use, might have >pageblock
> sizes and isolating part of them can cause free accounting issues.
Could you elaborate on how the acconting issue would happen in details?
> Restoring the migratetypes of the pageblocks not in the interesting
> range later is much easier.
>
> Signed-off-by: Zi Yan <ziy@nvidia.com>
> ---
> mm/page_alloc.c | 169 ++++++++++++++++++++++++++++++++++++++++++------
> 1 file changed, 149 insertions(+), 20 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 107a5f186d3b..5ffbeb1b7512 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8981,8 +8981,8 @@ struct page *has_unmovable_pages(struct zone *zone, struct page *page,
> #ifdef CONFIG_CONTIG_ALLOC
> static unsigned long pfn_max_align_down(unsigned long pfn)
> {
> - return pfn & ~(max_t(unsigned long, MAX_ORDER_NR_PAGES,
> - pageblock_nr_pages) - 1);
> + return ALIGN_DOWN(pfn, max_t(unsigned long, MAX_ORDER_NR_PAGES,
> + pageblock_nr_pages));
> }
>
> static unsigned long pfn_max_align_up(unsigned long pfn)
> @@ -9071,6 +9071,52 @@ static int __alloc_contig_migrate_range(struct compact_control *cc,
> return 0;
> }
>
> +static inline int save_migratetypes(unsigned char *migratetypes,
> + unsigned long start_pfn, unsigned long end_pfn)
> +{
> + unsigned long pfn = start_pfn;
> + int num = 0;
> +
> + while (pfn < end_pfn) {
> + migratetypes[num] = get_pageblock_migratetype(pfn_to_page(pfn));
> + num++;
> + pfn += pageblock_nr_pages;
> + }
> + return num;
> +}
> +
> +static inline int restore_migratetypes(unsigned char *migratetypes,
> + unsigned long start_pfn, unsigned long end_pfn)
> +{
> + unsigned long pfn = start_pfn;
> + int num = 0;
> +
> + while (pfn < end_pfn) {
> + set_pageblock_migratetype(pfn_to_page(pfn), migratetypes[num]);
> + num++;
> + pfn += pageblock_nr_pages;
> + }
> + return num;
> +}
> +
> +static inline void split_free_page_into_pageblocks(struct page *free_page,
> + int order, struct zone *zone)
> +{
> + unsigned long pfn;
> +
> + spin_lock(&zone->lock);
> + del_page_from_free_list(free_page, zone, order);
> + for (pfn = page_to_pfn(free_page);
> + pfn < page_to_pfn(free_page) + (1UL << order);
> + pfn += pageblock_nr_pages) {
> + int mt = get_pfnblock_migratetype(pfn_to_page(pfn), pfn);
> +
> + __free_one_page(pfn_to_page(pfn), pfn, zone, pageblock_order,
> + mt, FPI_NONE);
> + }
> + spin_unlock(&zone->lock);
> +}
> +
> /**
> * alloc_contig_range() -- tries to allocate given range of pages
> * @start: start PFN to allocate
> @@ -9096,8 +9142,15 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> unsigned migratetype, gfp_t gfp_mask)
> {
> unsigned long outer_start, outer_end;
> + unsigned long isolate_start = pfn_max_align_down(start);
> + unsigned long isolate_end = pfn_max_align_up(end);
> + unsigned long alloc_start = ALIGN_DOWN(start, pageblock_nr_pages);
> + unsigned long alloc_end = ALIGN(end, pageblock_nr_pages);
What is the differecence between isolate_* and alloc_*?
> + unsigned long num_pageblock_to_save;
> unsigned int order;
> int ret = 0;
> + unsigned char *saved_mt;
> + int num;
>
> struct compact_control cc = {
> .nr_migratepages = 0,
> @@ -9111,11 +9164,30 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> };
> INIT_LIST_HEAD(&cc.migratepages);
>
> + /*
> + * TODO: make MIGRATE_ISOLATE a standalone bit to avoid overwriting
> + * the exiting migratetype. Then, we will not need the save and restore
> + * process here.
> + */
> +
> + /* Save the migratepages of the pageblocks before start and after end */
> + num_pageblock_to_save = (alloc_start - isolate_start) / pageblock_nr_pages
> + + (isolate_end - alloc_end) / pageblock_nr_pages;
> + saved_mt =
> + kmalloc_array(num_pageblock_to_save,
> + sizeof(unsigned char), GFP_KERNEL);
> + if (!saved_mt)
> + return -ENOMEM;
> +
> + num = save_migratetypes(saved_mt, isolate_start, alloc_start);
> +
> + num = save_migratetypes(&saved_mt[num], alloc_end, isolate_end);
the two lines above looks confusing to figure out the logic. Could you
help explain a bit?
Thanks,
Eric
> +
> /*
> * What we do here is we mark all pageblocks in range as
> * MIGRATE_ISOLATE. Because pageblock and max order pages may
> * have different sizes, and due to the way page allocator
> - * work, we align the range to biggest of the two pages so
> + * work, we align the isolation range to biggest of the two so
> * that page allocator won't try to merge buddies from
> * different pageblocks and change MIGRATE_ISOLATE to some
> * other migration type.
> @@ -9125,6 +9197,20 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> * we are interested in). This will put all the pages in
> * range back to page allocator as MIGRATE_ISOLATE.
> *
> + * Afterwards, we restore the migratetypes of the pageblocks not
> + * in range, split free pages spanning outside the range,
> + * and put split free pages (at pageblock_order) to the right
> + * migratetype list.
> + *
> + * NOTE: the above approach is used because it can cause free
> + * page accounting issues during isolation, if a page, either
> + * free or in-use, contains multiple pageblocks and we only
> + * isolate a subset of them. For example, if only the second
> + * pageblock is isolated from a page with 2 pageblocks, after
> + * the page is free, it will be put in the first pageblock
> + * migratetype list instead of having 2 pageblocks in two
> + * separate migratetype lists.
> + *
> * When this is done, we take the pages in range from page
> * allocator removing them from the buddy system. This way
> * page allocator will never consider using them.
> @@ -9135,10 +9221,9 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> * put back to page allocator so that buddy can use them.
> */
>
> - ret = start_isolate_page_range(pfn_max_align_down(start),
> - pfn_max_align_up(end), migratetype, 0);
> + ret = start_isolate_page_range(isolate_start, isolate_end, migratetype, 0);
> if (ret)
> - return ret;
> + goto done;
>
> drain_all_pages(cc.zone);
>
> @@ -9174,6 +9259,19 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> * isolated thus they won't get removed from buddy.
> */
>
> + /*
> + * Restore migratetypes of pageblocks outside [start, end)
> + * TODO: remove it when MIGRATE_ISOLATE becomes a standalone bit
> + */
> +
> + num = restore_migratetypes(saved_mt, isolate_start, alloc_start);
> +
> + num = restore_migratetypes(&saved_mt[num], alloc_end, isolate_end);
> +
> + /*
> + * Split free page spanning [isolate_start, alloc_start) and put the
> + * pageblocks in the right migratetype lists.
> + */
> order = 0;
> outer_start = start;
> while (!PageBuddy(pfn_to_page(outer_start))) {
> @@ -9188,37 +9286,68 @@ int alloc_contig_range(unsigned long start, unsigned long end,
> order = buddy_order(pfn_to_page(outer_start));
>
> /*
> - * outer_start page could be small order buddy page and
> - * it doesn't include start page. Adjust outer_start
> - * in this case to report failed page properly
> - * on tracepoint in test_pages_isolated()
> + * split the free page has start page and put the pageblocks
> + * in the right migratetype list
> */
> - if (outer_start + (1UL << order) <= start)
> - outer_start = start;
> + if (outer_start + (1UL << order) > start) {
> + struct page *free_page = pfn_to_page(outer_start);
> +
> + split_free_page_into_pageblocks(free_page, order, cc.zone);
> + }
> + }
> +
> + /*
> + * Split free page spanning [alloc_end, isolate_end) and put the
> + * pageblocks in the right migratetype list
> + */
> + order = 0;
> + outer_end = end;
> + while (!PageBuddy(pfn_to_page(outer_end))) {
> + if (++order >= MAX_ORDER) {
> + outer_end = end;
> + break;
> + }
> + outer_end &= ~0UL << order;
> + }
> +
> + if (outer_end != end) {
> + order = buddy_order(pfn_to_page(outer_end));
> +
> + /*
> + * split the free page has start page and put the pageblocks
> + * in the right migratetype list
> + */
> + VM_BUG_ON(outer_end + (1UL << order) <= end);
> + {
> + struct page *free_page = pfn_to_page(outer_end);
> +
> + split_free_page_into_pageblocks(free_page, order, cc.zone);
> + }
> }
>
> /* Make sure the range is really isolated. */
> - if (test_pages_isolated(outer_start, end, 0)) {
> + if (test_pages_isolated(alloc_start, alloc_end, 0)) {
> ret = -EBUSY;
> goto done;
> }
>
> /* Grab isolated pages from freelists. */
> - outer_end = isolate_freepages_range(&cc, outer_start, end);
> + outer_end = isolate_freepages_range(&cc, alloc_start, alloc_end);
> if (!outer_end) {
> ret = -EBUSY;
> goto done;
> }
>
> /* Free head and tail (if any) */
> - if (start != outer_start)
> - free_contig_range(outer_start, start - outer_start);
> - if (end != outer_end)
> - free_contig_range(end, outer_end - end);
> + if (start != alloc_start)
> + free_contig_range(alloc_start, start - alloc_start);
> + if (end != alloc_end)
> + free_contig_range(end, alloc_end - end);
>
> done:
> - undo_isolate_page_range(pfn_max_align_down(start),
> - pfn_max_align_up(end), migratetype);
> + kfree(saved_mt);
> + undo_isolate_page_range(alloc_start,
> + alloc_end, migratetype);
> return ret;
> }
> EXPORT_SYMBOL(alloc_contig_range);
^ permalink raw reply
* Re: [RFC PATCH v2 3/7] mm: migrate: allocate the right size of non hugetlb or THP compound pages.
From: Yang Shi @ 2021-12-10 17:59 UTC (permalink / raw)
To: Zi Yan
Cc: Mel Gorman, David Hildenbrand, linuxppc-dev,
Linux Kernel Mailing List, virtualization, Linux MM, iommu,
Eric Ren, Robin Murphy, Christoph Hellwig, Vlastimil Babka,
Marek Szyprowski
In-Reply-To: <971750C3-DAEC-4EE8-B838-2DD3CBC29781@nvidia.com>
On Fri, Dec 10, 2021 at 8:08 AM Zi Yan <ziy@nvidia.com> wrote:
>
> On 10 Dec 2021, at 2:53, Eric Ren wrote:
>
> > Hi,
> >
> > On 2021/12/10 07:04, Zi Yan wrote:
> >> From: Zi Yan <ziy@nvidia.com>
> >>
> >> alloc_migration_target() is used by alloc_contig_range() and non-LRU
> >> movable compound pages can be migrated. Current code does not allocate the
> >> right page size for such pages. Check THP precisely using
> >> is_transparent_huge() and add allocation support for non-LRU compound
> >> pages.
> > Could you elaborate on why the current code doesn't get the right size? how this patch fixes it.
>
> The current code only check PageHuge() and PageTransHuge(). Non-LRU compound
> pages will be regarded as PageTransHuge() and an order-9 page is always allocated
> regardless of the actual page order. This patch makes the exact check for
> THPs using is_transparent_huge() instead of PageTransHuge() and checks PageCompound()
> after PageHuge() and is_transparent_huge() for non-LRU compound pages.
>
> >
> > The description sounds like it's an existing bug, if so, the patch subject should be changed to
> > "Fixes ..."?
>
> I have not seen any related bug report.
>
> >
> >>
> >> Signed-off-by: Zi Yan <ziy@nvidia.com>
> >> ---
> >> mm/migrate.c | 8 ++++++--
> >> 1 file changed, 6 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/mm/migrate.c b/mm/migrate.c
> >> index d487a399253b..2ce3c771b1de 100644
> >> --- a/mm/migrate.c
> >> +++ b/mm/migrate.c
> >> @@ -1563,7 +1563,7 @@ struct page *alloc_migration_target(struct page *page, unsigned long private)
> >> return alloc_huge_page_nodemask(h, nid, mtc->nmask, gfp_mask);
> >> }
> >> - if (PageTransHuge(page)) {
> >> + if (is_transparent_hugepage(page)) {
> >> /*
> >> * clear __GFP_RECLAIM to make the migration callback
> >> * consistent with regular THP allocations.
> >> @@ -1572,13 +1572,17 @@ struct page *alloc_migration_target(struct page *page, unsigned long private)
> > if (PageTransHuge(page)) { // just give more code context
> > ...
> >> gfp_mask |= GFP_TRANSHUGE;
> >> order = HPAGE_PMD_ORDER;
> > order assigned here
> >> }
> >> + if (PageCompound(page)) {
> >> + gfp_mask |= __GFP_COMP;
> >> + order = compound_order(page);
> > re-assinged again as THP is a compound page?
>
> Ah, you are right. Will use else if instead. Thanks.
You don't have to use else if, you could just do:
if (PageCompound(page)) {
gfp_mask |= __GFP_COMP;
order = compound_order(page);
}
if (is_transparent_hugepage(page)) {
/* Manipulate THP specific gfp mask */
....
}
>
> > Thanks,
> > Eric
> >> + }
> >> zidx = zone_idx(page_zone(page));
> >> if (is_highmem_idx(zidx) || zidx == ZONE_MOVABLE)
> >> gfp_mask |= __GFP_HIGHMEM;
> >> new_page = __alloc_pages(gfp_mask, order, nid, mtc->nmask);
> >> - if (new_page && PageTransHuge(new_page))
> >> + if (new_page && is_transparent_hugepage(page))
> >> prep_transhuge_page(new_page);
> >> return new_page;
>
>
> --
> Best Regards,
> Yan, Zi
^ permalink raw reply
* Re: [PATCH v4 07/25] reboot: Remove extern annotation from function prototypes
From: Rafael J. Wysocki @ 2021-12-10 18:09 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Rafael J. Wysocki, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Linux-sh list,
Helge Deller, the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <20211126180101.27818-8-digetx@gmail.com>
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> There is no need to annotate function prototypes with 'extern', it makes
> code less readable. Remove unnecessary annotations from <reboot.h>.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
I'm not sure that this is really useful.
Personally, I tend to respect the existing conventions like this.
Surely, this change is not required for the rest of the series to work.
> ---
> include/linux/reboot.h | 38 +++++++++++++++++++-------------------
> 1 file changed, 19 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
> index 7c288013a3ca..b7fa25726323 100644
> --- a/include/linux/reboot.h
> +++ b/include/linux/reboot.h
> @@ -40,36 +40,36 @@ extern int reboot_cpu;
> extern int reboot_force;
>
>
> -extern int register_reboot_notifier(struct notifier_block *);
> -extern int unregister_reboot_notifier(struct notifier_block *);
> +int register_reboot_notifier(struct notifier_block *);
> +int unregister_reboot_notifier(struct notifier_block *);
>
> -extern int devm_register_reboot_notifier(struct device *, struct notifier_block *);
> +int devm_register_reboot_notifier(struct device *, struct notifier_block *);
>
> -extern int register_restart_handler(struct notifier_block *);
> -extern int unregister_restart_handler(struct notifier_block *);
> -extern void do_kernel_restart(char *cmd);
> +int register_restart_handler(struct notifier_block *);
> +int unregister_restart_handler(struct notifier_block *);
> +void do_kernel_restart(char *cmd);
>
> /*
> * Architecture-specific implementations of sys_reboot commands.
> */
>
> -extern void migrate_to_reboot_cpu(void);
> -extern void machine_restart(char *cmd);
> -extern void machine_halt(void);
> -extern void machine_power_off(void);
> +void migrate_to_reboot_cpu(void);
> +void machine_restart(char *cmd);
> +void machine_halt(void);
> +void machine_power_off(void);
>
> -extern void machine_shutdown(void);
> +void machine_shutdown(void);
> struct pt_regs;
> -extern void machine_crash_shutdown(struct pt_regs *);
> +void machine_crash_shutdown(struct pt_regs *);
>
> /*
> * Architecture independent implementations of sys_reboot commands.
> */
>
> -extern void kernel_restart_prepare(char *cmd);
> -extern void kernel_restart(char *cmd);
> -extern void kernel_halt(void);
> -extern void kernel_power_off(void);
> +void kernel_restart_prepare(char *cmd);
> +void kernel_restart(char *cmd);
> +void kernel_halt(void);
> +void kernel_power_off(void);
>
> extern int C_A_D; /* for sysctl */
> void ctrl_alt_del(void);
> @@ -77,15 +77,15 @@ void ctrl_alt_del(void);
> #define POWEROFF_CMD_PATH_LEN 256
> extern char poweroff_cmd[POWEROFF_CMD_PATH_LEN];
>
> -extern void orderly_poweroff(bool force);
> -extern void orderly_reboot(void);
> +void orderly_poweroff(bool force);
> +void orderly_reboot(void);
> void hw_protection_shutdown(const char *reason, int ms_until_forced);
>
> /*
> * Emergency restart, callable from an interrupt handler.
> */
>
> -extern void emergency_restart(void);
> +void emergency_restart(void);
> #include <asm/emergency-restart.h>
>
> #endif /* _LINUX_REBOOT_H */
> --
> 2.33.1
>
^ permalink raw reply
* Re: [PATCH v4 02/25] notifier: Add blocking_notifier_call_chain_is_empty()
From: Rafael J. Wysocki @ 2021-12-10 18:14 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Rafael J. Wysocki, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Linux-sh list,
Helge Deller, the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <20211126180101.27818-3-digetx@gmail.com>
On Fri, Nov 26, 2021 at 7:01 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Add blocking_notifier_call_chain_is_empty() that returns true if call
> chain is empty.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> include/linux/notifier.h | 2 ++
> kernel/notifier.c | 14 ++++++++++++++
> 2 files changed, 16 insertions(+)
>
> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> index 4b80a815b666..924c9d7c8e73 100644
> --- a/include/linux/notifier.h
> +++ b/include/linux/notifier.h
> @@ -173,6 +173,8 @@ int blocking_notifier_call_chain_robust(struct blocking_notifier_head *nh,
> int raw_notifier_call_chain_robust(struct raw_notifier_head *nh,
> unsigned long val_up, unsigned long val_down, void *v);
>
> +bool blocking_notifier_call_chain_is_empty(struct blocking_notifier_head *nh);
> +
> #define NOTIFY_DONE 0x0000 /* Don't care */
> #define NOTIFY_OK 0x0001 /* Suits me */
> #define NOTIFY_STOP_MASK 0x8000 /* Don't call further */
> diff --git a/kernel/notifier.c b/kernel/notifier.c
> index b8251dc0bc0f..b20cb7b9b1f0 100644
> --- a/kernel/notifier.c
> +++ b/kernel/notifier.c
> @@ -322,6 +322,20 @@ int blocking_notifier_call_chain(struct blocking_notifier_head *nh,
> }
> EXPORT_SYMBOL_GPL(blocking_notifier_call_chain);
>
> +/**
> + * blocking_notifier_call_chain_is_empty - Check whether notifier chain is empty
> + * @nh: Pointer to head of the blocking notifier chain
> + *
> + * Checks whether notifier chain is empty.
> + *
> + * Returns true is notifier chain is empty, false otherwise.
> + */
> +bool blocking_notifier_call_chain_is_empty(struct blocking_notifier_head *nh)
> +{
> + return !rcu_access_pointer(nh->head);
> +}
> +EXPORT_SYMBOL_GPL(blocking_notifier_call_chain_is_empty);
The check is not reliable (racy) without locking, so I wouldn't export
anything like this to modules.
At least IMO it should be added along with a user.
^ permalink raw reply
* Re: [PATCH v4 07/25] reboot: Remove extern annotation from function prototypes
From: Dmitry Osipenko @ 2021-12-10 18:15 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <CAJZ5v0i=zgubEtF5-Wnaqa5FMnfVUdSnEmD11-LAuYCH8ZCwrA@mail.gmail.com>
10.12.2021 21:09, Rafael J. Wysocki пишет:
> On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> There is no need to annotate function prototypes with 'extern', it makes
>> code less readable. Remove unnecessary annotations from <reboot.h>.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>
> I'm not sure that this is really useful.
>
> Personally, I tend to respect the existing conventions like this.
>
> Surely, this change is not required for the rest of the series to work.
Problem that such things start to spread all over the kernel with a
copy-paste approach if there is nobody to clean up the code.
This is not a common convention and sometimes it's getting corrected [1].
[1] https://git.kernel.org/linus/6d7434931
^ permalink raw reply
* Re: [PATCH v4 02/25] notifier: Add blocking_notifier_call_chain_is_empty()
From: Dmitry Osipenko @ 2021-12-10 18:19 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <CAJZ5v0gy5M5yYT7k5CY0JtW4MvsgKq4psBEw81UKz=pjGo0xPw@mail.gmail.com>
10.12.2021 21:14, Rafael J. Wysocki пишет:
> On Fri, Nov 26, 2021 at 7:01 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> Add blocking_notifier_call_chain_is_empty() that returns true if call
>> chain is empty.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> include/linux/notifier.h | 2 ++
>> kernel/notifier.c | 14 ++++++++++++++
>> 2 files changed, 16 insertions(+)
>>
>> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
>> index 4b80a815b666..924c9d7c8e73 100644
>> --- a/include/linux/notifier.h
>> +++ b/include/linux/notifier.h
>> @@ -173,6 +173,8 @@ int blocking_notifier_call_chain_robust(struct blocking_notifier_head *nh,
>> int raw_notifier_call_chain_robust(struct raw_notifier_head *nh,
>> unsigned long val_up, unsigned long val_down, void *v);
>>
>> +bool blocking_notifier_call_chain_is_empty(struct blocking_notifier_head *nh);
>> +
>> #define NOTIFY_DONE 0x0000 /* Don't care */
>> #define NOTIFY_OK 0x0001 /* Suits me */
>> #define NOTIFY_STOP_MASK 0x8000 /* Don't call further */
>> diff --git a/kernel/notifier.c b/kernel/notifier.c
>> index b8251dc0bc0f..b20cb7b9b1f0 100644
>> --- a/kernel/notifier.c
>> +++ b/kernel/notifier.c
>> @@ -322,6 +322,20 @@ int blocking_notifier_call_chain(struct blocking_notifier_head *nh,
>> }
>> EXPORT_SYMBOL_GPL(blocking_notifier_call_chain);
>>
>> +/**
>> + * blocking_notifier_call_chain_is_empty - Check whether notifier chain is empty
>> + * @nh: Pointer to head of the blocking notifier chain
>> + *
>> + * Checks whether notifier chain is empty.
>> + *
>> + * Returns true is notifier chain is empty, false otherwise.
>> + */
>> +bool blocking_notifier_call_chain_is_empty(struct blocking_notifier_head *nh)
>> +{
>> + return !rcu_access_pointer(nh->head);
>> +}
>> +EXPORT_SYMBOL_GPL(blocking_notifier_call_chain_is_empty);
>
> The check is not reliable (racy) without locking, so I wouldn't export
> anything like this to modules.
>
> At least IMO it should be added along with a user.
>
I'll remove the export since it's indeed not obvious how other users may
want to use this function.
^ permalink raw reply
* Re: [PATCH v4 03/25] notifier: Add atomic/blocking_notifier_has_unique_priority()
From: Rafael J. Wysocki @ 2021-12-10 18:19 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Rafael J. Wysocki, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Linux-sh list,
Helge Deller, the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <20211126180101.27818-4-digetx@gmail.com>
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Add atomic/blocking_notifier_has_unique_priority() helpers which return
> true if given handler has unique priority.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> include/linux/notifier.h | 5 +++
> kernel/notifier.c | 69 ++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 74 insertions(+)
>
> diff --git a/include/linux/notifier.h b/include/linux/notifier.h
> index 924c9d7c8e73..2c4036f225e1 100644
> --- a/include/linux/notifier.h
> +++ b/include/linux/notifier.h
> @@ -175,6 +175,11 @@ int raw_notifier_call_chain_robust(struct raw_notifier_head *nh,
>
> bool blocking_notifier_call_chain_is_empty(struct blocking_notifier_head *nh);
>
> +bool atomic_notifier_has_unique_priority(struct atomic_notifier_head *nh,
> + struct notifier_block *nb);
> +bool blocking_notifier_has_unique_priority(struct blocking_notifier_head *nh,
> + struct notifier_block *nb);
> +
> #define NOTIFY_DONE 0x0000 /* Don't care */
> #define NOTIFY_OK 0x0001 /* Suits me */
> #define NOTIFY_STOP_MASK 0x8000 /* Don't call further */
> diff --git a/kernel/notifier.c b/kernel/notifier.c
> index b20cb7b9b1f0..7a325b742104 100644
> --- a/kernel/notifier.c
> +++ b/kernel/notifier.c
> @@ -122,6 +122,19 @@ static int notifier_call_chain_robust(struct notifier_block **nl,
> return ret;
> }
>
> +static int notifier_has_unique_priority(struct notifier_block **nl,
> + struct notifier_block *n)
> +{
> + while (*nl && (*nl)->priority >= n->priority) {
> + if ((*nl)->priority == n->priority && *nl != n)
> + return false;
> +
> + nl = &((*nl)->next);
> + }
> +
> + return true;
> +}
> +
> /*
> * Atomic notifier chain routines. Registration and unregistration
> * use a spinlock, and call_chain is synchronized by RCU (no locks).
> @@ -203,6 +216,30 @@ int atomic_notifier_call_chain(struct atomic_notifier_head *nh,
> EXPORT_SYMBOL_GPL(atomic_notifier_call_chain);
> NOKPROBE_SYMBOL(atomic_notifier_call_chain);
>
> +/**
> + * atomic_notifier_has_unique_priority - Checks whether notifier's priority is unique
> + * @nh: Pointer to head of the atomic notifier chain
> + * @n: Entry in notifier chain to check
> + *
> + * Checks whether there is another notifier in the chain with the same priority.
> + * Must be called in process context.
> + *
> + * Returns true if priority is unique, false otherwise.
> + */
> +bool atomic_notifier_has_unique_priority(struct atomic_notifier_head *nh,
> + struct notifier_block *n)
> +{
> + unsigned long flags;
> + bool ret;
> +
> + spin_lock_irqsave(&nh->lock, flags);
> + ret = notifier_has_unique_priority(&nh->head, n);
> + spin_unlock_irqrestore(&nh->lock, flags);
This only works if the caller can prevent new entries from being added
to the list at this point or if the caller knows that they cannot be
added for some reason, but the kerneldoc doesn't mention this
limitation.
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(atomic_notifier_has_unique_priority);
> +
> /*
> * Blocking notifier chain routines. All access to the chain is
> * synchronized by an rwsem.
> @@ -336,6 +373,38 @@ bool blocking_notifier_call_chain_is_empty(struct blocking_notifier_head *nh)
> }
> EXPORT_SYMBOL_GPL(blocking_notifier_call_chain_is_empty);
>
> +/**
> + * blocking_notifier_has_unique_priority - Checks whether notifier's priority is unique
> + * @nh: Pointer to head of the blocking notifier chain
> + * @n: Entry in notifier chain to check
> + *
> + * Checks whether there is another notifier in the chain with the same priority.
> + * Must be called in process context.
> + *
> + * Returns true if priority is unique, false otherwise.
> + */
> +bool blocking_notifier_has_unique_priority(struct blocking_notifier_head *nh,
> + struct notifier_block *n)
> +{
> + bool ret;
> +
> + /*
> + * This code gets used during boot-up, when task switching is
> + * not yet working and interrupts must remain disabled. At such
> + * times we must not call down_read().
> + */
> + if (system_state != SYSTEM_BOOTING)
No, please don't do this, it makes the whole thing error-prone.
> + down_read(&nh->rwsem);
> +
> + ret = notifier_has_unique_priority(&nh->head, n);
> +
> + if (system_state != SYSTEM_BOOTING)
> + up_read(&nh->rwsem);
And still what if a new entry with a non-unique priority is added to
the chain at this point?
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(blocking_notifier_has_unique_priority);
> +
> /*
> * Raw notifier chain routines. There is no protection;
> * the caller must provide it. Use at your own risk!
> --
> 2.33.1
>
^ permalink raw reply
* Re: [PATCH v4 04/25] reboot: Correct typo in a comment
From: Rafael J. Wysocki @ 2021-12-10 18:21 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Rafael J. Wysocki, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Linux-sh list,
Helge Deller, the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <20211126180101.27818-5-digetx@gmail.com>
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Correct s/implemenations/implementations/ in <reboot.h>.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
This patch clearly need not be part of this series.
> ---
> include/linux/reboot.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/linux/reboot.h b/include/linux/reboot.h
> index af907a3d68d1..7c288013a3ca 100644
> --- a/include/linux/reboot.h
> +++ b/include/linux/reboot.h
> @@ -63,7 +63,7 @@ struct pt_regs;
> extern void machine_crash_shutdown(struct pt_regs *);
>
> /*
> - * Architecture independent implemenations of sys_reboot commands.
> + * Architecture independent implementations of sys_reboot commands.
> */
>
> extern void kernel_restart_prepare(char *cmd);
> --
> 2.33.1
>
^ permalink raw reply
* Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority
From: Rafael J. Wysocki @ 2021-12-10 18:27 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Rafael J. Wysocki, Boris Ostrovsky, Catalin Marinas,
Linus Walleij, Dave Hansen, Liam Girdwood, James E.J. Bottomley,
Thierry Reding, Paul Mackerras, Pavel Machek, H. Peter Anvin,
linux-riscv, Vincent Chen, Will Deacon, Greg Ungerer,
Stefano Stabellini, alankao, Yoshinori Sato, Krzysztof Kozlowski,
Linux-sh list, Helge Deller, the arch/x86 maintainers,
Russell King, linux-csky, Jonathan Hunter, ACPI Devel Maling List,
Ingo Molnar, Geert Uytterhoeven, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones,
Michał Mirosław, linux-m68k, Mark Brown,
Borislav Petkov, Greentime Hu, Paul Walmsley, linux-tegra,
Thomas Gleixner, Andy Shevchenko, Linux ARM, Juergen Gross,
Thomas Bogendoerfer, Daniel Lezcano, linux-parisc, Linux PM,
Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <091321ea-4919-0579-88a8-23d05871575d@gmail.com>
On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 29.11.2021 03:26, Michał Mirosław пишет:
> > On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
> >> 28.11.2021 03:28, Michał Mirosław пишет:
> >>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> >>>> Add sanity check which ensures that there are no two restart handlers
> >>>> registered with the same priority. Normally it's a direct sign of a
> >>>> problem if two handlers use the same priority.
> >>>
> >>> The patch doesn't ensure the property that there are no duplicated-priority
> >>> entries on the chain.
> >>
> >> It's not the exact point of this patch.
> >>
> >>> I'd rather see a atomic_notifier_chain_register_unique() that returns
> >>> -EBUSY or something istead of adding an entry with duplicate priority.
> >>> That way it would need only one list traversal unless you want to
> >>> register the duplicate anyway (then you would call the older
> >>> atomic_notifier_chain_register() after reporting the error).
> >>
> >> The point of this patch is to warn developers about the problem that
> >> needs to be fixed. We already have such troubling drivers in mainline.
> >>
> >> It's not critical to register different handlers with a duplicated
> >> priorities, but such cases really need to be corrected. We shouldn't
> >> break users' machines during transition to the new API, meanwhile
> >> developers should take action of fixing theirs drivers.
> >>
> >>> (Or you could return > 0 when a duplicate is registered in
> >>> atomic_notifier_chain_register() if the callers are prepared
> >>> for that. I don't really like this way, though.)
> >>
> >> I had a similar thought at some point before and decided that I'm not in
> >> favor of this approach. It's nicer to have a dedicated function that
> >> verifies the uniqueness, IMO.
> >
> > I don't like the part that it traverses the list second time to check
> > the uniqueness. But actually you could avoid that if
> > notifier_chain_register() would always add equal-priority entries in
> > reverse order:
> >
> > static int notifier_chain_register(struct notifier_block **nl,
> > struct notifier_block *n)
> > {
> > while ((*nl) != NULL) {
> > if (unlikely((*nl) == n)) {
> > WARN(1, "double register detected");
> > return 0;
> > }
> > - if (n->priority > (*nl)->priority)
> > + if (n->priority >= (*nl)->priority)
> > break;
> > nl = &((*nl)->next);
> > }
> > n->next = *nl;
> > rcu_assign_pointer(*nl, n);
> > return 0;
> > }
> >
> > Then the check for uniqueness after adding would be:
> >
> > WARN(nb->next && nb->priority == nb->next->priority);
>
> We can't just change the registration order because invocation order of
> the call chain depends on the registration order
It doesn't if unique priorities are required and isn't that what you want?
> and some of current
> users may rely on that order. I'm pretty sure that changing the order
> will have unfortunate consequences.
Well, the WARN() doesn't help much then.
Either you can make all of the users register with unique priorities,
and then you can make the registration reject non-unique ones, or you
cannot assume them to be unique.
^ permalink raw reply
* Re: [PATCH v4 06/25] reboot: Warn if unregister_restart_handler() fails
From: Rafael J. Wysocki @ 2021-12-10 18:32 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Rafael J. Wysocki, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Linux-sh list,
Helge Deller, the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <20211126180101.27818-7-digetx@gmail.com>
On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> Emit warning if unregister_restart_handler() fails since it never should
> fail. This will ease further API development by catching mistakes early.
>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
> kernel/reboot.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/reboot.c b/kernel/reboot.c
> index e6659ae329f1..f0e7b9c13f6b 100644
> --- a/kernel/reboot.c
> +++ b/kernel/reboot.c
> @@ -210,7 +210,7 @@ EXPORT_SYMBOL(register_restart_handler);
> */
> int unregister_restart_handler(struct notifier_block *nb)
> {
> - return atomic_notifier_chain_unregister(&restart_handler_list, nb);
> + return WARN_ON(atomic_notifier_chain_unregister(&restart_handler_list, nb));
The only reason why it can fail is if the object pointed to by nb is
not in the chain. Why WARN() about this? And what about systems with
panic_on_warn set?
> }
> EXPORT_SYMBOL(unregister_restart_handler);
>
> --
> 2.33.1
>
^ permalink raw reply
* Re: [PATCH v4 07/25] reboot: Remove extern annotation from function prototypes
From: Rafael J. Wysocki @ 2021-12-10 18:35 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Rafael J. Wysocki, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Linux-sh list,
Helge Deller, the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <acf8289e-6ab8-6eda-ec06-e9044ddd9a92@gmail.com>
On Fri, Dec 10, 2021 at 7:16 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 10.12.2021 21:09, Rafael J. Wysocki пишет:
> > On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> There is no need to annotate function prototypes with 'extern', it makes
> >> code less readable. Remove unnecessary annotations from <reboot.h>.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >
> > I'm not sure that this is really useful.
> >
> > Personally, I tend to respect the existing conventions like this.
> >
> > Surely, this change is not required for the rest of the series to work.
>
> Problem that such things start to spread all over the kernel with a
> copy-paste approach if there is nobody to clean up the code.
>
> This is not a common convention and sometimes it's getting corrected [1].
>
> [1] https://git.kernel.org/linus/6d7434931
In separate patches outside of series adding new features, if one is
so inclined.
^ permalink raw reply
* Re: [PATCH v4 03/25] notifier: Add atomic/blocking_notifier_has_unique_priority()
From: Dmitry Osipenko @ 2021-12-10 18:52 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <CAJZ5v0jTJ3f7oUUR690PGaPJsxA8yzua9XDa8MONBHMzHnDfOQ@mail.gmail.com>
10.12.2021 21:19, Rafael J. Wysocki пишет:
...
>> +bool atomic_notifier_has_unique_priority(struct atomic_notifier_head *nh,
>> + struct notifier_block *n)
>> +{
>> + unsigned long flags;
>> + bool ret;
>> +
>> + spin_lock_irqsave(&nh->lock, flags);
>> + ret = notifier_has_unique_priority(&nh->head, n);
>> + spin_unlock_irqrestore(&nh->lock, flags);
>
> This only works if the caller can prevent new entries from being added
> to the list at this point or if the caller knows that they cannot be
> added for some reason, but the kerneldoc doesn't mention this
> limitation.
I'll update the comment.
..
>> +bool blocking_notifier_has_unique_priority(struct blocking_notifier_head *nh,
>> + struct notifier_block *n)
>> +{
>> + bool ret;
>> +
>> + /*
>> + * This code gets used during boot-up, when task switching is
>> + * not yet working and interrupts must remain disabled. At such
>> + * times we must not call down_read().
>> + */
>> + if (system_state != SYSTEM_BOOTING)
>
> No, please don't do this, it makes the whole thing error-prone.
What should I do then?
>> + down_read(&nh->rwsem);
>> +
>> + ret = notifier_has_unique_priority(&nh->head, n);
>> +
>> + if (system_state != SYSTEM_BOOTING)
>> + up_read(&nh->rwsem);
>
> And still what if a new entry with a non-unique priority is added to
> the chain at this point?
If entry with a non-unique priority is added after the check, then
obviously it won't be detected. I don't understand the question. These
down/up_read() are the locks that prevent the race, if that's the question.
^ permalink raw reply
* Re: [PATCH v4 06/25] reboot: Warn if unregister_restart_handler() fails
From: Dmitry Osipenko @ 2021-12-10 18:54 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <CAJZ5v0ii7tGRDbxw+5GqdyONXvRPznXUqBZd03+pdoAd+pH=JQ@mail.gmail.com>
10.12.2021 21:32, Rafael J. Wysocki пишет:
> On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> Emit warning if unregister_restart_handler() fails since it never should
>> fail. This will ease further API development by catching mistakes early.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>> kernel/reboot.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/reboot.c b/kernel/reboot.c
>> index e6659ae329f1..f0e7b9c13f6b 100644
>> --- a/kernel/reboot.c
>> +++ b/kernel/reboot.c
>> @@ -210,7 +210,7 @@ EXPORT_SYMBOL(register_restart_handler);
>> */
>> int unregister_restart_handler(struct notifier_block *nb)
>> {
>> - return atomic_notifier_chain_unregister(&restart_handler_list, nb);
>> + return WARN_ON(atomic_notifier_chain_unregister(&restart_handler_list, nb));
>
> The only reason why it can fail is if the object pointed to by nb is
> not in the chain.
I had exactly this case where object wasn't in the chain due to a bug
and this warning was very helpful.
> Why WARN() about this? And what about systems with
> panic_on_warn set?
That warning condition will never happen normally, only when something
is seriously wrong.
Those systems with panic_on_warn will get what was they asked for.
^ permalink raw reply
* Re: [PATCH v4 07/25] reboot: Remove extern annotation from function prototypes
From: Dmitry Osipenko @ 2021-12-10 18:56 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <CAJZ5v0gvuteY4EtXWTKmh4-Wt-Z_dPcqfDLwc-ja1uovbV3rpw@mail.gmail.com>
10.12.2021 21:35, Rafael J. Wysocki пишет:
> On Fri, Dec 10, 2021 at 7:16 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 10.12.2021 21:09, Rafael J. Wysocki пишет:
>>> On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> There is no need to annotate function prototypes with 'extern', it makes
>>>> code less readable. Remove unnecessary annotations from <reboot.h>.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>
>>> I'm not sure that this is really useful.
>>>
>>> Personally, I tend to respect the existing conventions like this.
>>>
>>> Surely, this change is not required for the rest of the series to work.
>>
>> Problem that such things start to spread all over the kernel with a
>> copy-paste approach if there is nobody to clean up the code.
>>
>> This is not a common convention and sometimes it's getting corrected [1].
>>
>> [1] https://git.kernel.org/linus/6d7434931
>
> In separate patches outside of series adding new features, if one is
> so inclined.
>
Alright, I'll drop this patch then because it can't be done in parallel
without creating the merge conflict. I'll try not to forget to come back
to this later on.
^ permalink raw reply
* Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority
From: Dmitry Osipenko @ 2021-12-10 19:04 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Catalin Marinas, Linus Walleij,
Dave Hansen, Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, xen-devel, linux-mips, Guenter Roeck,
Len Brown, Albert Ou, Lee Jones, Michał Mirosław,
linux-m68k, Mark Brown, Borislav Petkov, Greentime Hu,
Paul Walmsley, linux-tegra, Thomas Gleixner, Andy Shevchenko,
Linux ARM, Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano,
linux-parisc, Linux PM, Sebastian Reichel,
Linux Kernel Mailing List, K . C . Kuen-Chern Lin, Palmer Dabbelt,
Philipp Zabel, Guo Ren, Andrew Morton, linuxppc-dev,
Joshua Thompson
In-Reply-To: <CAJZ5v0jMvdhfBqjY+V9h_Z6EH1ohuJH+KjuGiOw_Jor1Tnp7vg@mail.gmail.com>
10.12.2021 21:27, Rafael J. Wysocki пишет:
> On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 29.11.2021 03:26, Michał Mirosław пишет:
>>> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
>>>> 28.11.2021 03:28, Michał Mirosław пишет:
>>>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
>>>>>> Add sanity check which ensures that there are no two restart handlers
>>>>>> registered with the same priority. Normally it's a direct sign of a
>>>>>> problem if two handlers use the same priority.
>>>>>
>>>>> The patch doesn't ensure the property that there are no duplicated-priority
>>>>> entries on the chain.
>>>>
>>>> It's not the exact point of this patch.
>>>>
>>>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
>>>>> -EBUSY or something istead of adding an entry with duplicate priority.
>>>>> That way it would need only one list traversal unless you want to
>>>>> register the duplicate anyway (then you would call the older
>>>>> atomic_notifier_chain_register() after reporting the error).
>>>>
>>>> The point of this patch is to warn developers about the problem that
>>>> needs to be fixed. We already have such troubling drivers in mainline.
>>>>
>>>> It's not critical to register different handlers with a duplicated
>>>> priorities, but such cases really need to be corrected. We shouldn't
>>>> break users' machines during transition to the new API, meanwhile
>>>> developers should take action of fixing theirs drivers.
>>>>
>>>>> (Or you could return > 0 when a duplicate is registered in
>>>>> atomic_notifier_chain_register() if the callers are prepared
>>>>> for that. I don't really like this way, though.)
>>>>
>>>> I had a similar thought at some point before and decided that I'm not in
>>>> favor of this approach. It's nicer to have a dedicated function that
>>>> verifies the uniqueness, IMO.
>>>
>>> I don't like the part that it traverses the list second time to check
>>> the uniqueness. But actually you could avoid that if
>>> notifier_chain_register() would always add equal-priority entries in
>>> reverse order:
>>>
>>> static int notifier_chain_register(struct notifier_block **nl,
>>> struct notifier_block *n)
>>> {
>>> while ((*nl) != NULL) {
>>> if (unlikely((*nl) == n)) {
>>> WARN(1, "double register detected");
>>> return 0;
>>> }
>>> - if (n->priority > (*nl)->priority)
>>> + if (n->priority >= (*nl)->priority)
>>> break;
>>> nl = &((*nl)->next);
>>> }
>>> n->next = *nl;
>>> rcu_assign_pointer(*nl, n);
>>> return 0;
>>> }
>>>
>>> Then the check for uniqueness after adding would be:
>>>
>>> WARN(nb->next && nb->priority == nb->next->priority);
>>
>> We can't just change the registration order because invocation order of
>> the call chain depends on the registration order
>
> It doesn't if unique priorities are required and isn't that what you want?
>
>> and some of current
>> users may rely on that order. I'm pretty sure that changing the order
>> will have unfortunate consequences.
>
> Well, the WARN() doesn't help much then.
>
> Either you can make all of the users register with unique priorities,
> and then you can make the registration reject non-unique ones, or you
> cannot assume them to be unique.
There is no strong requirement for priorities to be unique, the reboot.c
code will work properly.
The potential problem is on the user's side and the warning is intended
to aid the user.
We can make it a strong requirement, but only after converting and
testing all kernel drivers. I'll consider to add patches for that.
^ permalink raw reply
* Re: [PATCH v4 03/25] notifier: Add atomic/blocking_notifier_has_unique_priority()
From: Rafael J. Wysocki @ 2021-12-10 19:05 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Rafael J. Wysocki, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Linux-sh list,
Helge Deller, the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <e6ff1cea-a168-1cb0-25c5-fb16c681cf4a@gmail.com>
On Fri, Dec 10, 2021 at 7:52 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 10.12.2021 21:19, Rafael J. Wysocki пишет:
> ...
> >> +bool atomic_notifier_has_unique_priority(struct atomic_notifier_head *nh,
> >> + struct notifier_block *n)
> >> +{
> >> + unsigned long flags;
> >> + bool ret;
> >> +
> >> + spin_lock_irqsave(&nh->lock, flags);
> >> + ret = notifier_has_unique_priority(&nh->head, n);
> >> + spin_unlock_irqrestore(&nh->lock, flags);
> >
> > This only works if the caller can prevent new entries from being added
> > to the list at this point or if the caller knows that they cannot be
> > added for some reason, but the kerneldoc doesn't mention this
> > limitation.
>
> I'll update the comment.
>
> ..
> >> +bool blocking_notifier_has_unique_priority(struct blocking_notifier_head *nh,
> >> + struct notifier_block *n)
> >> +{
> >> + bool ret;
> >> +
> >> + /*
> >> + * This code gets used during boot-up, when task switching is
> >> + * not yet working and interrupts must remain disabled. At such
> >> + * times we must not call down_read().
> >> + */
> >> + if (system_state != SYSTEM_BOOTING)
> >
> > No, please don't do this, it makes the whole thing error-prone.
>
> What should I do then?
First of all, do you know of any users who may want to call this
during early initialization? If so, then why may they want to do
that?
Depending on the above, I would consider adding a special mechanism for them.
> >> + down_read(&nh->rwsem);
> >> +
> >> + ret = notifier_has_unique_priority(&nh->head, n);
> >> +
> >> + if (system_state != SYSTEM_BOOTING)
> >> + up_read(&nh->rwsem);
> >
> > And still what if a new entry with a non-unique priority is added to
> > the chain at this point?
>
> If entry with a non-unique priority is added after the check, then
> obviously it won't be detected.
Why isn't this a problem?
> I don't understand the question. These
> down/up_read() are the locks that prevent the race, if that's the question.
Not really, they only prevent the race from occurring while
notifier_has_unique_priority() is running.
If anyone depends on this check for correctness, they need to lock the
rwsem, do the check, do the thing depending on the check while holding
the rwsem and then release the rwsem. Otherwise it is racy.
^ permalink raw reply
* Re: [PATCH v4 06/25] reboot: Warn if unregister_restart_handler() fails
From: Rafael J. Wysocki @ 2021-12-10 19:08 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Rafael J. Wysocki, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Linux-sh list,
Helge Deller, the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <c518ce36-09d2-16a3-cec2-6bab8260e3cf@gmail.com>
On Fri, Dec 10, 2021 at 7:54 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 10.12.2021 21:32, Rafael J. Wysocki пишет:
> > On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> Emit warning if unregister_restart_handler() fails since it never should
> >> fail. This will ease further API development by catching mistakes early.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >> kernel/reboot.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/kernel/reboot.c b/kernel/reboot.c
> >> index e6659ae329f1..f0e7b9c13f6b 100644
> >> --- a/kernel/reboot.c
> >> +++ b/kernel/reboot.c
> >> @@ -210,7 +210,7 @@ EXPORT_SYMBOL(register_restart_handler);
> >> */
> >> int unregister_restart_handler(struct notifier_block *nb)
> >> {
> >> - return atomic_notifier_chain_unregister(&restart_handler_list, nb);
> >> + return WARN_ON(atomic_notifier_chain_unregister(&restart_handler_list, nb));
> >
> > The only reason why it can fail is if the object pointed to by nb is
> > not in the chain.
>
> I had exactly this case where object wasn't in the chain due to a bug
> and this warning was very helpful.
During the development. In production it would be rather annoying.
> > Why WARN() about this? And what about systems with
> > panic_on_warn set?
>
> That warning condition will never happen normally, only when something
> is seriously wrong.
>
> Those systems with panic_on_warn will get what was they asked for.
They may not be asking for panicking on bugs in the reboot notifier
code, though. That's what your change is making them panic on.
^ permalink raw reply
* Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority
From: Rafael J. Wysocki @ 2021-12-10 19:14 UTC (permalink / raw)
To: Dmitry Osipenko
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Rafael J. Wysocki, Boris Ostrovsky, Catalin Marinas,
Linus Walleij, Dave Hansen, Liam Girdwood, James E.J. Bottomley,
Thierry Reding, Paul Mackerras, Pavel Machek, H. Peter Anvin,
linux-riscv, Vincent Chen, Will Deacon, Greg Ungerer,
Stefano Stabellini, alankao, Yoshinori Sato, Krzysztof Kozlowski,
Linux-sh list, Helge Deller, the arch/x86 maintainers,
Russell King, linux-csky, Jonathan Hunter, ACPI Devel Maling List,
Ingo Molnar, Geert Uytterhoeven, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones,
Michał Mirosław, linux-m68k, Mark Brown,
Borislav Petkov, Greentime Hu, Paul Walmsley, linux-tegra,
Thomas Gleixner, Andy Shevchenko, Linux ARM, Juergen Gross,
Thomas Bogendoerfer, Daniel Lezcano, linux-parisc, Linux PM,
Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <45025b2d-4be1-f694-be61-31903795cf5d@gmail.com>
On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>
> 10.12.2021 21:27, Rafael J. Wysocki пишет:
> > On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko <digetx@gmail.com> wrote:
> >>
> >> 29.11.2021 03:26, Michał Mirosław пишет:
> >>> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
> >>>> 28.11.2021 03:28, Michał Mirosław пишет:
> >>>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
> >>>>>> Add sanity check which ensures that there are no two restart handlers
> >>>>>> registered with the same priority. Normally it's a direct sign of a
> >>>>>> problem if two handlers use the same priority.
> >>>>>
> >>>>> The patch doesn't ensure the property that there are no duplicated-priority
> >>>>> entries on the chain.
> >>>>
> >>>> It's not the exact point of this patch.
> >>>>
> >>>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
> >>>>> -EBUSY or something istead of adding an entry with duplicate priority.
> >>>>> That way it would need only one list traversal unless you want to
> >>>>> register the duplicate anyway (then you would call the older
> >>>>> atomic_notifier_chain_register() after reporting the error).
> >>>>
> >>>> The point of this patch is to warn developers about the problem that
> >>>> needs to be fixed. We already have such troubling drivers in mainline.
> >>>>
> >>>> It's not critical to register different handlers with a duplicated
> >>>> priorities, but such cases really need to be corrected. We shouldn't
> >>>> break users' machines during transition to the new API, meanwhile
> >>>> developers should take action of fixing theirs drivers.
> >>>>
> >>>>> (Or you could return > 0 when a duplicate is registered in
> >>>>> atomic_notifier_chain_register() if the callers are prepared
> >>>>> for that. I don't really like this way, though.)
> >>>>
> >>>> I had a similar thought at some point before and decided that I'm not in
> >>>> favor of this approach. It's nicer to have a dedicated function that
> >>>> verifies the uniqueness, IMO.
> >>>
> >>> I don't like the part that it traverses the list second time to check
> >>> the uniqueness. But actually you could avoid that if
> >>> notifier_chain_register() would always add equal-priority entries in
> >>> reverse order:
> >>>
> >>> static int notifier_chain_register(struct notifier_block **nl,
> >>> struct notifier_block *n)
> >>> {
> >>> while ((*nl) != NULL) {
> >>> if (unlikely((*nl) == n)) {
> >>> WARN(1, "double register detected");
> >>> return 0;
> >>> }
> >>> - if (n->priority > (*nl)->priority)
> >>> + if (n->priority >= (*nl)->priority)
> >>> break;
> >>> nl = &((*nl)->next);
> >>> }
> >>> n->next = *nl;
> >>> rcu_assign_pointer(*nl, n);
> >>> return 0;
> >>> }
> >>>
> >>> Then the check for uniqueness after adding would be:
> >>>
> >>> WARN(nb->next && nb->priority == nb->next->priority);
> >>
> >> We can't just change the registration order because invocation order of
> >> the call chain depends on the registration order
> >
> > It doesn't if unique priorities are required and isn't that what you want?
> >
> >> and some of current
> >> users may rely on that order. I'm pretty sure that changing the order
> >> will have unfortunate consequences.
> >
> > Well, the WARN() doesn't help much then.
> >
> > Either you can make all of the users register with unique priorities,
> > and then you can make the registration reject non-unique ones, or you
> > cannot assume them to be unique.
>
> There is no strong requirement for priorities to be unique, the reboot.c
> code will work properly.
In which case adding the WARN() is not appropriate IMV.
Also I've looked at the existing code and at least in some cases the
order in which the notifiers run doesn't matter. I'm not sure what
the purpose of this patch is TBH.
> The potential problem is on the user's side and the warning is intended
> to aid the user.
Unless somebody has the panic_on_warn mentioned previously set and
really the user need not understand what the WARN() is about. IOW,
WARN() helps developers, not users.
> We can make it a strong requirement, but only after converting and
> testing all kernel drivers.
Right.
> I'll consider to add patches for that.
But can you avoid adding more patches to this series?
^ permalink raw reply
* Re: [PATCH v4 03/25] notifier: Add atomic/blocking_notifier_has_unique_priority()
From: Dmitry Osipenko @ 2021-12-10 19:33 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <CAJZ5v0gwnY07vg71_NB8RDWyv84FtMsmx7UTDd8TkUd7vFzc6A@mail.gmail.com>
10.12.2021 22:05, Rafael J. Wysocki пишет:
> On Fri, Dec 10, 2021 at 7:52 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 10.12.2021 21:19, Rafael J. Wysocki пишет:
>> ...
>>>> +bool atomic_notifier_has_unique_priority(struct atomic_notifier_head *nh,
>>>> + struct notifier_block *n)
>>>> +{
>>>> + unsigned long flags;
>>>> + bool ret;
>>>> +
>>>> + spin_lock_irqsave(&nh->lock, flags);
>>>> + ret = notifier_has_unique_priority(&nh->head, n);
>>>> + spin_unlock_irqrestore(&nh->lock, flags);
>>>
>>> This only works if the caller can prevent new entries from being added
>>> to the list at this point or if the caller knows that they cannot be
>>> added for some reason, but the kerneldoc doesn't mention this
>>> limitation.
>>
>> I'll update the comment.
>>
>> ..
>>>> +bool blocking_notifier_has_unique_priority(struct blocking_notifier_head *nh,
>>>> + struct notifier_block *n)
>>>> +{
>>>> + bool ret;
>>>> +
>>>> + /*
>>>> + * This code gets used during boot-up, when task switching is
>>>> + * not yet working and interrupts must remain disabled. At such
>>>> + * times we must not call down_read().
>>>> + */
>>>> + if (system_state != SYSTEM_BOOTING)
>>>
>>> No, please don't do this, it makes the whole thing error-prone.
>>
>> What should I do then?
>
> First of all, do you know of any users who may want to call this
> during early initialization? If so, then why may they want to do
> that?
I'll need to carefully review all those dozens of platform restart
handlers to answer this question.
> Depending on the above, I would consider adding a special mechanism for them.
Please notice that every blocking_notifier_*() function has this
SYSTEM_BOOTING check, it's not my invention. Notifier API needs to be
generic.
>>>> + down_read(&nh->rwsem);
>>>> +
>>>> + ret = notifier_has_unique_priority(&nh->head, n);
>>>> +
>>>> + if (system_state != SYSTEM_BOOTING)
>>>> + up_read(&nh->rwsem);
>>>
>>> And still what if a new entry with a non-unique priority is added to
>>> the chain at this point?
>>
>> If entry with a non-unique priority is added after the check, then
>> obviously it won't be detected.
>
> Why isn't this a problem?>> I don't understand the question. These
>> down/up_read() are the locks that prevent the race, if that's the question.
>
> Not really, they only prevent the race from occurring while
> notifier_has_unique_priority() is running.
>
> If anyone depends on this check for correctness, they need to lock the
> rwsem, do the check, do the thing depending on the check while holding
> the rwsem and then release the rwsem. Otherwise it is racy.
>
It's fine that it's a bit "racy" since in the context of this series. We
always do the check after adding new entry, so it's not a problem.
There are two options:
1. Use blocking_notifier_has_unique_priority() like it's done in this
patchset. Remove it after all drivers are converted to the new API and
add blocking_notifier_chain_register_unique().
2. Add blocking_notifier_chain_register_unique(), but don't let it fail
the registration of non-unique entries until all drivers are converted
to the new API.
^ permalink raw reply
* Re: [PATCH v4 06/25] reboot: Warn if unregister_restart_handler() fails
From: Dmitry Osipenko @ 2021-12-10 19:38 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <CAJZ5v0iP_VnNW3ChmdvRFg1gvwvkDGmqtGnf1oVwQxXY303RkQ@mail.gmail.com>
10.12.2021 22:08, Rafael J. Wysocki пишет:
> On Fri, Dec 10, 2021 at 7:54 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 10.12.2021 21:32, Rafael J. Wysocki пишет:
>>> On Fri, Nov 26, 2021 at 7:02 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> Emit warning if unregister_restart_handler() fails since it never should
>>>> fail. This will ease further API development by catching mistakes early.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>> kernel/reboot.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/kernel/reboot.c b/kernel/reboot.c
>>>> index e6659ae329f1..f0e7b9c13f6b 100644
>>>> --- a/kernel/reboot.c
>>>> +++ b/kernel/reboot.c
>>>> @@ -210,7 +210,7 @@ EXPORT_SYMBOL(register_restart_handler);
>>>> */
>>>> int unregister_restart_handler(struct notifier_block *nb)
>>>> {
>>>> - return atomic_notifier_chain_unregister(&restart_handler_list, nb);
>>>> + return WARN_ON(atomic_notifier_chain_unregister(&restart_handler_list, nb));
>>>
>>> The only reason why it can fail is if the object pointed to by nb is
>>> not in the chain.
>>
>> I had exactly this case where object wasn't in the chain due to a bug
>> and this warning was very helpful.
>
> During the development. In production it would be rather annoying.
>
>>> Why WARN() about this? And what about systems with
>>> panic_on_warn set?
>>
>> That warning condition will never happen normally, only when something
>> is seriously wrong.
>>
>> Those systems with panic_on_warn will get what was they asked for.
>
> They may not be asking for panicking on bugs in the reboot notifier
> code, though. That's what your change is making them panic on.
>
Alright, I'll drop the warnings and turn the warning about uniqueness
into error or warning message.
^ permalink raw reply
* Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority
From: Dmitry Osipenko @ 2021-12-10 19:42 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Catalin Marinas, Linus Walleij,
Dave Hansen, Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, xen-devel, linux-mips, Guenter Roeck,
Len Brown, Albert Ou, Lee Jones, Michał Mirosław,
linux-m68k, Mark Brown, Borislav Petkov, Greentime Hu,
Paul Walmsley, linux-tegra, Thomas Gleixner, Andy Shevchenko,
Linux ARM, Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano,
linux-parisc, Linux PM, Sebastian Reichel,
Linux Kernel Mailing List, K . C . Kuen-Chern Lin, Palmer Dabbelt,
Philipp Zabel, Guo Ren, Andrew Morton, linuxppc-dev,
Joshua Thompson
In-Reply-To: <CAJZ5v0ieTwnBVjW8R_VTdPFH3yr5AwLc+ZEG5N3KrpTH+j8qZw@mail.gmail.com>
10.12.2021 22:14, Rafael J. Wysocki пишет:
> On Fri, Dec 10, 2021 at 8:04 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>
>> 10.12.2021 21:27, Rafael J. Wysocki пишет:
>>> On Mon, Nov 29, 2021 at 12:34 PM Dmitry Osipenko <digetx@gmail.com> wrote:
>>>>
>>>> 29.11.2021 03:26, Michał Mirosław пишет:
>>>>> On Mon, Nov 29, 2021 at 12:06:19AM +0300, Dmitry Osipenko wrote:
>>>>>> 28.11.2021 03:28, Michał Mirosław пишет:
>>>>>>> On Fri, Nov 26, 2021 at 09:00:41PM +0300, Dmitry Osipenko wrote:
>>>>>>>> Add sanity check which ensures that there are no two restart handlers
>>>>>>>> registered with the same priority. Normally it's a direct sign of a
>>>>>>>> problem if two handlers use the same priority.
>>>>>>>
>>>>>>> The patch doesn't ensure the property that there are no duplicated-priority
>>>>>>> entries on the chain.
>>>>>>
>>>>>> It's not the exact point of this patch.
>>>>>>
>>>>>>> I'd rather see a atomic_notifier_chain_register_unique() that returns
>>>>>>> -EBUSY or something istead of adding an entry with duplicate priority.
>>>>>>> That way it would need only one list traversal unless you want to
>>>>>>> register the duplicate anyway (then you would call the older
>>>>>>> atomic_notifier_chain_register() after reporting the error).
>>>>>>
>>>>>> The point of this patch is to warn developers about the problem that
>>>>>> needs to be fixed. We already have such troubling drivers in mainline.
>>>>>>
>>>>>> It's not critical to register different handlers with a duplicated
>>>>>> priorities, but such cases really need to be corrected. We shouldn't
>>>>>> break users' machines during transition to the new API, meanwhile
>>>>>> developers should take action of fixing theirs drivers.
>>>>>>
>>>>>>> (Or you could return > 0 when a duplicate is registered in
>>>>>>> atomic_notifier_chain_register() if the callers are prepared
>>>>>>> for that. I don't really like this way, though.)
>>>>>>
>>>>>> I had a similar thought at some point before and decided that I'm not in
>>>>>> favor of this approach. It's nicer to have a dedicated function that
>>>>>> verifies the uniqueness, IMO.
>>>>>
>>>>> I don't like the part that it traverses the list second time to check
>>>>> the uniqueness. But actually you could avoid that if
>>>>> notifier_chain_register() would always add equal-priority entries in
>>>>> reverse order:
>>>>>
>>>>> static int notifier_chain_register(struct notifier_block **nl,
>>>>> struct notifier_block *n)
>>>>> {
>>>>> while ((*nl) != NULL) {
>>>>> if (unlikely((*nl) == n)) {
>>>>> WARN(1, "double register detected");
>>>>> return 0;
>>>>> }
>>>>> - if (n->priority > (*nl)->priority)
>>>>> + if (n->priority >= (*nl)->priority)
>>>>> break;
>>>>> nl = &((*nl)->next);
>>>>> }
>>>>> n->next = *nl;
>>>>> rcu_assign_pointer(*nl, n);
>>>>> return 0;
>>>>> }
>>>>>
>>>>> Then the check for uniqueness after adding would be:
>>>>>
>>>>> WARN(nb->next && nb->priority == nb->next->priority);
>>>>
>>>> We can't just change the registration order because invocation order of
>>>> the call chain depends on the registration order
>>>
>>> It doesn't if unique priorities are required and isn't that what you want?
>>>
>>>> and some of current
>>>> users may rely on that order. I'm pretty sure that changing the order
>>>> will have unfortunate consequences.
>>>
>>> Well, the WARN() doesn't help much then.
>>>
>>> Either you can make all of the users register with unique priorities,
>>> and then you can make the registration reject non-unique ones, or you
>>> cannot assume them to be unique.
>>
>> There is no strong requirement for priorities to be unique, the reboot.c
>> code will work properly.
>
> In which case adding the WARN() is not appropriate IMV.
>
> Also I've looked at the existing code and at least in some cases the
> order in which the notifiers run doesn't matter. I'm not sure what
> the purpose of this patch is TBH.
The purpose is to let developer know that driver needs to be corrected.
>> The potential problem is on the user's side and the warning is intended
>> to aid the user.
>
> Unless somebody has the panic_on_warn mentioned previously set and
> really the user need not understand what the WARN() is about. IOW,
> WARN() helps developers, not users.
>
>> We can make it a strong requirement, but only after converting and
>> testing all kernel drivers.
>
> Right.
>
>> I'll consider to add patches for that.
>
> But can you avoid adding more patches to this series?
I won't add more patches since such patches can be added only after
completion of transition to the new API of the whole kernel.
^ permalink raw reply
* Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority
From: Dmitry Osipenko @ 2021-12-10 19:44 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Catalin Marinas, Linus Walleij,
Dave Hansen, Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, xen-devel, linux-mips, Guenter Roeck,
Len Brown, Albert Ou, Lee Jones, Michał Mirosław,
linux-m68k, Mark Brown, Borislav Petkov, Greentime Hu,
Paul Walmsley, linux-tegra, Thomas Gleixner, Andy Shevchenko,
Linux ARM, Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano,
linux-parisc, Linux PM, Sebastian Reichel,
Linux Kernel Mailing List, K . C . Kuen-Chern Lin, Palmer Dabbelt,
Philipp Zabel, Guo Ren, Andrew Morton, linuxppc-dev,
Joshua Thompson
In-Reply-To: <45228c88-4d51-591e-5da5-9ec468e71684@gmail.com>
10.12.2021 22:42, Dmitry Osipenko пишет:
...
>>> There is no strong requirement for priorities to be unique, the reboot.c
>>> code will work properly.
>>
>> In which case adding the WARN() is not appropriate IMV.
>>
>> Also I've looked at the existing code and at least in some cases the
>> order in which the notifiers run doesn't matter. I'm not sure what
>> the purpose of this patch is TBH.
>
> The purpose is to let developer know that driver needs to be corrected.
>
>>> The potential problem is on the user's side and the warning is intended
>>> to aid the user.
>>
>> Unless somebody has the panic_on_warn mentioned previously set and
>> really the user need not understand what the WARN() is about. IOW,
>> WARN() helps developers, not users.
>>
>>> We can make it a strong requirement, but only after converting and
>>> testing all kernel drivers.
>>
>> Right.
>>
>>> I'll consider to add patches for that.
>>
>> But can you avoid adding more patches to this series?
>
> I won't add more patches since such patches can be added only after
> completion of transition to the new API of the whole kernel.
>
Thank you for the review.
^ permalink raw reply
* Re: [PATCH v4 05/25] reboot: Warn if restart handler has duplicated priority
From: Dmitry Osipenko @ 2021-12-10 19:49 UTC (permalink / raw)
To: Rafael J. Wysocki, Michał Mirosław
Cc: Ulf Hansson, Rich Felker, linux-ia64, Santosh Shilimkar,
Linux-sh list, Boris Ostrovsky, Linus Walleij, Dave Hansen,
Liam Girdwood, James E.J. Bottomley, Thierry Reding,
Paul Mackerras, Pavel Machek, H. Peter Anvin, linux-riscv,
Vincent Chen, Will Deacon, Greg Ungerer, Stefano Stabellini,
alankao, Yoshinori Sato, Krzysztof Kozlowski, Helge Deller,
the arch/x86 maintainers, Russell King, linux-csky,
Jonathan Hunter, ACPI Devel Maling List, Ingo Molnar,
Geert Uytterhoeven, Catalin Marinas, xen-devel, linux-mips,
Guenter Roeck, Len Brown, Albert Ou, Lee Jones, linux-m68k,
Mark Brown, Borislav Petkov, Greentime Hu, Paul Walmsley,
linux-tegra, Thomas Gleixner, Andy Shevchenko, Linux ARM,
Juergen Gross, Thomas Bogendoerfer, Daniel Lezcano, linux-parisc,
Linux PM, Sebastian Reichel, Linux Kernel Mailing List,
K . C . Kuen-Chern Lin, Palmer Dabbelt, Philipp Zabel, Guo Ren,
Andrew Morton, linuxppc-dev, Joshua Thompson
In-Reply-To: <ad6c7d73-e7d3-4901-fd63-ef87eecd39a2@gmail.com>
10.12.2021 22:44, Dmitry Osipenko пишет:
> 10.12.2021 22:42, Dmitry Osipenko пишет:
> ...
>>>> There is no strong requirement for priorities to be unique, the reboot.c
>>>> code will work properly.
>>>
>>> In which case adding the WARN() is not appropriate IMV.
>>>
>>> Also I've looked at the existing code and at least in some cases the
>>> order in which the notifiers run doesn't matter. I'm not sure what
>>> the purpose of this patch is TBH.
>>
>> The purpose is to let developer know that driver needs to be corrected.
>>
>>>> The potential problem is on the user's side and the warning is intended
>>>> to aid the user.
>>>
>>> Unless somebody has the panic_on_warn mentioned previously set and
>>> really the user need not understand what the WARN() is about. IOW,
>>> WARN() helps developers, not users.
>>>
>>>> We can make it a strong requirement, but only after converting and
>>>> testing all kernel drivers.
>>>
>>> Right.
>>>
>>>> I'll consider to add patches for that.
>>>
>>> But can you avoid adding more patches to this series?
>>
>> I won't add more patches since such patches can be added only after
>> completion of transition to the new API of the whole kernel.
>>
>
> Thank you for the review.
>
I meant you, Rafael, and Michał, just in case :)
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox