Patchwork [1/2,V3] virtio-spec: dynamic network offloads configuration

login
register
mail settings
Submitter Dmitry Fleytman
Date April 2, 2013, 11:38 a.m.
Message ID <1364902740-24948-2-git-send-email-dmitry@daynix.com>
Download mbox | patch
Permalink /patch/232966/
State New
Headers show

Comments

Dmitry Fleytman - April 2, 2013, 11:38 a.m.
From: Dmitry Fleytman <dfleytma@redhat.com>

Virtio-net driver currently negotiates network offloads
on startup via features mechanism and have no ability to
change offloads state later.
This patch introduced a new control command that allows
to configure device network offloads state dynamically.
The patch also introduces a new feature flag
VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.

Signed-off-by: Dmitry Fleytman <dfleytma@redhat.com>
---
 virtio-spec.lyx | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 143 insertions(+)
Rusty Russell - April 3, 2013, 1:20 a.m.
Dmitry Fleytman <dmitry@daynix.com> writes:
> From: Dmitry Fleytman <dfleytma@redhat.com>
>
> Virtio-net driver currently negotiates network offloads
> on startup via features mechanism and have no ability to
> change offloads state later.
> This patch introduced a new control command that allows
> to configure device network offloads state dynamically.
> The patch also introduces a new feature flag
> VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
>
> Signed-off-by: Dmitry Fleytman <dfleytma@redhat.com>

(BTW, I like to be CC'd on these things directly, so I don't miss them)

The idea is fine.

But I dislike the duplication of constants: let's just use the feature
bits directly:

#define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
#define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
#define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
#define VIRTIO_NET_F_GUEST_ECN	9	/* Guest can handle TSO[6] w/ ECN in. */
#define VIRTIO_NET_F_GUEST_UFO	10	/* Guest can handle UFO in. */

You want this, because you have to test against them anyway before
trying to re-enable them.

And secondly, it'll be much clearer if you don't say "change" but
"disable and re-enable", which is what's actually allowed.

Thanks,
Rusty.
Michael S. Tsirkin - April 3, 2013, 11:25 a.m.
On Wed, Apr 03, 2013 at 11:50:19AM +1030, Rusty Russell wrote:
> Dmitry Fleytman <dmitry@daynix.com> writes:
> > From: Dmitry Fleytman <dfleytma@redhat.com>
> >
> > Virtio-net driver currently negotiates network offloads
> > on startup via features mechanism and have no ability to
> > change offloads state later.
> > This patch introduced a new control command that allows
> > to configure device network offloads state dynamically.
> > The patch also introduces a new feature flag
> > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> >
> > Signed-off-by: Dmitry Fleytman <dfleytma@redhat.com>
> 
> (BTW, I like to be CC'd on these things directly, so I don't miss them)
> 
> The idea is fine.
> 
> But I dislike the duplication of constants: let's just use the feature
> bits directly:
> 
> #define VIRTIO_NET_F_GUEST_CSUM	1	/* Guest handles pkts w/ partial csum */
> #define VIRTIO_NET_F_GUEST_TSO4	7	/* Guest can handle TSOv4 in. */
> #define VIRTIO_NET_F_GUEST_TSO6	8	/* Guest can handle TSOv6 in. */
> #define VIRTIO_NET_F_GUEST_ECN	9	/* Guest can handle TSO[6] w/ ECN in. */
> #define VIRTIO_NET_F_GUEST_UFO	10	/* Guest can handle UFO in. */
> 
> You want this, because you have to test against them anyway before
> trying to re-enable them.
> 
> And secondly, it'll be much clearer if you don't say "change" but
> "disable and re-enable", which is what's actually allowed.
> 
> Thanks,
> Rusty.

Okay and let's make command it bigger, say 64 bit then,
in case we add lots of feature bits in the future?
Dmitry Fleytman - April 4, 2013, 7:51 a.m.
Ok, sending new patches...


On Wed, Apr 3, 2013 at 2:25 PM, Michael S. Tsirkin <mst@redhat.com> wrote:

> On Wed, Apr 03, 2013 at 11:50:19AM +1030, Rusty Russell wrote:
> > Dmitry Fleytman <dmitry@daynix.com> writes:
> > > From: Dmitry Fleytman <dfleytma@redhat.com>
> > >
> > > Virtio-net driver currently negotiates network offloads
> > > on startup via features mechanism and have no ability to
> > > change offloads state later.
> > > This patch introduced a new control command that allows
> > > to configure device network offloads state dynamically.
> > > The patch also introduces a new feature flag
> > > VIRTIO_NET_F_CTRL_GUEST_OFFLOADS.
> > >
> > > Signed-off-by: Dmitry Fleytman <dfleytma@redhat.com>
> >
> > (BTW, I like to be CC'd on these things directly, so I don't miss them)
> >
> > The idea is fine.
> >
> > But I dislike the duplication of constants: let's just use the feature
> > bits directly:
> >
> > #define VIRTIO_NET_F_GUEST_CSUM       1       /* Guest handles pkts w/
> partial csum */
> > #define VIRTIO_NET_F_GUEST_TSO4       7       /* Guest can handle TSOv4
> in. */
> > #define VIRTIO_NET_F_GUEST_TSO6       8       /* Guest can handle TSOv6
> in. */
> > #define VIRTIO_NET_F_GUEST_ECN        9       /* Guest can handle TSO[6]
> w/ ECN in. */
> > #define VIRTIO_NET_F_GUEST_UFO        10      /* Guest can handle UFO
> in. */
> >
> > You want this, because you have to test against them anyway before
> > trying to re-enable them.
> >
> > And secondly, it'll be much clearer if you don't say "change" but
> > "disable and re-enable", which is what's actually allowed.
> >
> > Thanks,
> > Rusty.
>
> Okay and let's make command it bigger, say 64 bit then,
> in case we add lots of feature bits in the future?
>
>
>

Patch

diff --git a/virtio-spec.lyx b/virtio-spec.lyx
index 3d2f485..fdba814 100644
--- a/virtio-spec.lyx
+++ b/virtio-spec.lyx
@@ -60,6 +60,7 @@ 
 \author -1930653948 "Amos Kong" 
 \author -608949062 "Rusty Russell,,," 
 \author -385801441 "Cornelia Huck" cornelia.huck@de.ibm.com
+\author 460276516 "Dmitry Fleytman" dfleytma@redhat.com
 \author 1112500848 "Rusty Russell" rusty@rustcorp.com.au
 \author 1531152142 "Paolo Bonzini,,," 
 \author 1717892615 "Alexey Zaytsev,,," 
@@ -4261,6 +4262,20 @@  VIRTIO_NET_F_GUEST_CSUM
 \end_inset
 
 (1) Guest handles packets with partial checksum
+\change_inserted 460276516 1363712169
+
+\end_layout
+
+\begin_layout Description
+
+\change_inserted 460276516 1363712334
+VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
+\begin_inset space ~
+\end_inset
+
+(2) Control channel offloads reconfiguration support.
+\change_unchanged
+
 \end_layout
 
 \begin_layout Description
@@ -5675,6 +5690,134 @@  virtqueue_pairs = 1
  
 \end_layout
 
+\begin_layout Subsection*
+
+\change_inserted 460276516 1363765850
+Offloads State Configuration
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363765861
+If the VIRTIO_NET_F_CTRL_GUEST_OFFLOADS feature is negotiated, the driver can
+ send control commands for dynamic offloads state configuration.
+\end_layout
+
+\begin_layout Subsubsection*
+
+\change_inserted 460276516 1363765928
+Setting offloads state
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363713225
+\begin_inset listings
+inline false
+status open
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363765996
+
+u32 offloads;
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363765997
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766044
+
+#define VIRTIO_NET_OFFLOAD_GUEST_CSUM       1
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766051
+
+#define VIRTIO_NET_OFFLOAD_GUEST_TSO4       2
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766055
+
+#define VIRTIO_NET_OFFLOAD_GUEST_TSO6       4
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766064
+
+#define VIRTIO_NET_OFFLOAD_GUEST_ECN        8
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766035
+
+#define VIRTIO_NET_OFFLOAD_GUEST_UFO       16
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363766031
+
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363765865
+
+#define VIRTIO_NET_CTRL_GUEST_OFFLOADS       5
+\end_layout
+
+\begin_layout Plain Layout
+
+\change_inserted 460276516 1363765867
+
+ #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET          0
+\end_layout
+
+\end_inset
+
+
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363766082
+The class VIRTIO_NET_CTRL_GUEST_OFFLOADS has one command: VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET
+ applies the new offloads configuration.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363766435
+u32 value passed as command data is a bitmask, bits set define offloads
+ to be enabled, bits cleared - offloads to be disabled.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363766757
+There is a corresponding device feature for each offload.
+ Upon feature negotiation corresponding offload gets enabled to preserve
+ backward compartibility.
+\end_layout
+
+\begin_layout Standard
+
+\change_inserted 460276516 1363766720
+Corresponding feature must be negotiated at startup in order to allow dynamic
+ change of specific offload state.
+\end_layout
+
 \begin_layout Chapter*
 Appendix D: Block Device
 \end_layout