* [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
@ 2023-01-09 14:47 Yishai Hadas
2023-01-09 18:08 ` Logan Gunthorpe
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Yishai Hadas @ 2023-01-09 14:47 UTC (permalink / raw)
To: linux-kernel, linux-block, linux-mm, jgg, axboe, logang
Cc: hch, alex.williamson, yishaih, leonro, maorg
The last_pg is wrong, it is actually the first page of the last
scatterlist element. To get the last page of the last scatterlist
element we have to add prv->length. So it is checking mergability
against the wrong page, Further, a SG element is not guaranteed to end
on a page boundary, so we have to check the sub page location also for
merge eligibility.
Fix the above by checking physical contiguity, compute the actual last
page and then call pages_are_mergable().
Fixes: 1567b49d1a40 ("lib/scatterlist: add check when merging zone device pages")
Reported-by: Jason Gunthorpe <jgg@nvidia.com>
Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
---
lib/scatterlist.c | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/lib/scatterlist.c b/lib/scatterlist.c
index f72aa50c6654..2377917893e7 100644
--- a/lib/scatterlist.c
+++ b/lib/scatterlist.c
@@ -470,22 +470,29 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
return -EOPNOTSUPP;
if (sgt_append->prv) {
+ unsigned long paddr =
+ (page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
+ sgt_append->prv->offset + sgt_append->prv->length) /
+ PAGE_SIZE;
+
if (WARN_ON(offset))
return -EINVAL;
/* Merge contiguous pages into the last SG */
prv_len = sgt_append->prv->length;
- last_pg = sg_page(sgt_append->prv);
- while (n_pages && pages_are_mergeable(pages[0], last_pg)) {
- if (sgt_append->prv->length + PAGE_SIZE > max_segment)
- break;
- sgt_append->prv->length += PAGE_SIZE;
- last_pg = pages[0];
- pages++;
- n_pages--;
+ if (page_to_pfn(pages[0]) == paddr) {
+ last_pg = pfn_to_page(paddr - PAGE_SIZE);
+ while (n_pages && pages_are_mergeable(pages[0], last_pg)) {
+ if (sgt_append->prv->length + PAGE_SIZE > max_segment)
+ break;
+ sgt_append->prv->length += PAGE_SIZE;
+ last_pg = pages[0];
+ pages++;
+ n_pages--;
+ }
+ if (!n_pages)
+ goto out;
}
- if (!n_pages)
- goto out;
}
/* compute number of contiguous chunks */
--
2.18.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
2023-01-09 14:47 [PATCH] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
@ 2023-01-09 18:08 ` Logan Gunthorpe
2023-01-10 3:18 ` Jens Axboe
2023-01-10 5:08 ` Matthew Wilcox
2 siblings, 0 replies; 9+ messages in thread
From: Logan Gunthorpe @ 2023-01-09 18:08 UTC (permalink / raw)
To: Yishai Hadas, linux-kernel, linux-block, linux-mm, jgg, axboe
Cc: hch, alex.williamson, leonro, maorg
On 2023-01-09 07:47, Yishai Hadas wrote:
> The last_pg is wrong, it is actually the first page of the last
> scatterlist element. To get the last page of the last scatterlist
> element we have to add prv->length. So it is checking mergability
> against the wrong page, Further, a SG element is not guaranteed to end
> on a page boundary, so we have to check the sub page location also for
> merge eligibility.
>
> Fix the above by checking physical contiguity, compute the actual last
> page and then call pages_are_mergable().
>
> Fixes: 1567b49d1a40 ("lib/scatterlist: add check when merging zone device pages")
> Reported-by: Jason Gunthorpe <jgg@nvidia.com>
> Signed-off-by: Yishai Hadas <yishaih@nvidia.com>
Looks right by my eye, thanks.
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Logan
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
2023-01-09 14:47 [PATCH] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
2023-01-09 18:08 ` Logan Gunthorpe
@ 2023-01-10 3:18 ` Jens Axboe
2023-01-11 0:03 ` Andrew Morton
2023-01-10 5:08 ` Matthew Wilcox
2 siblings, 1 reply; 9+ messages in thread
From: Jens Axboe @ 2023-01-10 3:18 UTC (permalink / raw)
To: Yishai Hadas, linux-kernel, linux-block, linux-mm, jgg, logang,
Andrew Morton
Cc: hch, alex.williamson, leonro, maorg
On 1/9/23 7:47 AM, Yishai Hadas wrote:
> The last_pg is wrong, it is actually the first page of the last
> scatterlist element. To get the last page of the last scatterlist
> element we have to add prv->length. So it is checking mergability
> against the wrong page, Further, a SG element is not guaranteed to end
> on a page boundary, so we have to check the sub page location also for
> merge eligibility.
>
> Fix the above by checking physical contiguity, compute the actual last
> page and then call pages_are_mergable().
Reviewed-by: Jens Axboe <axboe@kernel.dk>
Andrew, can you pick this one up?
--
Jens Axboe
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
2023-01-09 14:47 [PATCH] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
2023-01-09 18:08 ` Logan Gunthorpe
2023-01-10 3:18 ` Jens Axboe
@ 2023-01-10 5:08 ` Matthew Wilcox
2023-01-10 8:01 ` Yishai Hadas
2 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2023-01-10 5:08 UTC (permalink / raw)
To: Yishai Hadas
Cc: linux-kernel, linux-block, linux-mm, jgg, axboe, logang, hch,
alex.williamson, leonro, maorg
On Mon, Jan 09, 2023 at 04:47:01PM +0200, Yishai Hadas wrote:
> if (sgt_append->prv) {
> + unsigned long paddr =
> + (page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
1. page_to_pfn() * PAGE_SIZE is spelled page_to_phys()
2. physical addresses have type phys_addr_t. Oh, wait, paddr isn't a
physical address, it's a pfn because you divide by PAGE_SIZE at the end.
But you will get truncation on 32-bit, because page_to_pfn() has type
unsigned long. Anyway, this shouldn't be called paddr. Maybe last_pfn
or something?
> + sgt_append->prv->offset + sgt_append->prv->length) /
> + PAGE_SIZE;
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
2023-01-10 5:08 ` Matthew Wilcox
@ 2023-01-10 8:01 ` Yishai Hadas
2023-01-10 13:53 ` Jason Gunthorpe
0 siblings, 1 reply; 9+ messages in thread
From: Yishai Hadas @ 2023-01-10 8:01 UTC (permalink / raw)
To: Matthew Wilcox
Cc: linux-kernel, linux-block, linux-mm, jgg, axboe, logang, hch,
alex.williamson, leonro, maorg
On 10/01/2023 7:08, Matthew Wilcox wrote:
> On Mon, Jan 09, 2023 at 04:47:01PM +0200, Yishai Hadas wrote:
>> if (sgt_append->prv) {
>> + unsigned long paddr =
>> + (page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
> 1. page_to_pfn() * PAGE_SIZE is spelled page_to_phys()
>
> 2. physical addresses have type phys_addr_t. Oh, wait, paddr isn't a
> physical address, it's a pfn because you divide by PAGE_SIZE at the end.
> But you will get truncation on 32-bit, because page_to_pfn() has type
> unsigned long. Anyway, this shouldn't be called paddr. Maybe last_pfn
> or something?
>
>> + sgt_append->prv->offset + sgt_append->prv->length) /
>> + PAGE_SIZE;
In this area of the patch I just reverted to what we had in 6.1 [1]
where we were good.
I can send V1 and replace the 'paddr' to be called 'last_pfn' if it's
really needed.
Alternatively, we can stay with this patch version unless you can see
here any real issue.
What do you suggest ?
[1] https://elixir.bootlin.com/linux/v6.1.4/source/lib/scatterlist.c#L462
Yishai
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
2023-01-10 8:01 ` Yishai Hadas
@ 2023-01-10 13:53 ` Jason Gunthorpe
2023-01-10 15:05 ` Yishai Hadas
0 siblings, 1 reply; 9+ messages in thread
From: Jason Gunthorpe @ 2023-01-10 13:53 UTC (permalink / raw)
To: Yishai Hadas
Cc: Matthew Wilcox, linux-kernel, linux-block, linux-mm, axboe,
logang, hch, alex.williamson, leonro, maorg
On Tue, Jan 10, 2023 at 10:01:53AM +0200, Yishai Hadas wrote:
> On 10/01/2023 7:08, Matthew Wilcox wrote:
> > On Mon, Jan 09, 2023 at 04:47:01PM +0200, Yishai Hadas wrote:
> > > if (sgt_append->prv) {
> > > + unsigned long paddr =
> > > + (page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
> > 1. page_to_pfn() * PAGE_SIZE is spelled page_to_phys()
> >
> > 2. physical addresses have type phys_addr_t. Oh, wait, paddr isn't a
> > physical address, it's a pfn because you divide by PAGE_SIZE at the end.
> > But you will get truncation on 32-bit, because page_to_pfn() has type
> > unsigned long. Anyway, this shouldn't be called paddr. Maybe last_pfn
> > or something?
> >
> > > + sgt_append->prv->offset + sgt_append->prv->length) /
> > > + PAGE_SIZE;
>
> In this area of the patch I just reverted to what we had in 6.1 [1] where we
> were good.
>
> I can send V1 and replace the 'paddr' to be called 'last_pfn' if it's really
> needed.
Please change it as Matthew describes so it is clearer
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
2023-01-10 13:53 ` Jason Gunthorpe
@ 2023-01-10 15:05 ` Yishai Hadas
0 siblings, 0 replies; 9+ messages in thread
From: Yishai Hadas @ 2023-01-10 15:05 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: Matthew Wilcox, linux-kernel, linux-block, linux-mm, axboe,
logang, hch, alex.williamson, leonro, maorg
On 10/01/2023 15:53, Jason Gunthorpe wrote:
> On Tue, Jan 10, 2023 at 10:01:53AM +0200, Yishai Hadas wrote:
>> On 10/01/2023 7:08, Matthew Wilcox wrote:
>>> On Mon, Jan 09, 2023 at 04:47:01PM +0200, Yishai Hadas wrote:
>>>> if (sgt_append->prv) {
>>>> + unsigned long paddr =
>>>> + (page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
>>> 1. page_to_pfn() * PAGE_SIZE is spelled page_to_phys()
>>>
>>> 2. physical addresses have type phys_addr_t. Oh, wait, paddr isn't a
>>> physical address, it's a pfn because you divide by PAGE_SIZE at the end.
>>> But you will get truncation on 32-bit, because page_to_pfn() has type
>>> unsigned long. Anyway, this shouldn't be called paddr. Maybe last_pfn
>>> or something?
>>>
>>>> + sgt_append->prv->offset + sgt_append->prv->length) /
>>>> + PAGE_SIZE;
>> In this area of the patch I just reverted to what we had in 6.1 [1] where we
>> were good.
>>
>> I can send V1 and replace the 'paddr' to be called 'last_pfn' if it's really
>> needed.
> Please change it as Matthew describes so it is clearer
>
> Jason
OK
Once cleaning the code and renaming to 'last_pfn', could also see that
we had to use below '- 1' instead of '- PAGE_SIZE'.
The expected diff compared to V0 can be seen below.
Will repeat some testing and send V1 once be ready.
+++ b/lib/scatterlist.c
@@ -470,18 +470,16 @@ int sg_alloc_append_table_from_pages(struct
sg_append_table *sgt_append,
return -EOPNOTSUPP;
if (sgt_append->prv) {
- unsigned long paddr =
- (page_to_pfn(sg_page(sgt_append->prv)) * PAGE_SIZE +
- sgt_append->prv->offset +
sgt_append->prv->length) /
- PAGE_SIZE;
+ unsigned long last_pfn =
(page_to_phys(sg_page(sgt_append->prv)) +
+ sgt_append->prv->offset +
sgt_append->prv->length) / PAGE_SIZE;
if (WARN_ON(offset))
return -EINVAL;
/* Merge contiguous pages into the last SG */
prv_len = sgt_append->prv->length;
- if (page_to_pfn(pages[0]) == paddr) {
- last_pg = pfn_to_page(paddr - PAGE_SIZE);
+ if (page_to_pfn(pages[0]) == last_pfn) {
+ last_pg = pfn_to_page(last_pfn - 1);
while (n_pages && pages_are_mergeable(pages[0],
last_pg)) {
if (sgt_append->prv->length + PAGE_SIZE
> max_segment)
break;
Yishai
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
2023-01-10 3:18 ` Jens Axboe
@ 2023-01-11 0:03 ` Andrew Morton
2023-01-11 13:28 ` Jason Gunthorpe
0 siblings, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2023-01-11 0:03 UTC (permalink / raw)
To: Jens Axboe
Cc: Yishai Hadas, linux-kernel, linux-block, linux-mm, jgg, logang,
hch, alex.williamson, leonro, maorg
On Mon, 9 Jan 2023 20:18:39 -0700 Jens Axboe <axboe@kernel.dk> wrote:
> On 1/9/23 7:47 AM, Yishai Hadas wrote:
> > The last_pg is wrong, it is actually the first page of the last
> > scatterlist element. To get the last page of the last scatterlist
> > element we have to add prv->length. So it is checking mergability
> > against the wrong page, Further, a SG element is not guaranteed to end
> > on a page boundary, so we have to check the sub page location also for
> > merge eligibility.
> >
> > Fix the above by checking physical contiguity, compute the actual last
> > page and then call pages_are_mergable().
>
> Reviewed-by: Jens Axboe <axboe@kernel.dk>
>
> Andrew, can you pick this one up?
This patch is against Yishai's
CommitDate: Thu Jan 5 16:01:05 2023 -0400
lib/scatterlist: Fix to merge contiguous pages into the last SG properly
which is in -next, apparently via a tree of Jason's. So I'll assume
that Jason will handleit?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] lib/scatterlist: Fix to calculate the last_pg properly
2023-01-11 0:03 ` Andrew Morton
@ 2023-01-11 13:28 ` Jason Gunthorpe
0 siblings, 0 replies; 9+ messages in thread
From: Jason Gunthorpe @ 2023-01-11 13:28 UTC (permalink / raw)
To: Andrew Morton
Cc: Jens Axboe, Yishai Hadas, linux-kernel, linux-block, linux-mm,
logang, hch, alex.williamson, leonro, maorg
On Tue, Jan 10, 2023 at 04:03:47PM -0800, Andrew Morton wrote:
> On Mon, 9 Jan 2023 20:18:39 -0700 Jens Axboe <axboe@kernel.dk> wrote:
>
> > On 1/9/23 7:47 AM, Yishai Hadas wrote:
> > > The last_pg is wrong, it is actually the first page of the last
> > > scatterlist element. To get the last page of the last scatterlist
> > > element we have to add prv->length. So it is checking mergability
> > > against the wrong page, Further, a SG element is not guaranteed to end
> > > on a page boundary, so we have to check the sub page location also for
> > > merge eligibility.
> > >
> > > Fix the above by checking physical contiguity, compute the actual last
> > > page and then call pages_are_mergable().
> >
> > Reviewed-by: Jens Axboe <axboe@kernel.dk>
> >
> > Andrew, can you pick this one up?
>
> This patch is against Yishai's
>
> CommitDate: Thu Jan 5 16:01:05 2023 -0400
>
> lib/scatterlist: Fix to merge contiguous pages into the last SG properly
>
>
> which is in -next, apparently via a tree of Jason's. So I'll assume
> that Jason will handleit?
It is in v6.2-rc3 already, but sure I will take this too
Jason
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-01-11 13:33 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-09 14:47 [PATCH] lib/scatterlist: Fix to calculate the last_pg properly Yishai Hadas
2023-01-09 18:08 ` Logan Gunthorpe
2023-01-10 3:18 ` Jens Axboe
2023-01-11 0:03 ` Andrew Morton
2023-01-11 13:28 ` Jason Gunthorpe
2023-01-10 5:08 ` Matthew Wilcox
2023-01-10 8:01 ` Yishai Hadas
2023-01-10 13:53 ` Jason Gunthorpe
2023-01-10 15:05 ` Yishai Hadas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).