Re: [PATCH v2 1/4] scatterlist: Introduce some helper functions

[Robert Jarzmik]

Baolin Wang <baolin.wang@xxxxxxxxxx> writes:

> +/**
> + * sg_is_contiguous - Check if the scatterlists are contiguous
> + * @sga: SG entry
> + * @sgb: SG entry
> + *
> + * Description:
> + *   If the sga scatterlist is contiguous with the sgb scatterlist,
> + *   that means they can be merged together.
> + *
> + **/
> +static inline bool sg_is_contiguous(struct scatterlist *sga,
> +				    struct scatterlist *sgb)
> +{
> +	return *(unsigned long *)sg_virt(sga) + sga->length ==
> +		*(unsigned long *)sg_virt(sgb);
As I already said, I don't like casts.

But let's take some height : you're needing this function to decide to merge
scatterlists. That means that you think the probability of having 2 scatterlist
mergeable is not meaningless, ie. 50% or more.

I suppose your scatterlists are allocated through kmalloc(). I'd like to know,
through your testing, what is the success rate of sg_is_contiguous(), ie. I'd
like to know how many times sg_is_contiguous() was called, and amongst these
calls how many times it returned true.

That will tell me how "worth" is this optimization.

> + * 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 mapped scatterlists has been added
> + *   in the dynamic sg table. The 'orig_nents' member indicates the size of the
> + *   dynamic sg table.
> + *
> + *   Copy one mapped @src@ scatterlist into the dynamic sg table and increase
> + *   'nents' member.
> + *
> + **/
Okay, I still believe this one is wrong, because we don't understand each other.
So let's take an example :
sg_table = {
         .sgl = {
		{
                	.page_link = PAGE_48,
                        .offset = 0,
                        .length = 2048,
                        .dma_address = 0x30000,
                        .dma_length = 4096,
                },
		{
                	.page_link = PAGE_48 | 0x02,
                        .offset = 2048,
                        .length = 2048,
                        .dma_address = 0,
                        .dma_length = 0,
                },
	},
         .nents = 1,
         .orig_nents = 2,
};

In this example, by sheer luck the 2 scatterlist entries were physically
contiguous, and the mapping function coallesced the 2 entries into only one
(dma_address, dma_length) entry. That could also happen with an IOMMU by the
way.  Therefore, sg_table.nents = 1.

If I understand your function correctly, it will erase sg_table.sgl[1], and that
looks incorrect to me. This is why I can't understand how your code can be
correct, and why I say you add a new "meaning" to sg_table->nents, which is not
consistent with the meaning I understand.

Cheers.

-- 
Robert