diff mbox

net: xen-netback: convert to hw_features

Message ID 1303219073.5997.191.camel@zakaz.uk.xensource.com
State Superseded, archived
Delegated to: David Miller
Headers show

Commit Message

Ian Campbell April 19, 2011, 1:17 p.m. UTC 
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>



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Michał Mirosław April 19, 2011, 1:30 p.m. UTC | #1 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Campbell April 19, 2011, 1:39 p.m. UTC | #2 
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


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michał Mirosław April 19, 2011, 1:43 p.m. UTC | #3 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ian Campbell April 19, 2011, 3:15 p.m. UTC | #4 
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.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Michał Mirosław April 19, 2011, 3:25 p.m. UTC | #5 
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
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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();