Re: [PATCH rdma-next v2 1/2] lib/scatterlist: Fix wrong update of orig_nents
From: Jason Gunthorpe <jgg@nvidia.com>
Date: 2021-07-22 13:00:50
Also in:
dri-devel, intel-gfx, lkml
On Sun, Jul 18, 2021 at 02:09:12PM +0300, Leon Romanovsky wrote:
quoted hunk ↗ jump to hunk
@@ -386,12 +414,14 @@ static struct scatterlist *get_next_sg(struct sg_table *table, return ERR_PTR(-ENOMEM); sg_init_table(new_sg, alloc_size); if (cur) { + if (total_nents) + *total_nents += alloc_size - 1; __sg_chain(next_sg, new_sg); - table->orig_nents += alloc_size - 1; } else { table->sgl = new_sg; - table->orig_nents = alloc_size; table->nents = 0;
Why does this still touch nents?
quoted hunk ↗ jump to hunk
@@ -515,6 +548,7 @@ struct scatterlist *__sg_alloc_table_from_pages(struct sg_table *sgt, cur_page = j; } sgt->nents += added_nents; + sgt->orig_nents = sgt->nents;
And here too?
nents should only be set by the dma mapper, right?
I'm also trying to understand why it is OK to pass in NULL for
total_nents?
Any situation where _sg_alloc_table_from_pages() returns with
sgt->orig_nents != total_nents requires the use of
sg_free_table_entries()
It looks like there is some trouble here:
for (i = 0; i < chunks; i++) {
s = get_next_sg(sgt, s, chunks - i + left_pages, gfp_mask,
total_nents);
if (IS_ERR(s)) {
This will update total_nents but after a few loops it can exit without
synchronizing sgt->orig_nents - thus any caller handling an error
return from __sg_alloc_table_from_pages() must not pass in NULL and
must use sg_free_table_entries()
So I would see two options:
1) Remove the possiblity to return NULL and fix all callers to use
sg_free_table_entries() on error
2) Once __sg_alloc_table_from_pages() fails the sg_table is corrupted
and the user must call sg_free_table_entries().
ie forcibly store total_nents in the orig_nents and thus destroy
the ability to continue to use the sg_table.
This is what sg_alloc_table_from_pages() already has to do
Further upon success of __sg_alloc_table_from_pages() it should be
true that sgt->orig_nents == total_nents so the ib_umem change is
confusing. total_nents should be removed from the struct and only the
failure paths in the function calling __sg_alloc_table_from_pages()
need a stack local variable and sg_free_table_entries()
IMHO this API may have become unwieldly and complicated, I wonder if
this is better:
struct sg_append_table state;
sg_append_init(&state, sgt, gfp_mask);
while (..)
ret = sg_append_pages(&state, pages, n_pages, ..)
if (ret)
sg_append_abort(&state); // Frees the sgt and puts it to NULL
sg_append_complete(&state)
Which allows sg_alloc_table_from_pages() to be written as
struct sg_append_table state;
sg_append_init(&state, sgt, gfp_mask);
ret = sg_append_pages(&state,pages, n_pages, offset, size, UINT_MAX)
if (ret) {
sg_append_abort(&state);
return ret;
}
sg_append_complete(&state);
return 0;
And then the API can manage all of this in some sane and
understandable way.
Jason