All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Mike Rapoport <rppt@kernel.org>
Subject: Re: [PATCH] arm: dma-mapping: don't call folio_next() beyond the requested region
Date: Tue, 22 Aug 2023 18:12:10 +0100	[thread overview]
Message-ID: <ZOTsamBusDJu7wY0@casper.infradead.org> (raw)
In-Reply-To: <ZNTEoUB7V5BtNvfp@shell.armlinux.org.uk>

On Thu, Aug 10, 2023 at 12:06:09PM +0100, Russell King (Oracle) wrote:
> However, consider what happens with the above when offset is larger
> than the first folio size. To show this, let's rewrite it:

Hmm.  I thought 'off' had to be smaller than PAGE_SIZE.

> So, in all, to me it looks like this conversion is basically wrong, and it
> needs to be something like:
> 
> 		size_t left = size;
> 
> 		while (off >= folio_size(folio)) {
> 			off -= folio_size(folio);
> 			folio = folio_next(folio);
> 		}


We can jump straight to the first folio without iterating over the
folios in between.  Like so:

static void __dma_page_dev_to_cpu(struct page *page, unsigned long off,
        size_t size, enum dma_data_direction dir)
{
        phys_addr_t paddr = page_to_phys(page) + off;

...

        if (dir != DMA_TO_DEVICE && size >= PAGE_SIZE) {
                struct folio *folio = pfn_folio(paddr / PAGE_SIZE);
                size_t offset = offset_in_folio(folio, paddr);

                for (;;) {
                        size_t sz = folio_size(folio) - offset;

                        if (size < sz)
                                break;
                        if (!offset)
                                set_bit(PG_dcache_clean, &folio->flags);
                        offset = 0;
                        size -= sz;
                        if (!size)
                                break;
                        folio = folio_next(folio);
                }
        }

Advantages:
 * No more signed arithmetic
 * Not even an intended arithmetic overflow
 * Only one call to folio_size() per loop
 * Folded the first conditional into the loop

Disadvantages:
 * Some maintainers don't like a for (;;) loop, or a two-exit loop.
   (we could remove the for (;;) by moving 'sz' outside the loop and
    recalculating it at the end of the loop)

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

WARNING: multiple messages have this Message-ID (diff)
From: Matthew Wilcox <willy@infradead.org>
To: "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Andrew Morton <akpm@linux-foundation.org>,
	Mike Rapoport <rppt@kernel.org>
Subject: Re: [PATCH] arm: dma-mapping: don't call folio_next() beyond the requested region
Date: Tue, 22 Aug 2023 18:12:10 +0100	[thread overview]
Message-ID: <ZOTsamBusDJu7wY0@casper.infradead.org> (raw)
In-Reply-To: <ZNTEoUB7V5BtNvfp@shell.armlinux.org.uk>

On Thu, Aug 10, 2023 at 12:06:09PM +0100, Russell King (Oracle) wrote:
> However, consider what happens with the above when offset is larger
> than the first folio size. To show this, let's rewrite it:

Hmm.  I thought 'off' had to be smaller than PAGE_SIZE.

> So, in all, to me it looks like this conversion is basically wrong, and it
> needs to be something like:
> 
> 		size_t left = size;
> 
> 		while (off >= folio_size(folio)) {
> 			off -= folio_size(folio);
> 			folio = folio_next(folio);
> 		}


We can jump straight to the first folio without iterating over the
folios in between.  Like so:

static void __dma_page_dev_to_cpu(struct page *page, unsigned long off,
        size_t size, enum dma_data_direction dir)
{
        phys_addr_t paddr = page_to_phys(page) + off;

...

        if (dir != DMA_TO_DEVICE && size >= PAGE_SIZE) {
                struct folio *folio = pfn_folio(paddr / PAGE_SIZE);
                size_t offset = offset_in_folio(folio, paddr);

                for (;;) {
                        size_t sz = folio_size(folio) - offset;

                        if (size < sz)
                                break;
                        if (!offset)
                                set_bit(PG_dcache_clean, &folio->flags);
                        offset = 0;
                        size -= sz;
                        if (!size)
                                break;
                        folio = folio_next(folio);
                }
        }

Advantages:
 * No more signed arithmetic
 * Not even an intended arithmetic overflow
 * Only one call to folio_size() per loop
 * Folded the first conditional into the loop

Disadvantages:
 * Some maintainers don't like a for (;;) loop, or a two-exit loop.
   (we could remove the for (;;) by moving 'sz' outside the loop and
    recalculating it at the end of the loop)

  reply	other threads:[~2023-08-22 17:12 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CGME20230810092017eucas1p23930e5e5ec443ac7776002f2f09967d4@eucas1p2.samsung.com>
2023-08-10  9:19 ` [PATCH] arm: dma-mapping: don't call folio_next() beyond the requested region Marek Szyprowski
2023-08-10  9:19   ` Marek Szyprowski
2023-08-10 11:06   ` Russell King (Oracle)
2023-08-10 11:06     ` Russell King (Oracle)
2023-08-22 17:12     ` Matthew Wilcox [this message]
2023-08-22 17:12       ` Matthew Wilcox

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZOTsamBusDJu7wY0@casper.infradead.org \
    --to=willy@infradead.org \
    --cc=akpm@linux-foundation.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=m.szyprowski@samsung.com \
    --cc=rppt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.