If I change the "options" on any of the xn network interfaces with ifconfig then perform a xenmotion, the options are reset to default after the xenmotion. This is on a server with XenServer 6.0.2 running fairly recent patches. I plan to re-test on XenServer 6.2 but we don't have a pool setup yet. I'd test on FreeBSD 10 but xen-tools needs to be built with gcc instead of clang and I haven't put much effort into that yet. How-To-Repeat: # ifconfig xn0 xn0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500 options=503<RXCSUM,TXCSUM,TSO4,LRO> # ifconfig xn0 -rxcsum # ifconfig xn0 -lro # ifconfig xn0 xn0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500 options=102<TXCSUM,TSO4> (perform xenmotion) # ifconfig xn0 xn0: flags=8843<UP,BROADCAST,RUNNING,SIMPLEX,MULTICAST> metric 0 mtu 1500 options=503<RXCSUM,TXCSUM,TSO4,LRO>
Hello, The following patch seems to solve the problem, but I'm not that much familiar with netfront or net drivers in general (also netfront code is not that great, which makes it even harder to understand IMHO). --- diff --git a/sys/dev/xen/netfront/netfront.c b/sys/dev/xen/netfront/netfront.c index f9c72e6..99d3cf9 100644 --- a/sys/dev/xen/netfront/netfront.c +++ b/sys/dev/xen/netfront/netfront.c @@ -2022,15 +2022,31 @@ static int xn_configure_features(struct netfront_info *np) { int err; + bool resume; err = 0; + resume = !!np->xn_ifp->if_capenable; + + if (!resume || ((np->xn_ifp->if_capenable & np->xn_ifp->if_capabilities) + != np->xn_ifp->if_capenable)) { + /* + * Check if current enabled capabilities are available, + * if not switch to default capabilities. + */ #if __FreeBSD_version >= 700000 - if ((np->xn_ifp->if_capenable & IFCAP_LRO) != 0) - tcp_lro_free(&np->xn_lro); + if ((np->xn_ifp->if_capenable & IFCAP_LRO) != 0) + tcp_lro_free(&np->xn_lro); #endif - np->xn_ifp->if_capenable = - np->xn_ifp->if_capabilities & ~(IFCAP_LRO|IFCAP_TSO4); - np->xn_ifp->if_hwassist &= ~CSUM_TSO; + np->xn_ifp->if_capenable = + np->xn_ifp->if_capabilities & ~(IFCAP_LRO|IFCAP_TSO4); + np->xn_ifp->if_hwassist &= ~CSUM_TSO; + } else { + /* + * What we have currently enabled is supported by the + * new host, no need to change anything. + */ + return 0; + } #if __FreeBSD_version >= 700000 if (xn_enable_lro && (np->xn_ifp->if_capabilities & IFCAP_LRO) != 0) { err = tcp_lro_init(&np->xn_lro); --
On 10/28/2013 12:13, Roger Pau Monné wrote: > Hello, > > The following patch seems to solve the problem, but I'm not that much > familiar with netfront or net drivers in general (also netfront code is > not that great, which makes it even harder to understand IMHO). > > --- (trimmed) So far, it almost works as I expect. It seems to work as long as at least one option is left (example: options=400<LRO>) but if I have none, they all come back on migrate. Thanks.
On 28/10/13 22:28, Adam McDougall wrote: > So far, it almost works as I expect. It seems to work as long as at > least one option is left (example: options=400<LRO>) but if I have none, > they all come back on migrate. Thanks. Yes, the "resume" condition is not triggered if there isn't any feature enabled. I've updated the patch, now it should work as expected: --- diff --git a/sys/dev/xen/netfront/netfront.c b/sys/dev/xen/netfront/netfront.c index f9c72e6..52d8c11 100644 --- a/sys/dev/xen/netfront/netfront.c +++ b/sys/dev/xen/netfront/netfront.c @@ -287,6 +287,8 @@ struct netfront_info { multicall_entry_t rx_mcl[NET_RX_RING_SIZE+1]; mmu_update_t rx_mmu[NET_RX_RING_SIZE]; struct ifmedia sc_media; + + bool xn_resume; }; #define rx_mbufs xn_cdata.xn_rx_chain @@ -502,6 +504,7 @@ netfront_resume(device_t dev) { struct netfront_info *info = device_get_softc(dev); + info->xn_resume = true; netif_disconnect_backend(info); return (0); } @@ -2024,13 +2027,28 @@ xn_configure_features(struct netfront_info *np) int err; err = 0; + + if (!np->xn_resume || + ((np->xn_ifp->if_capenable & np->xn_ifp->if_capabilities) + != np->xn_ifp->if_capenable)) { + /* + * Check if current enabled capabilities are available, + * if not switch to default capabilities. + */ #if __FreeBSD_version >= 700000 - if ((np->xn_ifp->if_capenable & IFCAP_LRO) != 0) - tcp_lro_free(&np->xn_lro); + if ((np->xn_ifp->if_capenable & IFCAP_LRO) != 0) + tcp_lro_free(&np->xn_lro); #endif - np->xn_ifp->if_capenable = - np->xn_ifp->if_capabilities & ~(IFCAP_LRO|IFCAP_TSO4); - np->xn_ifp->if_hwassist &= ~CSUM_TSO; + np->xn_ifp->if_capenable = + np->xn_ifp->if_capabilities & ~(IFCAP_LRO|IFCAP_TSO4); + np->xn_ifp->if_hwassist &= ~CSUM_TSO; + } else { + /* + * What we have currently enabled is supported by the + * new host, no need to change anything. + */ + return 0; + } #if __FreeBSD_version >= 700000 if (xn_enable_lro && (np->xn_ifp->if_capabilities & IFCAP_LRO) != 0) { err = tcp_lro_init(&np->xn_lro);
Thanks, this works! On Tue, Oct 29, 2013 at 10:42:55AM +0100, Roger Pau Monné wrote: On 28/10/13 22:28, Adam McDougall wrote: > So far, it almost works as I expect. It seems to work as long as at > least one option is left (example: options=400<LRO>) but if I have none, > they all come back on migrate. Thanks. Yes, the "resume" condition is not triggered if there isn't any feature enabled. I've updated the patch, now it should work as expected: --- diff --git a/sys/dev/xen/netfront/netfront.c b/sys/dev/xen/netfront/netfront.c index f9c72e6..52d8c11 100644 --- a/sys/dev/xen/netfront/netfront.c +++ b/sys/dev/xen/netfront/netfront.c @@ -287,6 +287,8 @@ struct netfront_info { multicall_entry_t rx_mcl[NET_RX_RING_SIZE+1]; mmu_update_t rx_mmu[NET_RX_RING_SIZE]; struct ifmedia sc_media; + + bool xn_resume; }; #define rx_mbufs xn_cdata.xn_rx_chain @@ -502,6 +504,7 @@ netfront_resume(device_t dev) { struct netfront_info *info = device_get_softc(dev); + info->xn_resume = true; netif_disconnect_backend(info); return (0); } @@ -2024,13 +2027,28 @@ xn_configure_features(struct netfront_info *np) int err; err = 0; + + if (!np->xn_resume || + ((np->xn_ifp->if_capenable & np->xn_ifp->if_capabilities) + != np->xn_ifp->if_capenable)) { + /* + * Check if current enabled capabilities are available, + * if not switch to default capabilities. + */ #if __FreeBSD_version >= 700000 - if ((np->xn_ifp->if_capenable & IFCAP_LRO) != 0) - tcp_lro_free(&np->xn_lro); + if ((np->xn_ifp->if_capenable & IFCAP_LRO) != 0) + tcp_lro_free(&np->xn_lro); #endif - np->xn_ifp->if_capenable = - np->xn_ifp->if_capabilities & ~(IFCAP_LRO|IFCAP_TSO4); - np->xn_ifp->if_hwassist &= ~CSUM_TSO; + np->xn_ifp->if_capenable = + np->xn_ifp->if_capabilities & ~(IFCAP_LRO|IFCAP_TSO4); + np->xn_ifp->if_hwassist &= ~CSUM_TSO; + } else { + /* + * What we have currently enabled is supported by the + * new host, no need to change anything. + */ + return 0; + } #if __FreeBSD_version >= 700000 if (xn_enable_lro && (np->xn_ifp->if_capabilities & IFCAP_LRO) != 0) { err = tcp_lro_init(&np->xn_lro);
Responsible Changed From-To: freebsd-bugs->freebsd-xen Over to maintainer(s).
Roger, could you take a look at this please?
Today I manually cut and pasted this patch into 10.1-STABLE r283511 sources and it still compiled and worked.
Not sure why I didn't commit this, AFAICT the patch looks OK. Unless someone has any comments on it I'm going to commit it later today. The MFC will have to wait a couple of weeks, since I will be on vacation until the 20th of July.
Created attachment 158280 [details] Preserve xn options across migrations
Now I remember the problem with the current patch, if the set of available options is different between the hosts we will fall back to enabling everything. The attached patch should solve this, trying to preserve as many options as possible when migrating. I've tested it and it seems to work fine, but currently I don't have two hosts that expose different xn features. I'm running a tinderbox now to make sure I don't break anything else, once that finishes I will commit it.
A commit references this bug: Author: royger Date: Fri Jul 3 12:09:06 UTC 2015 New revision: 285089 URL: https://svnweb.freebsd.org/changeset/base/285089 Log: netfront: preserve configuration across migrations Try to preserve the xn configuration when migrating. This is not always possible since the backend might not have the same set of options available, in which case we will try to preserve as many as possible. MFC after: 2 weeks PR: 183139 Reported by: mcdouga9@egr.msu.edu Sponsored by: Citrix Systems R&D Changes: head/sys/dev/xen/netfront/netfront.c
This got merged to 10-stable as r285737. Thanks! Closing