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

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

On Thu, Jun 01, 2023 at 01:17:08PM +0800, Heng Qi wrote:
> On Thu, Jun 01, 2023 at 12:30:30AM -0400, Michael S. Tsirkin wrote:
> > On Tue, May 30, 2023 at 03:40:18PM -0400, Michael S. Tsirkin wrote:
> > > On Fri, May 26, 2023 at 04:04:18PM +0800, Heng Qi wrote:
> > > > 
> > > > 
> > > > å 2023/5/23 äå11:58, Heng Qi åé:
> > > > > On Mon, May 22, 2023 at 03:19:16PM -0400, Michael S. Tsirkin wrote:
> > > > > > On Mon, May 22, 2023 at 01:02:36PM +0800, Heng Qi 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.
> > > > > > > 
> > > > > > > Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> > > > > > > Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > > ---
> > > > > > > v13->v14:
> > > > > > > 	1. Move supported_hash_tunnel_types from config space into cvq command. @Parav Pandit
> > > > > > > 	2. Rebase to master branch.
> > > > > > > 	3. Some minor modifications.
> > > > > > So, I proposed adding a "generic UDP tunnel" option which simply uses UDP source
> > > > > > port for hash. I think it will help us not having to chaise future tunnels as
> > > > > > more and more are added.
> > > > > I agree, but I thought we'd do this in another thread, sorry.
> > > > > Following your suggestion, we should add a field similar to
> > > > > \field{generic_udp_tunnel_option} in the virtnet_hash_tunnel_config_set
> > > > > structure.
> > > > > 
> > > > > \field{generic_udp_tunnel_option} should be 0, 1 or 2.
> > > > > 
> > > > > \field{hash_tunnel_types} is still useful, but for more general purpose we need
> > > > > to use it together with \field{generic_udp_tunnel_option}.
> > > > > 
> > > > > When \field{generic_udp_tunnel_option} is 0, all tunneling protocols included in
> > > > > \field{hash_tunnel_types} use the inner header for hashing. For other tunnel
> > > > > protocols not included in \field{hash_tunnel_types}, the hash is calculated as if
> > > > > VIRTIO_NET_F_TUNNEL_HASH is not negotiated.
> > > > > 
> > > > > When \field{generic_udp_tunnel_option} is 1, all tunneling protocols included in
> > > > > \field{hash_tunnel_types} use the inner header for hashing. For other tunnel
> > > > > protocols not included in \field{hash_tunnel_types}, if their outer headers are
> > > > > based on UDP protocol, the device use the outer UDP source port for hashing.
> > > > > For the rest of the tunnel protocols, the hash is calculated as if VIRTIO_NET_F_TUNNEL_HASH
> > > > > was not negotiated.
> > > > > 
> > > > > When \field{generic_udp_tunnel_option} is 2, for all UDP tunneling protocols,
> > > > > the outer udp source port is used for hashing, otherwise if the tunneling protocol
> > > > > is included in \field{hash_tunnel_types}, the inner header is used for hashing.
> > > > > For the rest of the tunnel protocols, the hash is calculated as if VIRTIO_NET_F_TUNNEL_HASH
> > > > > was not negotiated.
> > > > > 
> > > > > And for this option, we need to add a reminder:
> > > > > Although the \field{generic_udp_tunnel_option} helps us adapt to more new
> > > > > tunneling protocols, it is still an unreliable option, especially for
> > > > > tunneling protocols that use "SHOULD" "Recommended" in their own
> > > > > specifications, because it means the udp source port does not
> > > > > always fully identify a stream.
> > > > > 
> > > > 
> > > > Hi, Michael.
> > > > 
> > > > Do you agree with this plan? Please let me know if you have any comments.:)
> > > > 
> > > > If there are no comments, I can start a new version to make progress.
> > > > 
> > > > Thanks.
> > > 
> > > How are "tunneling protocols" defined though?
> > > 
> > > Maybe pass a mask of destination UDP ports for which this applies?
> > > 
> > > Then we don't need options, if port is set in mask then
> > > generic udp tunnel inner hash applies. If port is not set then
> > > hash is calculated in some other way, including
> > > one of tunnel specific flags.
> > 
> > I admit this is pretty complex though. As an intermediate step
> > I can see two other options:
> > - just do this for all UDP packets assuming most traffic is encapsulated
> 
> This is a bit crude, but it does simplify the complexity of device
> implementations.
> 
> > - assume that the list of protocols is configured in the NIC
> >   by other means (e.g. hard-coded, or we can add an admin command for this)
> 
> Other means also means hardcoding, because we always need to know what
> the "new tunnel type" is, otherwise the driver can't understand it.
> Assuming we don't use any hard-coded tunnel types for the
> VIRTIO_NET_F_TUNNEL_HASH feature, then we use the GET command to get the
> tunnel types supported by the device before we intend to use the
> inner header hashing capabilities. But if we don't do mappings for
> these codes, the driver can't understand what the device returns. Then
> we need to hardcode it...

Yes, if we have a GET command that will need a bitmap anyway, so
let's just set a bitmap with a command.

> 
> So I tend to dst port mask, or generic option (set to 1 to use source
> port for all UDP packets).
> Thanks.

Makes sense, and I don't think we need both options.

> > 
> > Thoghts?
> > 
> > 
> > > 
> > > > > > I also suggested dropping some tunnels which are less common and where
> > > > > > the specification is unambiguous enough that source port should include
> > > > > > inner hash.
> > > > > OK, I'll re-screen and update the tunneling protocols we already include
> > > > > (e.g. remove STT since it fits what you said).
> > > > > 
> > > > > Thanks.
> > > > > 
> > > > > ---------------------------------------------------------------------
> > > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org