Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
> Hi Robert,
>
> On 4 March 2016 at 03:15, Robert Jarzmik <robert.jarzmik@xxxxxxx> wrote:
>> Baolin Wang <baolin.wang@xxxxxxxxxx> writes:
>>> +static inline bool sg_is_contiguous(struct scatterlist *sga,
>>> + struct scatterlist *sgb)
>>> +{
>>> + return ((sga->page_link & ~0x3UL) + sga->offset + sga->length ==
>>> + (sgb->page_link & ~0x3UL));
>>> +}
>> I don't understand that one.
>> sga->page_link is a pointer to a "struct page *". How can it be added to an
>> offset within a page ???
>
>
> Ah, sorry that's a mistake. It should check as below:
> static inline bool sg_is_contiguous(struct scatterlist *sga, struct
> scatterlist *sgb)
> {
> return (unsigned int)sg_virt(sga) + sga->length == (unsigned
> int)sg_virt(sgb);
> }
NAK.
First, I don't like the cast.
Second ask yourself: what if you compile on a 64 bit architecture ?
Third, I don't like void* arithmetics, some compilers might refuse it.
And as a last comment, is it "virtual" contiguity you're looking after, physical
contiguity, or dma_addr_t contiguity ?
>>> @@ -370,6 +370,65 @@ int sg_alloc_table(struct sg_table *table, unsigned int
>>> nents, gfp_t gfp_mask)
>> ...
>>> /**
>>> + * sg_add_sg_to_table - Add one scatterlist into sg table
>>> + * @sgt: The sg table header to use
>>> + * @src: The sg need to be added into sg table
>>> + *
>>> + * Description:
>>> + * The 'nents' member indicates how many scatterlists added in the sg table.
>>> + * Copy the @src@ scatterlist into sg table and increase 'nents' member.
>>> + *
>>> + **/
>>> +int sg_add_sg_to_table(struct sg_table *sgt, struct scatterlist *src)
>>> +{
>>> + unsigned int i = 0, orig_nents = sgt->orig_nents;
>>> + struct scatterlist *sgl = sgt->sgl;
>>> + struct scatterlist *sg;
>>> +
>>> + /* Check if there are enough space for the new sg to be added */
>>> + if (sgt->nents >= sgt->orig_nents)
>>> + return -EINVAL;
>> I must admit I don't understand that one either : how do comparing the number of
>> "mapped" entries against the number of "allocated" entries determines if there
>> is enough room ?
>
> That's for a dynamic sg table. If there is one sg table allocated
> 'orig_nents' scatterlists, and we need copy another mapped scatterlist
> into the sg table if there are some requirements. So we use 'nents' to
> record how many scatterlists have been copied into the sg table.
As I'm still having difficulities to understand, please explain to me :
- what is a dynamic sg_table
- how it works
- if it's "struct sg_table", how the field of this structure are different when
it's a dynamic sg_table from a "static sg_table" ?
>>> +/**
>>> + * sg_alloc_empty_table - Allocate one empty sg table
>>> + * @sgt: The sg table header to use
>>> + * @nents: Number of entries in sg list
>>> + * @gfp_mask: GFP allocation mask
>>> + *
>>> + * Description:
>>> + * Allocate and initialize an sg table. The 'nents' member of sg_table
>>> + * indicates how many scatterlists added in the sg table. It should set
>>> + * 0 which means there are no scatterlists added in this sg table now.
>>> + *
>>> + **/
>>> +int sg_alloc_empty_table(struct sg_table *sgt, unsigned int nents,
>>> + gfp_t gfp_mask)
>> As for this one, there has to be a purpose for it I fail to see. From far away
>> it looks exactly like sg_alloc_table(), excepting it "works around" the nents >
>> 0 protection of __sg_alloc_table().
>> What is exactly the need for this one, and if it's usefull why not simply
>> changing the __sg_alloc_table() "nents > 0" test and see what the outcome of the
>> review will be ?
>
> Like I said above. If we want to copy some mapped scatterlists into
> one sg table, we should set the 'nents' to 0 to indicates how many
> scatterlists coppied in the sg table.
So how do you unmap this scatterlist once you've lost the number of mapped
entries ?
I think we'll come back to this once I understand how a "dynamic" sg_table is
different from a "static" one.
Cheers.
--
Robert
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
