On Thu, Feb 13, 2025 at 06:23:55PM +0900, Akihiko Odaki wrote: > On 2025/02/13 16:18, Michael S. Tsirkin wrote: > > > > Commit log needs some work. > > > > So my understanding is, this patch does not do much functionally, > > but makes adding the hash feature easier. OK. > > > > On Thu, Feb 13, 2025 at 03:54:06PM +0900, Akihiko Odaki wrote: > > > tun used to simply advance iov_iter when it needs to pad virtio header, > > > which leaves the garbage in the buffer as is. This is especially > > > problematic > > > > I think you mean "this will become especially problematic" > > > > > when tun starts to allow enabling the hash reporting > > > feature; even if the feature is enabled, the packet may lack a hash > > > value and may contain a hole in the virtio header because the packet > > > arrived before the feature gets enabled or does not contain the > > > header fields to be hashed. If the hole is not filled with zero, it is > > > impossible to tell if the packet lacks a hash value. > > > > > > In theory, a user of tun can fill the buffer with zero before calling > > > read() to avoid such a problem, but leaving the garbage in the buffer is > > > awkward anyway so fill the buffer in tun. > > > > > > What is missing here is description of what the patch does. > > I think it is > > "Replace advancing the iterator with writing zeros". > > > > There could be performance cost to the dirtying extra cache lines, though. > > Could you try checking that please? > > It will not dirty extra cache lines; an explanation follows later. Because > of that, any benchmark are likely to show only noises, but if you have an > idea of workloads that should be tested, please tell me. pktgen usually > > > > I think we should mention the risks of the patch, too. > > Maybe: > > > > Also in theory, a user might have initialized the buffer > > to some non-zero value, expecting tun to skip writing it. > > As this was never a documented feature, this seems unlikely. > > > > > > > > > The specification also says the device MUST set num_buffers to 1 when > > > the field is present so set it when the specified header size is big > > > enough to contain the field. > > > > This part I dislike. tun has no idea what the number of buffers is. > > Why 1 specifically? > > That's a valid point. I rewrote the commit log to clarify, but perhaps we > can drop the code to set the num_buffers as "[PATCH] vhost/net: Set > num_buffers for virtio 1.0" already landed. I think I'd prefer that second option. it allows userspace to reliably detect the new behaviour, by setting the value to != 0. > > Below is the rewritten commit log, which incorporates your suggestions and > is extended to cover the performance implication and reason the num_buffers > initialization: > > tun simply advances iov_iter when it needs to pad virtio header, > which leaves the garbage in the buffer as is. This will become > especially problematic when tun starts to allow enabling the hash > reporting feature; even if the feature is enabled, the packet may lack a > hash value and may contain a hole in the virtio header because the > packet arrived before the feature gets enabled or does not contain the > header fields to be hashed. If the hole is not filled with zero, it is > impossible to tell if the packet lacks a hash value. > > In theory, a user of tun can fill the buffer with zero before calling > read() to avoid such a problem, but leaving the garbage in the buffer is > awkward anyway so replace advancing the iterator with writing zeros. > > A user might have initialized the buffer to some non-zero value, > expecting tun to skip writing it. As this was never a documented > feature, this seems unlikely. Neither is there a non-zero value that can > be determined and set before receiving the packet; the only exception > is the num_buffers field, which is expected to be 1 for version 1 when > VIRTIO_NET_F_HASH_REPORT is not negotiated. you need mergeable buffers instead i presume. > This field is specifically > set to 1 instead of 0. > > The overhead of filling the hole in the header is negligible as the > entire header is already placed on the cache when a header size defined what does this mean? > in the current specification is used even if the cache line is small > (16 bytes for example). > > Below are the header sizes possible with the current specification: > a) 10 bytes if the legacy interface is used > b) 12 bytes if the modern interface is used > c) 20 bytes if VIRTIO_NET_F_HASH_REPORT is negotiated > > a) and b) obviously fit in a cache line. c) uses one extra cache line, > but the cache line also contains the first 12 bytes of the packet so > it is always placed on the cache. Hmm. But it could be clean so shared. write makes it dirty and so not shared. -- MST
