[PATCH] Register watches for frontend features.

[PATCH] Register watches for frontend features.

[Paul Durrant]

Frontend features are currently only sampled at ring connection time.
Some frontends can change these features dynamically so watches should
be used to update the corresponding netback flags as and when they
change.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
---
 drivers/xen/netback/common.h |    8 +
 drivers/xen/netback/xenbus.c |  293 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 274 insertions(+), 27 deletions(-)

diff --git a/drivers/xen/netback/common.h b/drivers/xen/netback/common.h
index a673331..3a0abf6 100644
--- a/drivers/xen/netback/common.h
+++ b/drivers/xen/netback/common.h
@@ -154,6 +154,14 @@ struct netback_accelerator {
 struct backend_info {
 	struct xenbus_device *dev;
 	struct xen_netif *netif;
+
+	/* Frontend feature watches. */
+    struct xenbus_watch can_sg_watch;
+    struct xenbus_watch gso_watch;
+    struct xenbus_watch gso_prefix_watch;
+    struct xenbus_watch csum_watch;
+    struct xenbus_watch smart_poll_watch;
+
 	enum xenbus_state frontend_state;
 	struct xenbus_watch hotplug_status_watch;
 	int have_hotplug_status_watch:1;
diff --git a/drivers/xen/netback/xenbus.c b/drivers/xen/netback/xenbus.c
index 99831c7..d08463c 100644
--- a/drivers/xen/netback/xenbus.c
+++ b/drivers/xen/netback/xenbus.c
@@ -33,6 +33,7 @@ static int connect_rings(struct backend_info *);
 static void connect(struct backend_info *);
 static void backend_create_netif(struct backend_info *be);
 static void unregister_hotplug_status_watch(struct backend_info *be);
+static void unregister_frontend_watches(struct backend_info *be);
 
 static int netback_remove(struct xenbus_device *dev)
 {
@@ -44,6 +45,7 @@ static int netback_remove(struct xenbus_device *dev)
 	if (be->netif) {
 		kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
 		xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
+		unregister_frontend_watches(be);
 		netif_disconnect(be->netif);
 		be->netif = NULL;
 	}
@@ -227,6 +229,7 @@ static void disconnect_backend(struct xenbus_device *dev)
 
 	if (be->netif) {
 		xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
+		unregister_frontend_watches(be);
 		netif_disconnect(be->netif);
 		be->netif = NULL;
 	}
@@ -409,6 +412,268 @@ static void connect(struct backend_info *be)
 	netif_wake_queue(be->netif->dev);
 }
 
+static void feature_can_sg_watch(struct xenbus_watch *watch,
+			         const char **vec,
+			         unsigned int vec_size)
+{
+	struct backend_info *be = container_of(watch,
+					       struct backend_info,
+					       can_sg_watch);
+	struct xen_netif *netif = be->netif;
+	struct xenbus_device *dev = be->dev;
+	int val;
+
+	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-sg",
+			 "%d", &val) < 0)
+		val = 0;
+	netif->can_sg = !!val;
+	DPRINTK("can_sg -> %d\n", netif->can_sg);
+
+	netif_set_features(netif);
+}
+
+static int register_can_sg_watch(struct backend_info *be)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	if (be->can_sg_watch.node)
+		return -EEXIST;
+
+	err = xenbus_watch_pathfmt(dev, &be->can_sg_watch,
+				   feature_can_sg_watch, "%s/%s",
+				   dev->otherend, "feature-sg");
+	if (err) {
+		xenbus_dev_fatal(dev, err, "watching %s/feature-sg",
+				 dev->otherend);
+		return err;
+	}
+	return 0;
+}
+
+static void unregister_can_sg_watch(struct backend_info *be)
+{
+	if (!be->can_sg_watch.node)
+		return;
+
+	unregister_xenbus_watch(&be->can_sg_watch);
+	kfree(be->can_sg_watch.node);
+	be->can_sg_watch.node = NULL;
+}
+
+static void feature_gso_watch(struct xenbus_watch *watch,
+			      const char **vec,
+			      unsigned int vec_size)
+{
+	struct backend_info *be = container_of(watch,
+					       struct backend_info,
+					       gso_watch);
+	struct xen_netif *netif = be->netif;
+	struct xenbus_device *dev = be->dev;
+	int val;
+
+	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4",
+			 "%d", &val) < 0)
+		val = 0;
+	netif->gso = !!val;
+	DPRINTK("gso -> %d\n", netif->gso);
+
+	netif_set_features(netif);
+}
+
+static int register_gso_watch(struct backend_info *be)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	if (be->gso_watch.node)
+		return -EEXIST;
+
+	err = xenbus_watch_pathfmt(dev, &be->gso_watch,
+				   feature_gso_watch, "%s/%s",
+				   dev->otherend, "feature-gso-tcpv4");
+	if (err) {
+		xenbus_dev_fatal(dev, err, "watching %s/feature-gso-tcpv4",
+				 dev->otherend);
+		return err;
+	}
+	return 0;
+}
+
+static void unregister_gso_watch(struct backend_info *be)
+{
+	if (!be->gso_watch.node)
+		return;
+
+	unregister_xenbus_watch(&be->gso_watch);
+	kfree(be->gso_watch.node);
+	be->gso_watch.node = NULL;
+}
+
+static void feature_gso_prefix_watch(struct xenbus_watch *watch,
+				     const char **vec,
+				     unsigned int vec_size)
+{
+	struct backend_info *be = container_of(watch,
+					       struct backend_info,
+					       gso_prefix_watch);
+	struct xen_netif *netif = be->netif;
+	struct xenbus_device *dev = be->dev;
+	int val;
+
+	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4-prefix",
+			 "%d", &val) < 0)
+		val = 0;
+	netif->gso_prefix = !!val;
+	DPRINTK("gso_prefix -> %d\n", netif->gso_prefix);
+
+	netif_set_features(netif);
+}
+
+static int register_gso_prefix_watch(struct backend_info *be)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	if (be->gso_prefix_watch.node)
+		return -EEXIST;
+
+	err = xenbus_watch_pathfmt(dev, &be->gso_prefix_watch,
+				   feature_gso_prefix_watch, "%s/%s",
+				   dev->otherend, "feature-gso-tcpv4-prefix");
+	if (err) {
+		xenbus_dev_fatal(dev, err, "watching %s/feature-gso-tcpv4-prefix",
+				 dev->otherend);
+		return err;
+	}
+	return 0;
+}
+
+static void unregister_gso_prefix_watch(struct backend_info *be)
+{
+	if (!be->gso_prefix_watch.node)
+		return;
+
+	unregister_xenbus_watch(&be->gso_prefix_watch);
+	kfree(be->gso_prefix_watch.node);
+	be->gso_prefix_watch.node = NULL;
+}
+
+static void feature_csum_watch(struct xenbus_watch *watch,
+			       const char **vec,
+			       unsigned int vec_size)
+{
+	struct backend_info *be = container_of(watch,
+					       struct backend_info,
+					       csum_watch);
+	struct xen_netif *netif = be->netif;
+	struct xenbus_device *dev = be->dev;
+	int val;
+
+	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-no-csum",
+			 "%d", &val) < 0)
+		val = 0;
+	netif->csum = !val;
+	DPRINTK("csum -> %d\n", netif->csum);
+
+	netif_set_features(netif);
+}
+
+static int register_csum_watch(struct backend_info *be)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	if (be->csum_watch.node)
+		return -EEXIST;
+
+	err = xenbus_watch_pathfmt(dev, &be->csum_watch,
+				   feature_csum_watch, "%s/%s",
+				   dev->otherend, "feature-no-csum");
+	if (err) {
+		xenbus_dev_fatal(dev, err, "watching %s/feature-no-csum",
+				 dev->otherend);
+		return err;
+	}
+	return 0;
+}
+
+static void unregister_csum_watch(struct backend_info *be)
+{
+	if (!be->csum_watch.node)
+		return;
+
+	unregister_xenbus_watch(&be->csum_watch);
+	kfree(be->csum_watch.node);
+	be->csum_watch.node = NULL;
+}
+
+static void feature_smart_poll_watch(struct xenbus_watch *watch,
+				     const char **vec,
+				     unsigned int vec_size)
+{
+	struct backend_info *be = container_of(watch,
+					       struct backend_info,
+					       smart_poll_watch);
+	struct xen_netif *netif = be->netif;
+	struct xenbus_device *dev = be->dev;
+	int val;
+
+	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-smart-poll",
+			 "%d", &val) < 0)
+		val = 0;
+	netif->smart_poll = !!val;
+	DPRINTK("smart_poll -> %d\n", netif->smart_poll);
+
+	netif_set_features(netif);
+}
+
+static int register_smart_poll_watch(struct backend_info *be)
+{
+	struct xenbus_device *dev = be->dev;
+	int err;
+
+	if (be->smart_poll_watch.node)
+		return -EEXIST;
+
+	err = xenbus_watch_pathfmt(dev, &be->smart_poll_watch,
+				   feature_smart_poll_watch, "%s/%s",
+				   dev->otherend, "feature-smart-poll");
+	if (err) {
+		xenbus_dev_fatal(dev, err, "watching %s/feature-smart-poll",
+				 dev->otherend);
+		return err;
+	}
+	return 0;
+}
+
+static void unregister_smart_poll_watch(struct backend_info *be)
+{
+	if (!be->smart_poll_watch.node)
+		return;
+
+	unregister_xenbus_watch(&be->smart_poll_watch);
+	kfree(be->smart_poll_watch.node);
+	be->smart_poll_watch.node = NULL;
+}
+
+static void register_frontend_watches(struct backend_info *be)
+{
+	register_can_sg_watch(be);
+	register_gso_watch(be);
+	register_gso_prefix_watch(be);
+	register_csum_watch(be);
+	register_smart_poll_watch(be);
+}
+
+static void unregister_frontend_watches(struct backend_info *be)
+{
+	unregister_can_sg_watch(be);
+	unregister_gso_watch(be);
+	unregister_gso_prefix_watch(be);
+	unregister_csum_watch(be);
+	unregister_smart_poll_watch(be);
+}
 
 static int connect_rings(struct backend_info *be)
 {
@@ -457,33 +722,7 @@ static int connect_rings(struct backend_info *be)
 			netif->dev->tx_queue_len = 1;
 	}
 
-	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-sg",
-			 "%d", &val) < 0)
-		val = 0;
-	netif->can_sg = !!val;
-
-	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4",
-			 "%d", &val) < 0)
-		val = 0;
-	netif->gso = !!val;
-
-	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4-prefix",
-			 "%d", &val) < 0)
-		val = 0;
-	netif->gso_prefix = !!val;
-
-	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-no-csum-offload",
-			 "%d", &val) < 0)
-		val = 0;
-	netif->csum = !val;
-
-	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-smart-poll",
-			 "%d", &val) < 0)
-		val = 0;
-	netif->smart_poll = !!val;
-
-	/* Set dev->features */
-	netif_set_features(netif);
+	register_frontend_watches(be);
 
 	/* Map the shared frame, irq etc. */
 	err = netif_map(netif, tx_ring_ref, rx_ring_ref, evtchn);
-- 
1.5.6.5

Re: [PATCH] Register watches for frontend features.

[Jeremy Fitzhardinge]

On 07/05/2010 04:39 AM, Paul Durrant wrote:
> Frontend features are currently only sampled at ring connection time.
> Some frontends can change these features dynamically so watches should
> be used to update the corresponding netback flags as and when they
> change.
>   

Are there any concerns about these feature flags changing asynchronously?

The watches will run in their own task, and so can be concurrent to the
netback code using the flags.  What prevents that from upsetting the
tests of the flags in, for example, netbk_max_required_rx_slots?  Should
there be some locking?  Or double-buffer the feature flags so that the
netback code can get updated values at a safe/appropriate time?

Can all features be changed dynamically by the frontend, or just some?

    J

> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> ---
>  drivers/xen/netback/common.h |    8 +
>  drivers/xen/netback/xenbus.c |  293 ++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 274 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/xen/netback/common.h b/drivers/xen/netback/common.h
> index a673331..3a0abf6 100644
> --- a/drivers/xen/netback/common.h
> +++ b/drivers/xen/netback/common.h
> @@ -154,6 +154,14 @@ struct netback_accelerator {
>  struct backend_info {
>  	struct xenbus_device *dev;
>  	struct xen_netif *netif;
> +
> +	/* Frontend feature watches. */
> +    struct xenbus_watch can_sg_watch;
> +    struct xenbus_watch gso_watch;
> +    struct xenbus_watch gso_prefix_watch;
> +    struct xenbus_watch csum_watch;
> +    struct xenbus_watch smart_poll_watch;
> +
>  	enum xenbus_state frontend_state;
>  	struct xenbus_watch hotplug_status_watch;
>  	int have_hotplug_status_watch:1;
> diff --git a/drivers/xen/netback/xenbus.c b/drivers/xen/netback/xenbus.c
> index 99831c7..d08463c 100644
> --- a/drivers/xen/netback/xenbus.c
> +++ b/drivers/xen/netback/xenbus.c
> @@ -33,6 +33,7 @@ static int connect_rings(struct backend_info *);
>  static void connect(struct backend_info *);
>  static void backend_create_netif(struct backend_info *be);
>  static void unregister_hotplug_status_watch(struct backend_info *be);
> +static void unregister_frontend_watches(struct backend_info *be);
>  
>  static int netback_remove(struct xenbus_device *dev)
>  {
> @@ -44,6 +45,7 @@ static int netback_remove(struct xenbus_device *dev)
>  	if (be->netif) {
>  		kobject_uevent(&dev->dev.kobj, KOBJ_OFFLINE);
>  		xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
> +		unregister_frontend_watches(be);
>  		netif_disconnect(be->netif);
>  		be->netif = NULL;
>  	}
> @@ -227,6 +229,7 @@ static void disconnect_backend(struct xenbus_device *dev)
>  
>  	if (be->netif) {
>  		xenbus_rm(XBT_NIL, dev->nodename, "hotplug-status");
> +		unregister_frontend_watches(be);
>  		netif_disconnect(be->netif);
>  		be->netif = NULL;
>  	}
> @@ -409,6 +412,268 @@ static void connect(struct backend_info *be)
>  	netif_wake_queue(be->netif->dev);
>  }
>  
> +static void feature_can_sg_watch(struct xenbus_watch *watch,
> +			         const char **vec,
> +			         unsigned int vec_size)
> +{
> +	struct backend_info *be = container_of(watch,
> +					       struct backend_info,
> +					       can_sg_watch);
> +	struct xen_netif *netif = be->netif;
> +	struct xenbus_device *dev = be->dev;
> +	int val;
> +
> +	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-sg",
> +			 "%d", &val) < 0)
> +		val = 0;
> +	netif->can_sg = !!val;
> +	DPRINTK("can_sg -> %d\n", netif->can_sg);
> +
> +	netif_set_features(netif);
> +}
> +
> +static int register_can_sg_watch(struct backend_info *be)
> +{
> +	struct xenbus_device *dev = be->dev;
> +	int err;
> +
> +	if (be->can_sg_watch.node)
> +		return -EEXIST;
> +
> +	err = xenbus_watch_pathfmt(dev, &be->can_sg_watch,
> +				   feature_can_sg_watch, "%s/%s",
> +				   dev->otherend, "feature-sg");
> +	if (err) {
> +		xenbus_dev_fatal(dev, err, "watching %s/feature-sg",
> +				 dev->otherend);
> +		return err;
> +	}
> +	return 0;
> +}
> +
> +static void unregister_can_sg_watch(struct backend_info *be)
> +{
> +	if (!be->can_sg_watch.node)
> +		return;
> +
> +	unregister_xenbus_watch(&be->can_sg_watch);
> +	kfree(be->can_sg_watch.node);
> +	be->can_sg_watch.node = NULL;
> +}
> +
> +static void feature_gso_watch(struct xenbus_watch *watch,
> +			      const char **vec,
> +			      unsigned int vec_size)
> +{
> +	struct backend_info *be = container_of(watch,
> +					       struct backend_info,
> +					       gso_watch);
> +	struct xen_netif *netif = be->netif;
> +	struct xenbus_device *dev = be->dev;
> +	int val;
> +
> +	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4",
> +			 "%d", &val) < 0)
> +		val = 0;
> +	netif->gso = !!val;
> +	DPRINTK("gso -> %d\n", netif->gso);
> +
> +	netif_set_features(netif);
> +}
> +
> +static int register_gso_watch(struct backend_info *be)
> +{
> +	struct xenbus_device *dev = be->dev;
> +	int err;
> +
> +	if (be->gso_watch.node)
> +		return -EEXIST;
> +
> +	err = xenbus_watch_pathfmt(dev, &be->gso_watch,
> +				   feature_gso_watch, "%s/%s",
> +				   dev->otherend, "feature-gso-tcpv4");
> +	if (err) {
> +		xenbus_dev_fatal(dev, err, "watching %s/feature-gso-tcpv4",
> +				 dev->otherend);
> +		return err;
> +	}
> +	return 0;
> +}
> +
> +static void unregister_gso_watch(struct backend_info *be)
> +{
> +	if (!be->gso_watch.node)
> +		return;
> +
> +	unregister_xenbus_watch(&be->gso_watch);
> +	kfree(be->gso_watch.node);
> +	be->gso_watch.node = NULL;
> +}
> +
> +static void feature_gso_prefix_watch(struct xenbus_watch *watch,
> +				     const char **vec,
> +				     unsigned int vec_size)
> +{
> +	struct backend_info *be = container_of(watch,
> +					       struct backend_info,
> +					       gso_prefix_watch);
> +	struct xen_netif *netif = be->netif;
> +	struct xenbus_device *dev = be->dev;
> +	int val;
> +
> +	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4-prefix",
> +			 "%d", &val) < 0)
> +		val = 0;
> +	netif->gso_prefix = !!val;
> +	DPRINTK("gso_prefix -> %d\n", netif->gso_prefix);
> +
> +	netif_set_features(netif);
> +}
> +
> +static int register_gso_prefix_watch(struct backend_info *be)
> +{
> +	struct xenbus_device *dev = be->dev;
> +	int err;
> +
> +	if (be->gso_prefix_watch.node)
> +		return -EEXIST;
> +
> +	err = xenbus_watch_pathfmt(dev, &be->gso_prefix_watch,
> +				   feature_gso_prefix_watch, "%s/%s",
> +				   dev->otherend, "feature-gso-tcpv4-prefix");
> +	if (err) {
> +		xenbus_dev_fatal(dev, err, "watching %s/feature-gso-tcpv4-prefix",
> +				 dev->otherend);
> +		return err;
> +	}
> +	return 0;
> +}
> +
> +static void unregister_gso_prefix_watch(struct backend_info *be)
> +{
> +	if (!be->gso_prefix_watch.node)
> +		return;
> +
> +	unregister_xenbus_watch(&be->gso_prefix_watch);
> +	kfree(be->gso_prefix_watch.node);
> +	be->gso_prefix_watch.node = NULL;
> +}
> +
> +static void feature_csum_watch(struct xenbus_watch *watch,
> +			       const char **vec,
> +			       unsigned int vec_size)
> +{
> +	struct backend_info *be = container_of(watch,
> +					       struct backend_info,
> +					       csum_watch);
> +	struct xen_netif *netif = be->netif;
> +	struct xenbus_device *dev = be->dev;
> +	int val;
> +
> +	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-no-csum",
> +			 "%d", &val) < 0)
> +		val = 0;
> +	netif->csum = !val;
> +	DPRINTK("csum -> %d\n", netif->csum);
> +
> +	netif_set_features(netif);
> +}
> +
> +static int register_csum_watch(struct backend_info *be)
> +{
> +	struct xenbus_device *dev = be->dev;
> +	int err;
> +
> +	if (be->csum_watch.node)
> +		return -EEXIST;
> +
> +	err = xenbus_watch_pathfmt(dev, &be->csum_watch,
> +				   feature_csum_watch, "%s/%s",
> +				   dev->otherend, "feature-no-csum");
> +	if (err) {
> +		xenbus_dev_fatal(dev, err, "watching %s/feature-no-csum",
> +				 dev->otherend);
> +		return err;
> +	}
> +	return 0;
> +}
> +
> +static void unregister_csum_watch(struct backend_info *be)
> +{
> +	if (!be->csum_watch.node)
> +		return;
> +
> +	unregister_xenbus_watch(&be->csum_watch);
> +	kfree(be->csum_watch.node);
> +	be->csum_watch.node = NULL;
> +}
> +
> +static void feature_smart_poll_watch(struct xenbus_watch *watch,
> +				     const char **vec,
> +				     unsigned int vec_size)
> +{
> +	struct backend_info *be = container_of(watch,
> +					       struct backend_info,
> +					       smart_poll_watch);
> +	struct xen_netif *netif = be->netif;
> +	struct xenbus_device *dev = be->dev;
> +	int val;
> +
> +	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-smart-poll",
> +			 "%d", &val) < 0)
> +		val = 0;
> +	netif->smart_poll = !!val;
> +	DPRINTK("smart_poll -> %d\n", netif->smart_poll);
> +
> +	netif_set_features(netif);
> +}
> +
> +static int register_smart_poll_watch(struct backend_info *be)
> +{
> +	struct xenbus_device *dev = be->dev;
> +	int err;
> +
> +	if (be->smart_poll_watch.node)
> +		return -EEXIST;
> +
> +	err = xenbus_watch_pathfmt(dev, &be->smart_poll_watch,
> +				   feature_smart_poll_watch, "%s/%s",
> +				   dev->otherend, "feature-smart-poll");
> +	if (err) {
> +		xenbus_dev_fatal(dev, err, "watching %s/feature-smart-poll",
> +				 dev->otherend);
> +		return err;
> +	}
> +	return 0;
> +}
> +
> +static void unregister_smart_poll_watch(struct backend_info *be)
> +{
> +	if (!be->smart_poll_watch.node)
> +		return;
> +
> +	unregister_xenbus_watch(&be->smart_poll_watch);
> +	kfree(be->smart_poll_watch.node);
> +	be->smart_poll_watch.node = NULL;
> +}
> +
> +static void register_frontend_watches(struct backend_info *be)
> +{
> +	register_can_sg_watch(be);
> +	register_gso_watch(be);
> +	register_gso_prefix_watch(be);
> +	register_csum_watch(be);
> +	register_smart_poll_watch(be);
> +}
> +
> +static void unregister_frontend_watches(struct backend_info *be)
> +{
> +	unregister_can_sg_watch(be);
> +	unregister_gso_watch(be);
> +	unregister_gso_prefix_watch(be);
> +	unregister_csum_watch(be);
> +	unregister_smart_poll_watch(be);
> +}
>  
>  static int connect_rings(struct backend_info *be)
>  {
> @@ -457,33 +722,7 @@ static int connect_rings(struct backend_info *be)
>  			netif->dev->tx_queue_len = 1;
>  	}
>  
> -	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-sg",
> -			 "%d", &val) < 0)
> -		val = 0;
> -	netif->can_sg = !!val;
> -
> -	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4",
> -			 "%d", &val) < 0)
> -		val = 0;
> -	netif->gso = !!val;
> -
> -	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-gso-tcpv4-prefix",
> -			 "%d", &val) < 0)
> -		val = 0;
> -	netif->gso_prefix = !!val;
> -
> -	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-no-csum-offload",
> -			 "%d", &val) < 0)
> -		val = 0;
> -	netif->csum = !val;
> -
> -	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-smart-poll",
> -			 "%d", &val) < 0)
> -		val = 0;
> -	netif->smart_poll = !!val;
> -
> -	/* Set dev->features */
> -	netif_set_features(netif);
> +	register_frontend_watches(be);
>  
>  	/* Map the shared frame, irq etc. */
>  	err = netif_map(netif, tx_ring_ref, rx_ring_ref, evtchn);
>   

RE: [PATCH] Register watches for frontend features.

[Paul Durrant]

> -----Original Message-----
> From: Jeremy Fitzhardinge [mailto:jeremy@goop.org]
> Sent: 07 July 2010 19:11
> To: Paul Durrant
> Cc: xen-devel@lists.xensource.com; Ian Campbell
> Subject: Re: [Xen-devel] [PATCH] Register watches for frontend
> features.
> 
> On 07/05/2010 04:39 AM, Paul Durrant wrote:
> > Frontend features are currently only sampled at ring connection
> time.
> > Some frontends can change these features dynamically so watches
> should
> > be used to update the corresponding netback flags as and when they
> > change.
> >
> 
> Are there any concerns about these feature flags changing
> asynchronously?
> 
> The watches will run in their own task, and so can be concurrent to
> the
> netback code using the flags.  What prevents that from upsetting the
> tests of the flags in, for example, netbk_max_required_rx_slots?
> Should
> there be some locking?  Or double-buffer the feature flags so that
> the
> netback code can get updated values at a safe/appropriate time?
> 
> Can all features be changed dynamically by the frontend, or just
> some?
> 

Realistically, with a Windows frontend, the only ones that are going to change dynamically are gso_prefix and csum. I can 'de-watch' the others, if you'd prefer.
As for locking, it's not clear to me how fatal getting netbk_max_required_rx_slots wrong is. However, losing or gaining gso/gso_prefix part way through a receive is not going to be good so I thing some double buffering is probably called for. I'll re-work the patch accordingly.

  Paul

Re: [PATCH] Register watches for frontend features.

[Jeremy Fitzhardinge]

On 07/09/2010 05:53 AM, Paul Durrant wrote:
> Realistically, with a Windows frontend, the only ones that are going to change dynamically are gso_prefix and csum. I can 'de-watch' the others, if you'd prefer.
> As for locking, it's not clear to me how fatal getting netbk_max_required_rx_slots wrong is. However, losing or gaining gso/gso_prefix part way through a receive is not going to be good so I thing some double buffering is probably called for. I'll re-work the patch accordingly.
>   

Yes.  Given that those are guest-controlled, dom0 needs to be very
careful in how it handles them.

    J