[virtio-dev] Re: [virtio-comment] Re: [PATCH v13] virtio-net: support inner header hash
On Tue, Apr 25, 2023 at 04:28:33PM -0400, Parav Pandit wrote:
>
>
> On 4/23/2023 3:35 AM, Heng Qi wrote:
> > \subsubsection{Legacy Interface: Feature bits}\label{sec:Device Types /
> > Network Device / Feature bits / Legacy Interface: Feature bits}
> > @@ -198,6 +202,7 @@ \subsection{Device configuration
> > layout}\label{sec:Device Types / Network Device
> > u8 rss_max_key_size;
> > le16 rss_max_indirection_table_length;
> > le32 supported_hash_types;
> > +le32 supported_tunnel_hash_types;
this needs a comment explaining it only exists with some
feature bits.
> > };
> In v12 I was asking this to move to above field from the config area to the
> GET command in comment [1] as,
>
> "With that no need to define two fields at two different places in config
> area and also in cvq."
I think I disagree.
the proposed design is consistent with regular tunneling.
however I feel we should limit this to 1-2 legacy protocols.
with that, we do not really need a new field at all,
they can fit in supported_hash_types.
> I am sorry if that was not clear enough.
>
> [1]
> https://lore.kernel.org/virtio-dev/569cbaf9-f1fb-0e1f-a2ef-b1d7cd7db...@nvidia.com/
>
> > \subparagraph{Supported/enabled hash types}
> > \label{sec:Device Types / Network Device / Device Operation / Processing
> > of Incoming Packets / Hash calculation for incoming packets /
> > Supported/enabled hash types}
> > +This paragraph relies on definitions from \hyperref[intro:IP]{[IP]},
> > \hyperref[intro:UDP]{[UDP]} and \hyperref[intro:TCP]{[TCP]}.
> > Hash types applicable for IPv4 packets:
> > \begin{lstlisting}
> > #define VIRTIO_NET_HASH_TYPE_IPv4 (1 << 0)
> > @@ -980,6 +993,152 @@ \subsubsection{Processing of Incoming
> > Packets}\label{sec:Device Types / Network
> > (see \ref{sec:Device Types / Network Device / Device Operation /
> > Processing of Incoming Packets / Hash calculation for incoming packets /
> > IPv6 packets without extension header}).
> > \end{itemize}
> > +\paragraph{Inner Header Hash}
> > +\label{sec:Device Types / Network Device / Device Operation / Processing
> > of Incoming Packets / Inner Header Hash}
> > +
> > +If VIRTIO_NET_F_HASH_TUNNEL has been negotiated, the device supports inner
> > header hash and the driver can send
> > +commands VIRTIO_NET_CTRL_TUNNEL_HASH_SET and
> > VIRTIO_NET_CTRL_TUNNEL_HASH_GET for the inner header hash configuration.
> > +
> > +struct virtio_net_hash_tunnel_config {
> Please move field from the config struct to here. Both are RO fields.
>
> le32 supported_hash_tunnel_types;
> > +le32 hash_tunnel_types;
> > +};
> > +
> > +#define VIRTIO_NET_CTRL_TUNNEL_HASH 7
> > + #define VIRTIO_NET_CTRL_TUNNEL_HASH_SET 0
> > + #define VIRTIO_NET_CTRL_TUNNEL_HASH_GET 1
> > +
> > +Filed \field{hash_tunnel_types} contains a bitmask of configured hash
> > tunnel types as
> > +defined in \ref{sec:Device Types / Network Device / Device Operation /
> > Processing of Incoming Packets / Hash calculation for incoming packets /
> > Supported/enabled hash tunnel types}.
> > +
> > +The class VIRTIO_NET_CTRL_TUNNEL_HASH has the following commands:
> > +\begin{itemize}
> > +\item VIRTIO_NET_CTRL_TUNNEL_HASH_SET: set the \field{hash_tunnel_types}
> > to configure the inner header hash calculation for the device.
> > +\item VIRTIO_NET_CTRL_TUNNEL_HASH_GET: get the \field{hash_tunnel_types}
> > from the device.
> > +\end{itemize}
> > +
> > +For the command VIRTIO_NET_CTRL_TUNNEL_HASH_SET, the structure
> > virtio_net_hash_tunnel_config is write-only for the driver.
> > +For the command VIRTIO_NET_CTRL_TUNNEL_HASH_GET, the structure
> > virtio_net_hash_tunnel_config is read-only for the driver.
> > +
> You need to split the structures to two, one for get and one for set in
> above description as get and set contains different fields.
> > +
> > +If VIRTIO_NET_HASH_TUNNEL_TYPE_NONE is set or the encapsulation type is
> > not included in \field{hash_tunnel_types},
> > +the hash of the outer header is calculated for the received encapsulated
> > packet.
> > +
> > +
> > +For scenarios with sufficient external entropy or no internal hashing
> > requirements, inner header hash may not be needed:
> > +A tunnel is often expected to isolate the external network from the
> > internal one. By completely ignoring entropy
> > +in the external header and replacing it with entropy from the internal
> > header, for hash calculations, this expectation
> You wanted to say inner here like rest of the places.
>
> s/internal header/inner header
>
> > +The driver MUST NOT set any VIRTIO_NET_HASH_TUNNEL_TYPE_ flags that are
> > not supported by the device.
> Multiple flags so,
>
> s/flags that are/flags which are/
>
> This publicly archived list offers a means to provide input to the
> OASIS Virtual I/O Device (VIRTIO) TC.
>
> In order to verify user consent to the Feedback License terms and
> to minimize spam in the list archive, subscription
[virtio-dev] RE: [virtio-comment] Re: [PATCH v13] virtio-net: support inner header hash
> From: Michael S. Tsirkin
> Sent: Tuesday, April 25, 2023 5:06 PM
> > On 4/23/2023 3:35 AM, Heng Qi wrote:
> > > \subsubsection{Legacy Interface: Feature bits}\label{sec:Device
> > > Types / Network Device / Feature bits / Legacy Interface: Feature bits} @@
> -198,6 +202,7 @@ \subsection{Device configuration layout}\label{sec:Device
> Types / Network Device
> > > u8 rss_max_key_size;
> > > le16 rss_max_indirection_table_length;
> > > le32 supported_hash_types;
> > > +le32 supported_tunnel_hash_types;
>
> this needs a comment explaining it only exists with some feature bits.
>
Yes, it is already there.
+Field \field{supported_tunnel_hash_types} only exists if the device supports
inner header hash, i.e. if VIRTIO_NET_F_HASH_TUNNEL is set.
+
I think it should be changed from "device supports" to "driver negotiated".
> > > };
> > In v12 I was asking this to move to above field from the config area
> > to the GET command in comment [1] as,
> >
> > "With that no need to define two fields at two different places in
> > config area and also in cvq."
>
> I think I disagree.
> the proposed design is consistent with regular tunneling.
>
Sure.
I understand how config space has evolved from 0.9.5 to know without much
attention, but really expanding this way is not helpful.
It requires building more and more RAM based devices even for PCI PFs, which is
sub optimal.
CVQ already exists and provides the SET command. There is no reason to do GET
in some other way.
Device has single channel to provide a value and hence it doesn't need any
internal synchronization between two paths.
So, if we add a new feature bit as VIRTIO_F_CFG_SPACE_OVER_AQ it is an
improvement.
But it still comes with a cost which device cannot mitigate.
The cost is,
1. a driver may not negotiate such feature bit, and device ends up burning
memory.
1.b Device provisioning becomes a factor of what some guests may use/not
use/already using on the live system.
2. Every device needs AQ even when the CVQ exists.
Hence, better to avoid expanding current structure for any new fields,
specially which has the SET equivalent.
But if we want to with the path of VIRTIO_F_CFG_SPACE_OVER_AQ, it is fine.
More things can evolve for generic things like config space over AQ.
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] Re: [PATCH v13] virtio-net: support inner header hash
On Tue, Apr 25, 2023 at 09:39:28PM +, Parav Pandit wrote:
>
> > From: Michael S. Tsirkin
> > Sent: Tuesday, April 25, 2023 5:06 PM
> > > On 4/23/2023 3:35 AM, Heng Qi wrote:
> > > > \subsubsection{Legacy Interface: Feature bits}\label{sec:Device
> > > > Types / Network Device / Feature bits / Legacy Interface: Feature bits}
> > > > @@
> > -198,6 +202,7 @@ \subsection{Device configuration layout}\label{sec:Device
> > Types / Network Device
> > > > u8 rss_max_key_size;
> > > > le16 rss_max_indirection_table_length;
> > > > le32 supported_hash_types;
> > > > +le32 supported_tunnel_hash_types;
> >
> > this needs a comment explaining it only exists with some feature bits.
> >
> Yes, it is already there.
> +Field \field{supported_tunnel_hash_types} only exists if the device supports
> inner header hash, i.e. if VIRTIO_NET_F_HASH_TUNNEL is set.
> +
> I think it should be changed from "device supports" to "driver negotiated".
>
> > > > };
> > > In v12 I was asking this to move to above field from the config area
> > > to the GET command in comment [1] as,
> > >
> > > "With that no need to define two fields at two different places in
> > > config area and also in cvq."
> >
> > I think I disagree.
> > the proposed design is consistent with regular tunneling.
> >
> Sure.
> I understand how config space has evolved from 0.9.5 to know without much
> attention, but really expanding this way is not helpful.
> It requires building more and more RAM based devices even for PCI PFs, which
> is sub optimal.
No, this is ROM, not RAM.
> CVQ already exists and provides the SET command. There is no reason to do GET
> in some other way.
Spoken looking at just hardware cost :)
The reason is that this is device specific. Maintainance overhead and
system RAM cost for the code to support this should not be ignored.
> Device has single channel to provide a value and hence it doesn't need any
> internal synchronization between two paths.
>
> So, if we add a new feature bit as VIRTIO_F_CFG_SPACE_OVER_AQ it is an
> improvement.
> But it still comes with a cost which device cannot mitigate.
> The cost is,
> 1. a driver may not negotiate such feature bit, and device ends up burning
> memory.
> 1.b Device provisioning becomes a factor of what some guests may use/not
> use/already using on the live system.
>
> 2. Every device needs AQ even when the CVQ exists.
>
> Hence, better to avoid expanding current structure for any new fields,
> specially which has the SET equivalent.
>
> But if we want to with the path of VIRTIO_F_CFG_SPACE_OVER_AQ, it is fine.
> More things can evolve for generic things like config space over AQ.
I am not sure what does VIRTIO_F_CFG_SPACE_OVER_AQ mean, and what are
these costs. What I had in mind is extending proposed vq transport to
support sriov. I don't see why we can not have exactly 0 bytes of memory
per VF.
And if we care about single bytes of PF memory (do we? there's only one
PF per SRIOV device ...), what we should do is a variant of pci
transport that aggressively saves memory.
--
MST
-
To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org
For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] Re: [PATCH v13] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, April 26, 2023 12:12 AM > > > No, this is ROM, not RAM. > For VFs it is not ROM because one might configure VF with different feature bits. > > CVQ already exists and provides the SET command. There is no reason to do > GET in some other way. > > Spoken looking at just hardware cost :) Not really. The CVQ is already there, no extra overheads. > The reason is that this is device specific. Maintainance overhead and system > RAM cost for the code to support this should not be ignored. Sure. As above its not a new and such query occurs only once or extremely rare. > > > Device has single channel to provide a value and hence it doesn't need any > internal synchronization between two paths. > > > > So, if we add a new feature bit as VIRTIO_F_CFG_SPACE_OVER_AQ it is an > improvement. > > But it still comes with a cost which device cannot mitigate. > > The cost is, > > 1. a driver may not negotiate such feature bit, and device ends up burning > memory. > > 1.b Device provisioning becomes a factor of what some guests may use/not > use/already using on the live system. > > > > 2. Every device needs AQ even when the CVQ exists. > > > > Hence, better to avoid expanding current structure for any new fields, > specially which has the SET equivalent. > > > > But if we want to with the path of VIRTIO_F_CFG_SPACE_OVER_AQ, it is fine. > > More things can evolve for generic things like config space over AQ. > > I am not sure what does VIRTIO_F_CFG_SPACE_OVER_AQ mean, and what are > these costs. What I had in mind is all the rarely used and/or one time used config registers are queries over Q interface. A device exposes a feature bit indicating that it offers it via a q interface instead of MMIO. A net device already has a CVQ and its almost always there, so utilizing an existing object to query makes perfect sense. It can be argued other way that, hey other devices cannot benefit for it because they missed the CVQ. So may be a AQ can service for all the device types. > What I had in mind is extending proposed vq transport to support > sriov. I don't see why we can not have exactly 0 bytes of memory per VF. > VFs other than legacy purposes do not undergo mediation via PF. Legacy is the only exception; newer things is direct communication for a PCI device PF, SRIOV VFs and SIOV devices. > And if we care about single bytes of PF memory (do we? there's only one PF per > SRIOV device ...), what we should do is a variant of pci transport that > aggressively saves memory. Users already deploy 16 to 30 PFs coming from single physical card today in single system. Building things uniformly for PF, VF, SIOV devices is coming for free. It is not only this byte, but we also see that device needs to offer many capabilities more than boolean flag, so putting non latency sensitive item on the MMIO area hurts overall. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] Re: [virtio-comment] Re: [PATCH v13] virtio-net: support inner header hash
On Wed, Apr 26, 2023 at 04:27:34AM +, Parav Pandit wrote: > > > From: Michael S. Tsirkin > > Sent: Wednesday, April 26, 2023 12:12 AM > > > > > > No, this is ROM, not RAM. > > > For VFs it is not ROM because one might configure VF with different feature > bits. Hmm interesting. If you want to block a specific feature from VF you probably want to do it for things like LM compat, and I feel the way to do it is in software not in hardware because you want e.g. cross-vendor compatibility, which specific vendors rarely care about. No? For example, we have all the crazy amount of flexibility with layout of pci config space (not virtio config) that we have zero control over unless we want to go and mandate all that in virtio - which would put us on a treadmill of chasing each new pci capability added. BTW I generally wonder about why this aggressive fight against memory mapped registers does not seem to be reflected in the pci spec at all, which seems to keep happily adding more memory mapped stuff for control plane things in each revision. If anyone, I would expect these guys to know a thing or two about pci hardware. > > > CVQ already exists and provides the SET command. There is no reason to do > > GET in some other way. > > > > Spoken looking at just hardware cost :) > Not really. The CVQ is already there, no extra overheads. > > The reason is that this is device specific. Maintainance overhead and system > > RAM cost for the code to support this should not be ignored. > Sure. As above its not a new and such query occurs only once or extremely > rare. It will have to be done each time user runs ethtool -k, no? Unless you cache this in software then it's extra RAM :) > > > > > Device has single channel to provide a value and hence it doesn't need any > > internal synchronization between two paths. > > > > > > So, if we add a new feature bit as VIRTIO_F_CFG_SPACE_OVER_AQ it is an > > improvement. > > > But it still comes with a cost which device cannot mitigate. > > > The cost is, > > > 1. a driver may not negotiate such feature bit, and device ends up burning > > memory. > > > 1.b Device provisioning becomes a factor of what some guests may use/not > > use/already using on the live system. > > > > > > 2. Every device needs AQ even when the CVQ exists. > > > > > > Hence, better to avoid expanding current structure for any new fields, > > specially which has the SET equivalent. > > > > > > But if we want to with the path of VIRTIO_F_CFG_SPACE_OVER_AQ, it is fine. > > > More things can evolve for generic things like config space over AQ. > > > > I am not sure what does VIRTIO_F_CFG_SPACE_OVER_AQ mean, and what are > > these costs. > What I had in mind is all the rarely used and/or one time used config > registers are queries over Q interface. > A device exposes a feature bit indicating that it offers it via a q interface > instead of MMIO. > A net device already has a CVQ and its almost always there, so utilizing an > existing object to query makes perfect sense. > It can be argued other way that, hey other devices cannot benefit for it > because they missed the CVQ. > So may be a AQ can service for all the device types. Exactly. > > What I had in mind is extending proposed vq transport to support > > sriov. I don't see why we can not have exactly 0 bytes of memory per VF. > > > VFs other than legacy purposes do not undergo mediation via PF. > Legacy is the only exception; newer things is direct communication for a PCI > device PF, SRIOV VFs and SIOV devices. > > > And if we care about single bytes of PF memory (do we? there's only one PF > > per > > SRIOV device ...), what we should do is a variant of pci transport that > > aggressively saves memory. > Users already deploy 16 to 30 PFs coming from single physical card today in > single system. > Building things uniformly for PF, VF, SIOV devices is coming for free. > > It is not only this byte, but we also see that device needs to offer many > capabilities more than boolean flag, so putting non latency sensitive item on > the MMIO area hurts overall. -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] RE: [PATCH v13] virtio-net: support inner header hash
> From: Heng Qi > Sent: Wednesday, April 26, 2023 10:04 AM > Yes, but that seems like a tiny cost, and the cvq command-related structure is > much simpler. Current structure size is 24 bytes. This size becomes multiplier with device count scale to be always available and rarely changes. As we add new features such device capabilities grow making the multiplier bigger. For example a. flow steering capabilities (how many flows, what mask, supported protocols, generic options) b. hds capabilities c. counter capabilities (histogram based, which error counters supported, etc) d. which new type of tx vq improvements supported. e. hw gro context count supported May be more.. Depending on the container/VM size certain capabilities may change from device to device. Hence it is hard to deduplicate them at device level. Therefore, ability to query them over a non_always_available transport is preferred choice from the device. A driver may choose to cache it if its being frequently accessed or ask device when needed. Even when it's cached by driver, it is coming from the component that doesn’t have transport level timeouts associated with it.
[virtio-dev] Re: [virtio-comment] RE: [PATCH v13] virtio-net: support inner header hash
On Wed, Apr 26, 2023 at 02:24:48PM +, Parav Pandit wrote: > > > > From: Heng Qi > > Sent: Wednesday, April 26, 2023 10:04 AM > > > Yes, but that seems like a tiny cost, and the cvq command-related structure > > is > > much simpler. > Current structure size is 24 bytes. > This size becomes multiplier with device count scale to be always available > and rarely changes. > > As we add new features such device capabilities grow making the multiplier > bigger. > For example > a. flow steering capabilities (how many flows, what mask, supported > protocols, generic options) > b. hds capabilities > c. counter capabilities (histogram based, which error counters supported, > etc) > d. which new type of tx vq improvements supported. > e. hw gro context count supported > > May be more.. All these are ROM though. Can be shared between functions on a single device, be it VFs or multifunction PF. Yea I heard you about maybe making them programmable. For some cases where there is a hardware resource associated with it, it makes sense. Not in this case though, it's just another way to calculate hash. > Depending on the container/VM size certain capabilities may change from > device to device. > Hence it is hard to deduplicate them at device level. > > Therefore, ability to query them over a non_always_available transport is > preferred choice from the device. > > A driver may choose to cache it if its being frequently accessed or ask > device when needed. > Even when it's cached by driver, it is coming from the component that doesn’t > have transport level timeouts associated with it. well caching by driver is using up same amount of RAM, only with no chance to -- MST - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
[virtio-dev] RE: [virtio-comment] RE: [PATCH v13] virtio-net: support inner header hash
> From: Michael S. Tsirkin > Sent: Wednesday, April 26, 2023 10:58 AM > All these are ROM though. Can be shared between functions on a single device, > be it VFs or multifunction PF. Yea I heard you about maybe making them > programmable. For some cases where there is a hardware resource associated > with it, it makes sense. Right. Some are, some are not ROM. And it demands special circuitry for non-critical path. > Not in this case though, it's just another way to calculate hash. > Sure. Instead of doing field by field bi-section this way requires constant discussion like this. Instead, placement of time critical fields in MMIO is better approach as it is generic guideline vs ROM/RAM fields. > > > Depending on the container/VM size certain capabilities may change from > device to device. > > Hence it is hard to deduplicate them at device level. > > > > Therefore, ability to query them over a non_always_available transport is > preferred choice from the device. > > > > A driver may choose to cache it if its being frequently accessed or ask > > device > when needed. > > Even when it's cached by driver, it is coming from the component that > doesn’t have transport level timeouts associated with it. > > well caching by driver is using up same amount of RAM, only with no chance to > No chance to fault. And better utilizing the device. And it is not always the pinned RAM, future it can be paged out too. Many fields won't be even cached. Some may even get set in the net_device or other kernel level device structures.
Re: [virtio-dev] RE: [virtio-comment] RE: [PATCH v13] virtio-net: support inner header hash
在 2023/4/26 下午10:24, Parav Pandit 写道: From: Heng Qi Sent: Wednesday, April 26, 2023 10:04 AM Yes, but that seems like a tiny cost, and the cvq command-related structure is much simpler. Current structure size is 24 bytes. This size becomes multiplier with device count scale to be always available and rarely changes. As we add new features such device capabilities grow making the multiplier bigger. For example a. flow steering capabilities (how many flows, what mask, supported protocols, generic options) b. hds capabilities c. counter capabilities (histogram based, which error counters supported, etc) d. which new type of tx vq improvements supported. e. hw gro context count supported May be more.. Depending on the container/VM size certain capabilities may change from device to device. Hence it is hard to deduplicate them at device level. This makes sense. In general, we should be careful about adding things to the device space unless the benefit is non-trivial. Thanks. Therefore, ability to query them over a non_always_available transport is preferred choice from the device. A driver may choose to cache it if its being frequently accessed or ask device when needed. Even when it's cached by driver, it is coming from the component that doesn’t have transport level timeouts associated with it. - To unsubscribe, e-mail: virtio-dev-unsubscr...@lists.oasis-open.org For additional commands, e-mail: virtio-dev-h...@lists.oasis-open.org
