Re: [RFC v3 5/8] virtio: Implement the pasta vhost functions

["Ammar Yasser" <aerosound161@gmail.com>]

>> >> +	/* general purpose buffers */
>> >> +    vhost_memory.mem.regions[0] = VHOST_MEMORY_REGION(pkt_buf);
>> >> +	vhost_memory.mem.regions[1] = VHOST_MEMORY_REGION(eth_pad);
>> >> +
>> >> +	/* tcp specific buffers */
>> >> +   	vhost_memory.mem.regions[2] = VHOST_MEMORY_REGION(tcp_payload_tap_hdr);
>> >> +	vhost_memory.mem.regions[3] = VHOST_MEMORY_REGION(tcp4_payload_ip);
>> >> +	vhost_memory.mem.regions[4] = VHOST_MEMORY_REGION(tcp6_payload_ip);
>> >> +	vhost_memory.mem.regions[5] = VHOST_MEMORY_REGION(tcp_payload);
>> >> +	vhost_memory.mem.regions[6] = VHOST_MEMORY_REGION(tcp_eth_hdr);
>> >> +
>> >> +	/* udp specific buffers */
>> >> +	vhost_memory.mem.regions[7] = VHOST_MEMORY_REGION(udp_payload);
>> >> +	vhost_memory.mem.regions[8] = VHOST_MEMORY_REGION(udp_eth_hdr);
>> >> +	vhost_memory.mem.regions[9] = VHOST_MEMORY_REGION(udp_iov_recv);
>> >> +	vhost_memory.mem.regions[10] = VHOST_MEMORY_REGION(udp_mh_recv);
>> >> +	vhost_memory.mem.regions[11] = VHOST_MEMORY_REGION(udp_meta);
>> >
>> > Not sure if there would be value in delegating these to helpers in
>> > tcp.c and udp.c
>> 
>> You mean every protocol file registers its own memory regions through
>> calling the macro and taking as input the vhost_memory struct ?
>
> More or less, yes.  Specifically I was thinking that this function
> would call, say, tcp_vhost_regions() etc.
>
>> It will be better in the sense that it will remove the need for making
>> those buffers public. But i think it will be harder to follow from a
>> readability perspective. WDYT ?
>
> It's a trade off.  It's a bit harder to collate the complete set of
> memory regions.  However it avoids making the buffers public and makes
> it clearer when looking at the TCP code that part of "its" memory is
> shared via vhost.  It also makes it a little easier to keep the shared
> regions in sync if we change the data structures that (say) TCP uses
> internally.

The last point is pretty noteworthy. Yeah ok.. will go with functions
per protocol that register the dedicated regions

>
>> >> + * rx_descriptor_handoff() - Batch-announce freed RX descriptors to the kernel
>> >> + * @c:		Execution context
>> >> + *
>> >> + * Bumps avail.idx by the number of descriptors accumulated in
>> >> + * vqs[0].num_free (from prior consume_one_rx_descriptor() calls),
>> >> + * then resets the counter to zero. The kernel will see the new
>> >> + * avail.idx and consume the freshly-available descriptors.
>> >> + *
>> >> + */
>> >> +void rx_descriptor_handoff(struct ctx *c)
>> >
>> > Can this be static?  That's the sort of thing that's harder to review
>> > when signatures are split from implementations.  If not, it should
>> > have a properly prefixed name.
>> 
>> If by static you mean it gets defined in only one place and not exported
>> then its used in two places (virtio.c and tap.c). Unless you want a
>> duplicate definition which i personally am not in favor of.
>
> Right, again, because the uses in different places are split across
> patches, that makes it harder to see that it's used in multiple places.
>
>> Can you please clarify what you mean by a prefixed name ? Not sure i
>> understand what a proper prefix to a function like this would be
>
> Usually (though not always) functions exported from one module to be
> used in others have a name that starts with the module name.  So in
> this case it would be vhost_something_something().

The prefixed name sounds like a good idea. I think it may be difficult
to restructure the series so that both uses of the function will be in
one patch without this patch becoming super blocky. Going with the
prefixed name solution