Re: [virtio-comment] Re: [PATCH v19] virtio-net: support inner header hash

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Jun 29, 2023 at 07:54:29AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 29, 2023 at 11:31:58AM +0800, Jason Wang wrote:
> > 
> > å 2023/6/28 18:10, Michael S. Tsirkin åé:
> > > On Wed, Jun 28, 2023 at 11:46:22AM +0800, Jason Wang wrote:
> > > > On Wed, Jun 28, 2023 at 12:35âAM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > > 1. Currently, a received encapsulated packet has an outer and an inner header, but
> > > > > the virtio device is unable to calculate the hash for the inner header. The same
> > > > > flow can traverse through different tunnels, resulting in the encapsulated
> > > > > packets being spread across multiple receive queues (refer to the figure below).
> > > > > However, in certain scenarios, we may need to direct these encapsulated packets of
> > > > > the same flow to a single receive queue. This facilitates the processing
> > > > > of the flow by the same CPU to improve performance (warm caches, less locking, etc.).
> > > > > 
> > > > >                 client1                    client2
> > > > >                    |        +-------+         |
> > > > >                    +------->|tunnels|<--------+
> > > > >                             +-------+
> > > > >                                |  |
> > > > >                                v  v
> > > > >                        +-----------------+
> > > > >                        | monitoring host |
> > > > >                        +-----------------+
> > > > > 
> > > > > To achieve this, the device can calculate a symmetric hash based on the inner headers
> > > > > of the same flow.
> > > > > 
> > > > > 2. For legacy systems, they may lack entropy fields which modern protocols have in
> > > > > the outer header, resulting in multiple flows with the same outer header but
> > > > > different inner headers being directed to the same receive queue. This results in
> > > > > poor receive performance.
> > > > > 
> > > > > To address this limitation, inner header hash can be used to enable the device to advertise
> > > > > the capability to calculate the hash for the inner packet, regaining better receive performance.
> > > > > 
> > > > > Fixes: https://github.com/oasis-tcs/virtio-spec/issues/173
> > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > Reviewed-by: Parav Pandit <parav@nvidia.com>
> > > > > ---
> > > > > v18->v19:
> > > > >          1. Have a single structure instead of two. @Michael S . Tsirkin
> > > > >          2. Some small rewrites. @Michael S . Tsirkin
> > > > >          3. Rebase to master.
> > > > > 
> > > > > v17->v18:
> > > > >          1. Some rewording suggestions from Michael (Thanks!).
> > > > >          2. Use 0 to disable inner header hash and remove
> > > > >             VIRTIO_NET_HASH_TUNNEL_TYPE_NONE.
> > > > > v16->v17:
> > > > >          1. Some small rewrites. @Parav Pandit
> > > > >          2. Add Parav's Reviewed-by tag (Thanks!).
> > > > > 
> > > > > v15->v16:
> > > > >          1. Remove the hash_option. In order to delimit the inner header hash and RSS
> > > > >             configuration, the ability to configure the outer src udp port hash is given
> > > > >             to RSS. This is orthogonal to inner header hash, which will be done in the
> > > > >             RSS capability extension topic (considered as an RSS extension together
> > > > >             with the symmetric toeplitz hash algorithm, etc.). @Parav Pandit @Michael S . Tsirkin
> > > > >          2. Fix a 'field' typo. @Parav Pandit
> > > > > 
> > > > > v14->v15:
> > > > >          1. Add tunnel hash option suggested by @Michael S . Tsirkin
> > > > >          2. Adjust some descriptions.
> > > > > 
> > > > > v13->v14:
> > > > >          1. Move supported_hash_tunnel_types from config space into cvq command. @Parav Pandit
> > > > I may miss some discussions, but this complicates the provisioning a lot.
> > > > 
> > > > Having it in the config space, then a type agnostic provisioning
> > > > through config space + feature bits just works fine.
> > > > 
> > > > If we move it only via cvq, we need device specific provisioning interface.
> > > > 
> > > > Thanks
> > > Yea that's what I said too. Debugging too.  I think we should build a
> > > consistent solution that allows accessing config space through DMA,
> > > separately from this effort.  Parav do you think you can live with this
> > > approach so this specific proposal can move forward?
> > 
> > 
> > We can probably go another way, invent a new device configuration space
> > capability which fixed size like PCI configuration access capability?
> > 
> > struct virtio_pci_cfg_cap {
> > ÂÂÂÂÂÂÂ struct virtio_pci_cap cap;
> > ÂÂÂÂÂÂÂ u8 dev_cfg_data[4]; /* Data for device configuration space access.
> > */
> > };
> > 
> > So it won't grow as the size of device configuration space grows.
> > 
> > Thanks
> 
> It is true, it does not have to be DMA strictly speaking.
> 
> The basic issue is with synchronous access.
> 
> We can change the capability in some way to allow asynch then
> that works. E.g. make device change length to 0 and driver
> must poll before considering the operation done.
> 
> Having said that, this will increase # of VM exits even more.
> Not a fan, DMA seems cleaner.

And again, I am now guilty of this too - hijacking inner hash thread
for config space change discussions :(
Let's all stop please, start a new thread wen we are ready.

-- 
MST