All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] net: xen-netback: convert to hw_features
@ 2011-04-19 11:56 Michał Mirosław
  2011-04-19 13:17 ` Ian Campbell
  2011-04-19 13:35 ` [PATCH v2] " Michał Mirosław
  0 siblings, 2 replies; 11+ messages in thread
From: Michał Mirosław @ 2011-04-19 11:56 UTC (permalink / raw)
  To: netdev; +Cc: Ian Campbell, xen-devel

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/net/xen-netback/common.h    |    3 -
 drivers/net/xen-netback/interface.c |   84 ++++++----------------------------
 2 files changed, 15 insertions(+), 72 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 5d7bbf2..8753e6d 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -73,9 +73,6 @@ struct xenvif {
 	struct vm_struct *tx_comms_area;
 	struct vm_struct *rx_comms_area;
 
-	/* Flags that must not be set in dev->features */
-	u32 features_disabled;
-
 	/* Frontend feature information. */
 	u8 can_sg:1;
 	u8 gso:1;
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index de569cc..fe25308 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -165,69 +165,18 @@ static int xenvif_change_mtu(struct net_device *dev, int mtu)
 	return 0;
 }
 
-static void xenvif_set_features(struct xenvif *vif)
+static u32 xenvif_fix_features(struct xenvif *vif, u32 features)
 {
 	struct net_device *dev = vif->dev;
-	u32 features = dev->features;
 
-	if (vif->can_sg)
-		features |= NETIF_F_SG;
-	if (vif->gso || vif->gso_prefix)
-		features |= NETIF_F_TSO;
-	if (vif->csum)
-		features |= NETIF_F_IP_CSUM;
+	if (!vif->can_sg)
+		features &= ~NETIF_F_SG;
+	if (!vif->gso && !vif->gso_prefix)
+		features &= ~NETIF_F_TSO;
+	if (!vif->csum)
+		features &= ~NETIF_F_IP_CSUM;
 
-	features &= ~(vif->features_disabled);
-
-	if (!(features & NETIF_F_SG) && dev->mtu > ETH_DATA_LEN)
-		dev->mtu = ETH_DATA_LEN;
-
-	dev->features = features;
-}
-
-static int xenvif_set_tx_csum(struct net_device *dev, u32 data)
-{
-	struct xenvif *vif = netdev_priv(dev);
-	if (data) {
-		if (!vif->csum)
-			return -EOPNOTSUPP;
-		vif->features_disabled &= ~NETIF_F_IP_CSUM;
-	} else {
-		vif->features_disabled |= NETIF_F_IP_CSUM;
-	}
-
-	xenvif_set_features(vif);
-	return 0;
-}
-
-static int xenvif_set_sg(struct net_device *dev, u32 data)
-{
-	struct xenvif *vif = netdev_priv(dev);
-	if (data) {
-		if (!vif->can_sg)
-			return -EOPNOTSUPP;
-		vif->features_disabled &= ~NETIF_F_SG;
-	} else {
-		vif->features_disabled |= NETIF_F_SG;
-	}
-
-	xenvif_set_features(vif);
-	return 0;
-}
-
-static int xenvif_set_tso(struct net_device *dev, u32 data)
-{
-	struct xenvif *vif = netdev_priv(dev);
-	if (data) {
-		if (!vif->gso && !vif->gso_prefix)
-			return -EOPNOTSUPP;
-		vif->features_disabled &= ~NETIF_F_TSO;
-	} else {
-		vif->features_disabled |= NETIF_F_TSO;
-	}
-
-	xenvif_set_features(vif);
-	return 0;
+	return features;
 }
 
 static const struct xenvif_stat {
@@ -274,12 +223,6 @@ static void xenvif_get_strings(struct net_device *dev, u32 stringset, u8 * data)
 }
 
 static struct ethtool_ops xenvif_ethtool_ops = {
-	.get_tx_csum	= ethtool_op_get_tx_csum,
-	.set_tx_csum	= xenvif_set_tx_csum,
-	.get_sg		= ethtool_op_get_sg,
-	.set_sg		= xenvif_set_sg,
-	.get_tso	= ethtool_op_get_tso,
-	.set_tso	= xenvif_set_tso,
 	.get_link	= ethtool_op_get_link,
 
 	.get_sset_count = xenvif_get_sset_count,
@@ -293,6 +236,7 @@ static struct net_device_ops xenvif_netdev_ops = {
 	.ndo_open	= xenvif_open,
 	.ndo_stop	= xenvif_close,
 	.ndo_change_mtu	= xenvif_change_mtu,
+	.ndo_fix_features = xenvif_fix_features,
 };
 
 struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
@@ -331,7 +275,8 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 	vif->credit_timeout.expires = jiffies;
 
 	dev->netdev_ops	= &xenvif_netdev_ops;
-	xenvif_set_features(vif);
+	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
+	dev->features = dev->hw_features;
 	SET_ETHTOOL_OPS(dev, &xenvif_ethtool_ops);
 
 	dev->tx_queue_len = XENVIF_QUEUE_LENGTH;
@@ -367,8 +312,6 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
 	if (vif->irq)
 		return 0;
 
-	xenvif_set_features(vif);
-
 	err = xen_netbk_map_frontend_rings(vif, tx_ring_ref, rx_ring_ref);
 	if (err < 0)
 		goto err;
@@ -384,9 +327,12 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
 	xenvif_get(vif);
 
 	rtnl_lock();
-	netif_carrier_on(vif->dev);
 	if (netif_running(vif->dev))
 		xenvif_up(vif);
+	if (!vif->can_sg && vif->dev->mtu > ETH_DATA_LEN)
+		dev_set_mtu(vif->dev, ETH_DATA_LEN);
+	netdev_update_features(vif->dev);
+	netif_carrier_on(vif->dev);
 	rtnl_unlock();
 
 	return 0;
-- 
1.7.2.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: xen-netback: convert to hw_features
  2011-04-19 11:56 [PATCH] net: xen-netback: convert to hw_features Michał Mirosław
@ 2011-04-19 13:17 ` Ian Campbell
  2011-04-19 13:30   ` Michał Mirosław
  2011-04-19 13:35 ` [PATCH v2] " Michał Mirosław
  1 sibling, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2011-04-19 13:17 UTC (permalink / raw)
  To: Michał Mirosław; +Cc: netdev, xen-devel

On Tue, 2011-04-19 at 12:56 +0100, Michał Mirosław wrote:
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

Thanks for beating me to this! However the prototype for
xenvif_fix_features is wrong (needs to take a net_device not a xenvif).

I fixed it with the following, I also moved the !can_sg MTU clamping
into a set_features hook (like we do with netfront). Am I right that
this pattern copes with changes to SG via ethtool etc better? I think
it's more future proof in any case.

NB: I'm having some issues with my test hardware at the moment so this
is reviewed by eye and compile tested only...

I'm also happy for this to be folded into the original with my
"Signed-off-/Acked-by Ian Campbell <ian.campbell@citrix.com>" if that is
preferable.

Cheers,
Ian.
8<-----------------

net: xen-netback: correct prototype of xenvif_fix_features.

Also check MTU vs NETIF_F_SG in ndo_set_features hook to allow for
dynamic modification.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>

diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index fe25308..61757bd 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -165,9 +165,9 @@ static int xenvif_change_mtu(struct net_device *dev, int mtu)
 	return 0;
 }
 
-static u32 xenvif_fix_features(struct xenvif *vif, u32 features)
+static u32 xenvif_fix_features(struct net_device *dev, u32 features)
 {
-	struct net_device *dev = vif->dev;
+	struct xenvif *vif = netdev_priv(dev);
 
 	if (!vif->can_sg)
 		features &= ~NETIF_F_SG;
@@ -179,6 +179,16 @@ static u32 xenvif_fix_features(struct xenvif *vif, u32 features)
 	return features;
 }
 
+static int xenvif_set_features(struct net_device *dev, u32 features)
+{
+	if (!(features & NETIF_F_SG) && dev->mtu > ETH_DATA_LEN) {
+		netdev_info(dev, "Reducing MTU because no SG offload");
+		dev->mtu = ETH_DATA_LEN;
+	}
+
+	return 0;
+}
+
 static const struct xenvif_stat {
 	char name[ETH_GSTRING_LEN];
 	u16 offset;
@@ -237,6 +247,7 @@ static struct net_device_ops xenvif_netdev_ops = {
 	.ndo_stop	= xenvif_close,
 	.ndo_change_mtu	= xenvif_change_mtu,
 	.ndo_fix_features = xenvif_fix_features,
+	.ndo_set_features = xenvif_set_features,
 };
 
 struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
@@ -329,8 +340,6 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
 	rtnl_lock();
 	if (netif_running(vif->dev))
 		xenvif_up(vif);
-	if (!vif->can_sg && vif->dev->mtu > ETH_DATA_LEN)
-		dev_set_mtu(vif->dev, ETH_DATA_LEN);
 	netdev_update_features(vif->dev);
 	netif_carrier_on(vif->dev);
 	rtnl_unlock();



^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: xen-netback: convert to hw_features
  2011-04-19 13:17 ` Ian Campbell
@ 2011-04-19 13:30   ` Michał Mirosław
  2011-04-19 13:39     ` Ian Campbell
  0 siblings, 1 reply; 11+ messages in thread
From: Michał Mirosław @ 2011-04-19 13:30 UTC (permalink / raw)
  To: Ian Campbell; +Cc: netdev, xen-devel

On Tue, Apr 19, 2011 at 02:17:53PM +0100, Ian Campbell wrote:
> On Tue, 2011-04-19 at 12:56 +0100, Michał Mirosław wrote:
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Thanks for beating me to this! However the prototype for
> xenvif_fix_features is wrong (needs to take a net_device not a xenvif).

I'll resend v2 with this fix.

> I fixed it with the following, I also moved the !can_sg MTU clamping
> into a set_features hook (like we do with netfront). Am I right that
> this pattern copes with changes to SG via ethtool etc better? I think
> it's more future proof in any case.

This looks wrong. Even if SG is turned on, you might get big skbs which
are linearized. There is a difference in SG capability and SG offload
status and as I see it the capability is what you need to test for MTU.

> NB: I'm having some issues with my test hardware at the moment so this
> is reviewed by eye and compile tested only...
> 
> I'm also happy for this to be folded into the original with my
> "Signed-off-/Acked-by Ian Campbell <ian.campbell@citrix.com>" if that is
> preferable.

Thanks.

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH v2] net: xen-netback: convert to hw_features
  2011-04-19 11:56 [PATCH] net: xen-netback: convert to hw_features Michał Mirosław
  2011-04-19 13:17 ` Ian Campbell
@ 2011-04-19 13:35 ` Michał Mirosław
  2011-04-19 13:39   ` Michał Mirosław
  1 sibling, 1 reply; 11+ messages in thread
From: Michał Mirosław @ 2011-04-19 13:35 UTC (permalink / raw)
  To: netdev; +Cc: Ian Campbell, xen-devel

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
Acked-by: Ian Campbell <ian.campbell@citrix.com>

---
v2: fix xenvif_fix_features() arguments

 drivers/net/xen-netback/common.h    |    3 -
 drivers/net/xen-netback/interface.c |   84 ++++++----------------------------
 2 files changed, 15 insertions(+), 72 deletions(-)

diff --git a/drivers/net/xen-netback/common.h b/drivers/net/xen-netback/common.h
index 5d7bbf2..8753e6d 100644
--- a/drivers/net/xen-netback/common.h
+++ b/drivers/net/xen-netback/common.h
@@ -73,9 +73,6 @@ struct xenvif {
 	struct vm_struct *tx_comms_area;
 	struct vm_struct *rx_comms_area;
 
-	/* Flags that must not be set in dev->features */
-	u32 features_disabled;
-
 	/* Frontend feature information. */
 	u8 can_sg:1;
 	u8 gso:1;
diff --git a/drivers/net/xen-netback/interface.c b/drivers/net/xen-netback/interface.c
index de569cc..0ca86f9 100644
--- a/drivers/net/xen-netback/interface.c
+++ b/drivers/net/xen-netback/interface.c
@@ -165,69 +165,18 @@ static int xenvif_change_mtu(struct net_device *dev, int mtu)
 	return 0;
 }
 
-static void xenvif_set_features(struct xenvif *vif)
-{
-	struct net_device *dev = vif->dev;
-	u32 features = dev->features;
-
-	if (vif->can_sg)
-		features |= NETIF_F_SG;
-	if (vif->gso || vif->gso_prefix)
-		features |= NETIF_F_TSO;
-	if (vif->csum)
-		features |= NETIF_F_IP_CSUM;
-
-	features &= ~(vif->features_disabled);
-
-	if (!(features & NETIF_F_SG) && dev->mtu > ETH_DATA_LEN)
-		dev->mtu = ETH_DATA_LEN;
-
-	dev->features = features;
-}
-
-static int xenvif_set_tx_csum(struct net_device *dev, u32 data)
+static u32 xenvif_fix_features(struct net_device *dev, u32 features)
 {
 	struct xenvif *vif = netdev_priv(dev);
-	if (data) {
-		if (!vif->csum)
-			return -EOPNOTSUPP;
-		vif->features_disabled &= ~NETIF_F_IP_CSUM;
-	} else {
-		vif->features_disabled |= NETIF_F_IP_CSUM;
-	}
-
-	xenvif_set_features(vif);
-	return 0;
-}
 
-static int xenvif_set_sg(struct net_device *dev, u32 data)
-{
-	struct xenvif *vif = netdev_priv(dev);
-	if (data) {
-		if (!vif->can_sg)
-			return -EOPNOTSUPP;
-		vif->features_disabled &= ~NETIF_F_SG;
-	} else {
-		vif->features_disabled |= NETIF_F_SG;
-	}
-
-	xenvif_set_features(vif);
-	return 0;
-}
-
-static int xenvif_set_tso(struct net_device *dev, u32 data)
-{
-	struct xenvif *vif = netdev_priv(dev);
-	if (data) {
-		if (!vif->gso && !vif->gso_prefix)
-			return -EOPNOTSUPP;
-		vif->features_disabled &= ~NETIF_F_TSO;
-	} else {
-		vif->features_disabled |= NETIF_F_TSO;
-	}
+	if (!vif->can_sg)
+		features &= ~NETIF_F_SG;
+	if (!vif->gso && !vif->gso_prefix)
+		features &= ~NETIF_F_TSO;
+	if (!vif->csum)
+		features &= ~NETIF_F_IP_CSUM;
 
-	xenvif_set_features(vif);
-	return 0;
+	return features;
 }
 
 static const struct xenvif_stat {
@@ -274,12 +223,6 @@ static void xenvif_get_strings(struct net_device *dev, u32 stringset, u8 * data)
 }
 
 static struct ethtool_ops xenvif_ethtool_ops = {
-	.get_tx_csum	= ethtool_op_get_tx_csum,
-	.set_tx_csum	= xenvif_set_tx_csum,
-	.get_sg		= ethtool_op_get_sg,
-	.set_sg		= xenvif_set_sg,
-	.get_tso	= ethtool_op_get_tso,
-	.set_tso	= xenvif_set_tso,
 	.get_link	= ethtool_op_get_link,
 
 	.get_sset_count = xenvif_get_sset_count,
@@ -293,6 +236,7 @@ static struct net_device_ops xenvif_netdev_ops = {
 	.ndo_open	= xenvif_open,
 	.ndo_stop	= xenvif_close,
 	.ndo_change_mtu	= xenvif_change_mtu,
+	.ndo_fix_features = xenvif_fix_features,
 };
 
 struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
@@ -331,7 +275,8 @@ struct xenvif *xenvif_alloc(struct device *parent, domid_t domid,
 	vif->credit_timeout.expires = jiffies;
 
 	dev->netdev_ops	= &xenvif_netdev_ops;
-	xenvif_set_features(vif);
+	dev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_TSO;
+	dev->features = dev->hw_features;
 	SET_ETHTOOL_OPS(dev, &xenvif_ethtool_ops);
 
 	dev->tx_queue_len = XENVIF_QUEUE_LENGTH;
@@ -367,8 +312,6 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
 	if (vif->irq)
 		return 0;
 
-	xenvif_set_features(vif);
-
 	err = xen_netbk_map_frontend_rings(vif, tx_ring_ref, rx_ring_ref);
 	if (err < 0)
 		goto err;
@@ -384,9 +327,12 @@ int xenvif_connect(struct xenvif *vif, unsigned long tx_ring_ref,
 	xenvif_get(vif);
 
 	rtnl_lock();
-	netif_carrier_on(vif->dev);
 	if (netif_running(vif->dev))
 		xenvif_up(vif);
+	if (!vif->can_sg && vif->dev->mtu > ETH_DATA_LEN)
+		dev_set_mtu(vif->dev, ETH_DATA_LEN);
+	netdev_update_features(vif->dev);
+	netif_carrier_on(vif->dev);
 	rtnl_unlock();
 
 	return 0;
-- 
1.7.2.5


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: xen-netback: convert to hw_features
  2011-04-19 13:30   ` Michał Mirosław
@ 2011-04-19 13:39     ` Ian Campbell
  2011-04-19 13:43       ` Michał Mirosław
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2011-04-19 13:39 UTC (permalink / raw)
  To: Michał Mirosław; +Cc: netdev, xen-devel

On Tue, 2011-04-19 at 14:30 +0100, Michał Mirosław wrote:
> On Tue, Apr 19, 2011 at 02:17:53PM +0100, Ian Campbell wrote:
> > On Tue, 2011-04-19 at 12:56 +0100, Michał Mirosław wrote:
> > > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > Thanks for beating me to this! However the prototype for
> > xenvif_fix_features is wrong (needs to take a net_device not a xenvif).
> 
> I'll resend v2 with this fix.
> 
> > I fixed it with the following, I also moved the !can_sg MTU clamping
> > into a set_features hook (like we do with netfront). Am I right that
> > this pattern copes with changes to SG via ethtool etc better? I think
> > it's more future proof in any case.
> 
> This looks wrong. Even if SG is turned on, you might get big skbs which
> are linearized. There is a difference in SG capability and SG offload
> status and as I see it the capability is what you need to test for MTU.

So the existing stuff in drivers/net/xen-netfront.c is wrong too?

> > NB: I'm having some issues with my test hardware at the moment so this
> > is reviewed by eye and compile tested only...
> > 
> > I'm also happy for this to be folded into the original with my
> > "Signed-off-/Acked-by Ian Campbell <ian.campbell@citrix.com>" if that is
> > preferable.
> 
> Thanks.
> 
> Best Regards,
> Michał Mirosław



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] net: xen-netback: convert to hw_features
  2011-04-19 13:35 ` [PATCH v2] " Michał Mirosław
@ 2011-04-19 13:39   ` Michał Mirosław
  2011-04-20  7:58     ` Ian Campbell
  0 siblings, 1 reply; 11+ messages in thread
From: Michał Mirosław @ 2011-04-19 13:39 UTC (permalink / raw)
  To: David S. Miller; +Cc: netdev, Ian Campbell, xen-devel

On Tue, Apr 19, 2011 at 03:35:06PM +0200, Michał Mirosław wrote:
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

David, I was too quick with this, so please wait with this patch until Ian
acks it again.

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: xen-netback: convert to hw_features
  2011-04-19 13:39     ` Ian Campbell
@ 2011-04-19 13:43       ` Michał Mirosław
  2011-04-19 15:15         ` Ian Campbell
  0 siblings, 1 reply; 11+ messages in thread
From: Michał Mirosław @ 2011-04-19 13:43 UTC (permalink / raw)
  To: Ian Campbell; +Cc: netdev, xen-devel

On Tue, Apr 19, 2011 at 02:39:00PM +0100, Ian Campbell wrote:
> On Tue, 2011-04-19 at 14:30 +0100, Michał Mirosław wrote:
> > On Tue, Apr 19, 2011 at 02:17:53PM +0100, Ian Campbell wrote:
> > > I fixed it with the following, I also moved the !can_sg MTU clamping
> > > into a set_features hook (like we do with netfront). Am I right that
> > > this pattern copes with changes to SG via ethtool etc better? I think
> > > it's more future proof in any case.
> > This looks wrong. Even if SG is turned on, you might get big skbs which
> > are linearized. There is a difference in SG capability and SG offload
> > status and as I see it the capability is what you need to test for MTU.
> So the existing stuff in drivers/net/xen-netfront.c is wrong too?

Looks like it. But I don't really know what are the real constraints for MTU.
What I know is that SG even if turned on needs not be used (and currently
it's not e.g. if checksum offload is disabled). So MTU setting should not
depend on SG offload state but on some capability.

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: xen-netback: convert to hw_features
  2011-04-19 13:43       ` Michał Mirosław
@ 2011-04-19 15:15         ` Ian Campbell
  2011-04-19 15:25           ` Michał Mirosław
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2011-04-19 15:15 UTC (permalink / raw)
  To: Michał Mirosław; +Cc: netdev, xen-devel

On Tue, 2011-04-19 at 14:43 +0100, Michał Mirosław wrote:
> On Tue, Apr 19, 2011 at 02:39:00PM +0100, Ian Campbell wrote:
> > On Tue, 2011-04-19 at 14:30 +0100, Michał Mirosław wrote:
> > > On Tue, Apr 19, 2011 at 02:17:53PM +0100, Ian Campbell wrote:
> > > > I fixed it with the following, I also moved the !can_sg MTU clamping
> > > > into a set_features hook (like we do with netfront). Am I right that
> > > > this pattern copes with changes to SG via ethtool etc better? I think
> > > > it's more future proof in any case.
> > > This looks wrong. Even if SG is turned on, you might get big skbs which
> > > are linearized. There is a difference in SG capability and SG offload
> > > status and as I see it the capability is what you need to test for MTU.
> > So the existing stuff in drivers/net/xen-netfront.c is wrong too?
> 
> Looks like it. But I don't really know what are the real constraints for MTU.
> What I know is that SG even if turned on needs not be used (and currently
> it's not e.g. if checksum offload is disabled).

The interesting case is the opposite one, isn't it? IOW if NETIF_F_SG is
disabled but the frontend/backend agree that they have the capability to
handle >PAGE_SIZE skbs

In my experience, the normal reason for disabling the NETIF_F_SG offload
status is that the underlying capability is somehow buggy, otherwise is
there any reason to turn it off?

> So MTU setting should not depend on SG offload state but on some capability.

Ian.



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] net: xen-netback: convert to hw_features
  2011-04-19 15:15         ` Ian Campbell
@ 2011-04-19 15:25           ` Michał Mirosław
  0 siblings, 0 replies; 11+ messages in thread
From: Michał Mirosław @ 2011-04-19 15:25 UTC (permalink / raw)
  To: Ian Campbell; +Cc: netdev, xen-devel

On Tue, Apr 19, 2011 at 04:15:48PM +0100, Ian Campbell wrote:
> On Tue, 2011-04-19 at 14:43 +0100, Michał Mirosław wrote:
> > On Tue, Apr 19, 2011 at 02:39:00PM +0100, Ian Campbell wrote:
> > > On Tue, 2011-04-19 at 14:30 +0100, Michał Mirosław wrote:
> > > > On Tue, Apr 19, 2011 at 02:17:53PM +0100, Ian Campbell wrote:
> > > > > I fixed it with the following, I also moved the !can_sg MTU clamping
> > > > > into a set_features hook (like we do with netfront). Am I right that
> > > > > this pattern copes with changes to SG via ethtool etc better? I think
> > > > > it's more future proof in any case.
> > > > This looks wrong. Even if SG is turned on, you might get big skbs which
> > > > are linearized. There is a difference in SG capability and SG offload
> > > > status and as I see it the capability is what you need to test for MTU.
> > > So the existing stuff in drivers/net/xen-netfront.c is wrong too?
> > Looks like it. But I don't really know what are the real constraints for MTU.
> > What I know is that SG even if turned on needs not be used (and currently
> > it's not e.g. if checksum offload is disabled).
> The interesting case is the opposite one, isn't it? IOW if NETIF_F_SG is
> disabled but the frontend/backend agree that they have the capability to
> handle >PAGE_SIZE skbs

Then the driver might get bigger skbs but they won't ever be fragmented.

> In my experience, the normal reason for disabling the NETIF_F_SG offload
> status is that the underlying capability is somehow buggy, otherwise is
> there any reason to turn it off?

Some features depend on others to function or on some hardware/software state.
Though in most cases the reason is the one you wrote (capability also includes
what driver has implemented).

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] net: xen-netback: convert to hw_features
  2011-04-19 13:39   ` Michał Mirosław
@ 2011-04-20  7:58     ` Ian Campbell
  2011-04-20  8:31       ` David Miller
  0 siblings, 1 reply; 11+ messages in thread
From: Ian Campbell @ 2011-04-20  7:58 UTC (permalink / raw)
  To: Michał Mirosław; +Cc: David S. Miller, netdev, xen-devel

On Tue, 2011-04-19 at 14:39 +0100, Michał Mirosław wrote:
> On Tue, Apr 19, 2011 at 03:35:06PM +0200, Michał Mirosław wrote:
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
> 
> David, I was too quick with this, so please wait with this patch until Ian
> acks it again.

Thanks Michał.

Acked-by: Ian Campbell <ian.campbell@citrix.com>

Ian.


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v2] net: xen-netback: convert to hw_features
  2011-04-20  7:58     ` Ian Campbell
@ 2011-04-20  8:31       ` David Miller
  0 siblings, 0 replies; 11+ messages in thread
From: David Miller @ 2011-04-20  8:31 UTC (permalink / raw)
  To: Ian.Campbell; +Cc: mirq-linux, netdev, xen-devel

From: Ian Campbell <Ian.Campbell@eu.citrix.com>
Date: Wed, 20 Apr 2011 08:58:58 +0100

> On Tue, 2011-04-19 at 14:39 +0100, Michał Mirosław wrote:
>> On Tue, Apr 19, 2011 at 03:35:06PM +0200, Michał Mirosław wrote:
>> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
>> > Acked-by: Ian Campbell <ian.campbell@citrix.com>
>> 
>> David, I was too quick with this, so please wait with this patch until Ian
>> acks it again.
> 
> Thanks Michał.
> 
> Acked-by: Ian Campbell <ian.campbell@citrix.com>

Applied.

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2011-04-20  8:31 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-04-19 11:56 [PATCH] net: xen-netback: convert to hw_features Michał Mirosław
2011-04-19 13:17 ` Ian Campbell
2011-04-19 13:30   ` Michał Mirosław
2011-04-19 13:39     ` Ian Campbell
2011-04-19 13:43       ` Michał Mirosław
2011-04-19 15:15         ` Ian Campbell
2011-04-19 15:25           ` Michał Mirosław
2011-04-19 13:35 ` [PATCH v2] " Michał Mirosław
2011-04-19 13:39   ` Michał Mirosław
2011-04-20  7:58     ` Ian Campbell
2011-04-20  8:31       ` David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.