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 -- To unsubscribe from this list: send the line "unsubscribe linux-crypto" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html