{painfully BISECTED} Please revert f25c80a4b2: arch/um/drivers: remove duplicate structure field initialization

[bharrosh@fs2 ~/dev/git/pub/scsi-misc] 1115$ git bisect good f25c80a4b2bf93c99820f470573626557db35202 is the first bad commit commit f25c80a4b2bf93c99820f470573626557db35202

Author: Julia Lawall <[email protected]>
Date:   Tue Jul 20 12:25:17 2010 +0000

    arch/um/drivers: remove duplicate structure field initialization
    
    There are two initializations of ndo_set_mac_address, one to a local
    function that is not used otherwise and one to a function that is defined
    elsewhere.
    
    The semantic match that finds this problem is as follows:
    (http://coccinelle.lip6.fr/)
    
    // <smpl>
    @r@
    identifier I, s, fld;
    position p0,p;
    expression E;
    @@
    
    struct I s =@p0 { ... .fld@p = E, ...};
    
    @s@
    identifier I, s, r.fld;
    position r.p0,p;
    expression E;
    @@
    
    struct I s =@p0 { ... .fld@p = E, ...};
    
    @script:python@
    p0 << r.p0;
    fld << r.fld;
    ps << s.p;
    pr << r.p;
    @@
    
    if int(ps[0].line)<int(pr[0].line) or int(ps[0].column)<int(pr[0].column):
      cocci.print_main(fld,p0)
    // </smpl>
    
    akpm:
    
    - Use the standard eth_mac_addr() in uml_net_set_mac()
    
    - Remove unneeded and racy local set_ether_mac()
    
    - Remove duplicated (and incorrect)
      uml_netdev_ops.ndo_set_mac_address initializer.
    
    Fixes 8bb95b39a16ed55226810596f92216c53329d2fe ("uml: convert network
    device to netdevice ops").
    
    [[email protected]: rework as above]
    Signed-off-by: Julia Lawall <[email protected]>
    Cc: Stephen Hemminger <[email protected]>
    Cc: "David S. Miller" <[email protected]>
    Signed-off-by: Andrew Morton <[email protected]>
    Signed-off-by: David S. Miller <[email protected]>

:040000 040000 b3ff18738a5de659b97ff6ad7ba3c23eb2736263 241657298557b14317378fbb08ad664cf6f59ad3 M arch

When this patch is applied my UML's networking works less then 50% of the times. Then 30% of the times I see ethX where 1 < X < enf. Only 20% of boots will come up with the regular eth0.

At 50% of the times when the nic fails to show up I get:
* Bringing up interface eth0: RTNETLINK answers: Cannot assign requested address Failed to bring up eth0.

When I get the funny ethX I get:
* Bringing up interface eth0: Device eth0 does not seem to be present, delaying initialization. [FAILED]

The patch Reverts cleanly on top of 2.6.36-rc5 and after Revert works perfectly as before.

If indeed this is a cleanup. It can be included (fixed) again in next Kernel. I can try any patches until this is fixed.

<RANT A HEAD CAN BE IGNORED>

It has become extremely hard to bisect a simple problem in latest Kernels!

Most mainline merges during a merge window are based on an rc1 of the previous Kernel. In the last 5 Kernels there was a 90% chance of a BAD bug in systems I use, at rc1. If a bug is found that needs bisecting. The other bugs creep up during bisect and mask out the possibility to bisect.

For instance I found the bug I see was already present in 2.6.36-rc2 and that a good point was 2.6.35. Bisecting two bad(s) quickly took me to some 2.6.35-rc1 Kernel that did not boot at all. So I was clever I decided to merge in 2.6.35 at each bisect point. Then reset and continue. But for some reason the bisect got mixed up and complained about an impossible merge common bases. So out of desperation I did a very very^ stupid thing. []$ git rebase -i v2.6.35 v2.6.36-rc1
After endless merge conflicts, at about 3700/7100 I realised that I know what is the patch that makes my UML refuse to boot. I have actually bisect a problem with it's absence 2 Kernels ago. So I was again at my bisecting manually cherry-picking the offending patch. Making sure to reset it before every git bisect XXX step. But clearly I got lucky.

In short I wish at some 2.6.XX-rc[45] of every Kernel cycle. Maintainers would rebase their next's tree of [XX+1] to a some what more stable rc. Sure re-run all the tests. They still have time for the new tree in next to be tested and verified before the next merge window. (Hell one of my bisect points took me as back as 2.6.34)

Please remind me why maintainers should not rebase their trees once committed, to the point that they don't rebase even for buggy patches that are already in next, and apply fix patches, all within the same merge window. The same is also done with merge conflicts with the rc-cycle of their own code, instead of rebasing.

So in short this is a call for, possibly, cleaner History in main Kernel. Please remind me why re-writing history is a bad thing.

</RANT A HEAD CAN BE IGNORED>

Thanks
Boaz

On 09/27/2010 09:17 AM, Boaz Harrosh wrote:
[snip /]

I had similar problems when bisecting the recent USB HID regression. Once I realized that "bisect skip" kept dropping me into a rats nest, I guessed on -rc2 and was able to proceed from there.

...

I can't comment on whether rebasing is reasonable at that level, but I was wondering if it made sense to teach git bisect to automatically cherry-pick known regression fixes. If I recall correctly, someone once suggested a history tag of the form "Fixes: <git-commit-id>". By itself, that's probably not sufficient, as I'm sure some relevant commits would get through without that tag. A separate index file containing pairs of commit-ids could supplement the main history.

If that sounds like a reasonable approach, I'm willing to take a stab at implementing it. (Unless someone smarter than me beats me to it, of course.)

Phil

Ping, no comments?

It looks like that commit is indeed very misleading. The commit message says:

"arch/um/drivers: remove duplicate structure field initialization"

but it is in fact not duplicate: there's two field initializations, but they are _different_. Looking at the patch, it has:

.ndo_set_mac_address = uml_net_set_mac,
- .ndo_set_mac_address = eth_mac_addr,

so it removes the later one, but it is not at all clear which one the compiler actually used. My guess is that it used to use the later one (the standard eth_mac_addr function), and the patch made it suddenly use the uml_net_set_mac function.

I didn't check what gcc used to do, but this:

makes me suspect that nobody else checked it either.

It's always been extremely hard, it just depends on luck how well it works. Sometimes you never see any problems (except the one you bisect, which is obviously the problem you _want_ to see), and then sometimes bisection is really painful because there are multiple independent ones you hit.

That really wasn't clever. Don't do it. It will cause untold pain. All your problems resulted from that initial thing - you were basically bisecting commits that weren't even part of the original "bad" state. After a while, git-bisect ends up hitting those commits that aren't even reachable from 'bad', and now you're screwed.

So all the other problems you had were due to that.

Now, admittedly, the thing that caused you to do this in the first place (hitting a bad kernel that you couldn't even test) is painful. And no, "git bisect skip" doesn't necessarily work all that well. But what you tried really just made things worse.

If the real thing to do, just so you know next time, is to do the "git bisect visualize", and try to pick a good point, and just select that (with "git reset --hard xyzzy").

Now, that "good" is obviously a matter of judgement, because you don't want to pick it too close to a known-bad or known-good commit (that just makes bisection not work very efficiently), and seeing that can be hard. But usually a good strategy is to pick something that looks _reasonably_ central in the gitk view, and also looks like it's not in the middle of some big upheaval (and preferably as far as reasonable from the commit you know you can't test - pick a point on a different branch before that got merged, for example)

This _really_ won't work. I mean yes, it can work, but with any kind of complex history, you're setting yourself up for more pain than it's worth. It _can_ be worthwhile, but it's absolutely the last thing you should try.

Umm. Rebasing often makes things much _worse_.

The real problem is that maintainers often pick random - and not at all stable - points for their development to begin with. They just pick some random "this is where Linus -git tree is today", and do their development on top of that. THAT is the problem - they are unaware that there's some nasty bug in that version.

It's actually made worse by rebasing. A lot of people end up rebasing on top of such a random kernel (again, just because it's the 'most recent'). It's very annoying.

Not to mention that rebasing easily results in bugs of its own, when you do hit merge conflicts or double-apply something that already got applied (and wasn't seen as a duplicate and merged away automatically, because the code had been further modified since). We had that in the DVB pull request just yesterday.

Rebasing doesn't result in cleaner history. It just results in _incorrect_ history that looks simpler.

To get cleaner history, people should try to keep their tree clean. Not add random patches to random branches, and not start random branches at random points in time that aren't necessarily stable.

Linus

I checked! gcc uses the second initialiser.

uml_net_set_mac() is:

static int uml_net_set_mac(struct net_device *dev, void *addr)

{
	struct uml_net_private *lp = netdev_priv(dev);
	struct sockaddr *hwaddr = addr;

	spin_lock_irq(&lp->lock);
	eth_mac_addr(dev, hwaddr->sa_data);
	spin_unlock_irq(&lp->lock);

	return 0;
}

And I misread that, assuming that it's just a wrapper around eth_mac_addr(). Only it isn't, because it passes eth_mac_addr() the MAC address's address directly (with ->sa_data). But eth_mac_addr() expects a `struct sockaddr *'.

And for some wtf reason, eth_mac_addr() passes that struct sockaddr *' in a void *', thus cunningly hiding the bug.

Yeah, please revert it.

------------------------------------------------------------------------------
Start uncovering the many advantages of virtual appliances and start using them to simplify application deployment and accelerate your shift to cloud computing.
http://p.sf.net/sfu/novell-sfdev2dev
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

From: Andrew Morton <[email protected]>
Date: Tue, 28 Sep 2010 13:47:21 -0700

I'll take care of this, thanks everyone.

Well, the alternative would be to just fix uml_net_set_mac(). The code before the patch was clearly crap too, so reverting may make it work, but gets us back to a stupid situation with two different initializers for one field.

So perhaps something like the attached patch would make it work, and have the locking in place that apparently people think it should have?

Boaz?

Linus

 arch/um/drivers/net_kern.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

index 2ab233b..e44fd87 100644
@@ -257,14 +257,14 @@ static void uml_net_tx_timeout(struct net_device *dev)
 
 static int uml_net_set_mac(struct net_device *dev, void *addr)
 {
+	int ret;
 	struct uml_net_private *lp = netdev_priv(dev);
-	struct sockaddr *hwaddr = addr;
 
 	spin_lock_irq(&lp->lock);
-	eth_mac_addr(dev, hwaddr->sa_data);
+	ret = eth_mac_addr(dev, addr);
 	spin_unlock_irq(&lp->lock);
 
-	return 0;
+	return ret;
 }
 
 static int uml_net_change_mtu(struct net_device *dev, int new_mtu)

From: Linus Torvalds <[email protected]>
Date: Tue, 28 Sep 2010 13:57:04 -0700

I'm fine with this if it works:

And you can add my sign-off to it if somebody tests it.

It does look like the obvious fix. The "wrapper" is simply totally wrong as-is, and passes the wrong thing entirely to eth_mac_addr() afaik.

Linus

uml_net_set_mac() was broken and luckily it was never used, before. What it was trying to do is spin_lock before memcopy the mac address. Linus attempted to fix it in assumption that someone decided the lock was needed. But since it was never ever used at all, and was just dead code, I think we can assume that it is not needed, after all.

On the other hand patch [f25c80a4] was trying to use eth_mac_addr() in eth_configure(), *which was the real fallout*. Because of state checks done inside eth_mac_addr() the address was never set. I have not reintroduced the memcpy wrapper, but I've put a comment for future cats.

The code now is back to exactly as it was before [f25c80a4]. With the cleanup applied. If the spin_lock is indeed needed then a contender should supply a test case that fails, then fix it with the proper locking, as a separate unrelated patch.

---
 arch/um/drivers/net_kern.c |   17 +++--------------
 1 files changed, 3 insertions(+), 14 deletions(-)

index 2ab233b..47d0c37 100644
@@ -255,18 +255,6 @@ static void uml_net_tx_timeout(struct net_device *dev)
 	netif_wake_queue(dev);
 }
 
-static int uml_net_set_mac(struct net_device *dev, void *addr)
-{
-	struct uml_net_private *lp = netdev_priv(dev);
-	struct sockaddr *hwaddr = addr;
-
-	spin_lock_irq(&lp->lock);
-	eth_mac_addr(dev, hwaddr->sa_data);
-	spin_unlock_irq(&lp->lock);
-
-	return 0;
-}
-
 static int uml_net_change_mtu(struct net_device *dev, int new_mtu)
 {
 	dev->mtu = new_mtu;

@@ -373,7 +361,7 @@ static const struct net_device_ops uml_netdev_ops = {
 	.ndo_start_xmit 	= uml_net_start_xmit,
 	.ndo_set_multicast_list = uml_net_set_multicast_list,
 	.ndo_tx_timeout 	= uml_net_tx_timeout,
-	.ndo_set_mac_address	= uml_net_set_mac,
+	.ndo_set_mac_address	= eth_mac_addr,
 	.ndo_change_mtu 	= uml_net_change_mtu,
 	.ndo_validate_addr	= eth_validate_addr,
 };

@@ -472,7 +460,8 @@ static void eth_configure(int n, void *init, char *mac,
 	    ((*transport->user->init)(&lp->user, dev) != 0))
 		goto out_unregister;
 
-	eth_mac_addr(dev, device->mac);
+	/* don't use eth_mac_addr, it will not work here */
+	memcpy(dev->dev_addr, device->mac, ETH_ALEN);
 	dev->mtu = transport->user->mtu;
 	dev->netdev_ops = &uml_netdev_ops;
 	dev->ethtool_ops = &uml_net_ethtool_ops;

1.7.2.3

Ahh. Ok, this looks fine. David, please use this instead of my broken one that didn't do the whole thing.

Linus

From: Linus Torvalds <[email protected]>
Date: Wed, 29 Sep 2010 08:05:04 -0700

Oops, I just noticed this, ok will do.

OK I get the picture. We should attempt a proper fixing. Well since I get to test it, then I get to fix it as well, right ;-)?

With this I'm now back to my usual: boots fine but half of the times not halting. But I think that's a Fedora12 problem with interaction with uml. (Which I do not have time to investigate)

Thanks, have a good day
Boaz

So that "half of the time not halting" happened with the revert too? If so, we should just do the fix. Thanks for testing.

Linus

From: Linus Torvalds <[email protected]>
Date: Wed, 29 Sep 2010 08:01:07 -0700

Linus, I've queued up your fix into my tree.

C99 6.7.8p19:

The initialization shall occur in initializer list order, each
initializer provided for a particular subobject overriding any previously listed initializer for the same subobject[*]; all subobjects that are not initialized explicitly shall be initialized implicitly the same as objects that have static storage duration.

[*] Any initializer for subobject which is overridden and so not used to initialize that subobject might not be evaluated at all.

IOW, it _must_ use the last one in such cases.

As for the driver, I smell an interface change (in eth_mac_addr() arguments) that has been missed... FWIW, grep through the tree shows one more instance of eth_mac_addr() called with such argument and it's also in net_kern.c; there we simply want memcpy() instead, since device is definitely not running at that point and we'd done the validity checks earlier.

Not sure if we need lp->lock around that eth_mac_addr() call - not familiar with the driver in question. If we don't, we should switch to eth_mac_addr for the method, indeed...

FWIW, after looking at that code... I don't think we need lp->lock there, but I really wonder if we need to update lp->mac as well, or, perhaps simply remove it completely. Who maintains these drivers? It's not just net_kern/net_user; there's a bunch of subdrivers for that sucker...

------------------------------------------------------------------------------
Start uncovering the many advantages of virtual appliances and start using them to simplify application deployment and accelerate your shift to cloud computing.
http://p.sf.net/sfu/novell-sfdev2dev
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

From: Al Viro <[email protected]>
Date: Tue, 28 Sep 2010 22:24:17 +0100

Your guess is as good as mine, which probably means there is no active maintainer right now.

Oh, joy... I'm regulary using uml, with some of these drivers used from time to time, but I've no fscking clue on e.g. VDE - even on the "what the hell it is" level ;-/

------------------------------------------------------------------------------
Start uncovering the many advantages of virtual appliances and start using them to simplify application deployment and accelerate your shift to cloud computing.
http://p.sf.net/sfu/novell-sfdev2dev
_______________________________________________
User-mode-linux-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

VDE is Virtual Distributed Ethernet.
http://wiki.virtualsquare.org/wiki/index.php/Introduction#Virtual_Square_Networking

I am the main author of VDE.
Was it just an example or there is a bug on the VDE support for user-mode Linux?

renzo

I understand what you want to say, but I don't see how you define stable points. Maybe you mean that a stable point is a non-rc release of Linux. This of course sounds like a good idea when you start a complete new development without any dependencies, but think about following events (ordered by time of event):

* v2.6.37 was released
* foo starts to code the feature bar
* foo asks a subtree maintainer to merge his stuff (subtree maintainer doesn't
merge it because 2.6.38-rc1 wasn't released yet)
* v2.6.38-rc1 was released
* subtree maintainer merges the commit because we need more bar in Linux
* v2.6.38 was released
* subtree maintainer asks you to merge his tree with the cool bar feature
* foo wants to extent the bar feature to superbar and get it in 2.6.40
* v2.6.39-rc1 gets released with the cool bar feature
* v2.6.39 gets released with the final and bugfixed bar feature
* (your merge window - time were foo will not be able to get new patches to the
subtree maintainer for v2.6.40-rc1)
* v2.6.40-rc1 gets released

Even if this is a relative small example and the usual problem is more complex - what starting point should be used for his superbar feature? My obvious answer would be 2.6.39-rc1 (but is it really a stable starting point?)

Kind regards,
Emil Langrock