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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 02 Apr 2016 17:00:58 +0200
From:	Robert Jarzmik <robert.jarzmik@...e.fr>
To:	Baolin Wang <baolin.wang@...aro.org>
Cc:	herbert@...dor.apana.org.au, davem@...emloft.net, agk@...hat.com,
	snitzer@...hat.com, axboe@...com, dm-devel@...hat.com,
	akpm@...ux-foundation.org, david.s.gordon@...el.com,
	thomas.lendacky@....com, yamada.masahiro@...ionext.com,
	smueller@...onox.de, tadeusz.struk@...el.com,
	standby24x7@...il.com, shli@...nel.org, broonie@...nel.org,
	linus.walleij@...aro.org, arnd@...db.de,
	linux-kernel@...r.kernel.org, linux-crypto@...r.kernel.org,
	linux-raid@...r.kernel.org
Subject: Re: [PATCH v2 1/4] scatterlist: Introduce some helper functions

Baolin Wang <baolin.wang@...aro.org> 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