Re: [PATCH v5 net-next 02/36] iov_iter: DDP copy to iter/pages

[Boris Pismenny <borispismenny@gmail.com>]

On 22/07/2021 16:31, Christoph Hellwig wrote:
>> +#ifdef CONFIG_ULP_DDP
>> +size_t _ddp_copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i);
>> +#endif
>>  size_t _copy_from_iter(void *addr, size_t bytes, struct iov_iter *i);
>>  bool _copy_from_iter_full(void *addr, size_t bytes, struct iov_iter *i);
>>  size_t _copy_from_iter_nocache(void *addr, size_t bytes, struct iov_iter *i);
>> @@ -145,6 +148,16 @@ size_t copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i)
>>  		return _copy_to_iter(addr, bytes, i);
>>  }
>>  
>> +#ifdef CONFIG_ULP_DDP
>> +static __always_inline __must_check
>> +size_t ddp_copy_to_iter(const void *addr, size_t bytes, struct iov_iter *i)
>> +{
>> +	if (unlikely(!check_copy_size(addr, bytes, true)))
>> +		return 0;
>> +	return _ddp_copy_to_iter(addr, bytes, i);
>> +}
>> +#endif
> 
> There is no need to ifdef out externs with conditional implementations,
> or inlines using them.
> 
>> +#ifdef CONFIG_ULP_DDP
>> +static void ddp_memcpy_to_page(struct page *page, size_t offset, const char *from, size_t len)
> 
> Overly long line.
> 
>> +	char *to = kmap_atomic(page);
>> +
>> +	if (to + offset != from)
>> +		memcpy(to + offset, from, len);
>> +
>> +	kunmap_atomic(to);
> 
> This looks completely bogus to any casual read, so please document why
> it makes sense.  And no, a magic, unexplained ddp in the name does not
> count as explanation at all.  Please think about a more useful name.

This routine, like other changes in this file, replicates the logic in
memcpy_to_page. The only difference is that "ddp" avoids copies when the
copy source and destinations buffers are one and the same. These are
then used by nvme-tcp (see skb_ddp_copy_datagram_iter in nvme-tcp) which
receives SKBs from the NIC that already placed data in its destination,
and this is the source for the name Direct Data Placement. I'd gladly
take suggestions for better names, but this is the best we came up with
so far.

The reason we are doing it is to avoid modifying memcpy_to_page itself,
but rather allow users (e.g., nvme-tcp) to access this functionality
directly.

> 
> Can this ever write to user page?  If yes it needs a flush_dcache_page.

Yes, will add.

> 
> Last but not least: kmap_atomic is deprecated except for the very
> rate use case where it is actually called from atomic context.  Please
> use kmap_local_page instead.
> 

Will look into it, thanks!

>> +#ifdef CONFIG_CRYPTO_HASH
>> +	struct ahash_request *hash = hashp;
>> +	struct scatterlist sg;
>> +	size_t copied;
>> +
>> +	copied = ddp_copy_to_iter(addr, bytes, i);
>> +	sg_init_one(&sg, addr, copied);
>> +	ahash_request_set_crypt(hash, &sg, NULL, copied);
>> +	crypto_ahash_update(hash);
>> +	return copied;
>> +#else
>> +	return 0;
>> +#endif
> 
> What is the point of this stub?  To me it looks extremely dangerous.
> 

As above, we use the same logic as in hash_and_copy_to_iter. The purpose
is again to eventually avoid the copy in case the source and destination
buffers are one and the same.