compat: Unlock qdisc root lock in case of failure when dumping stats.

[heistp]

Linux versions prior to 4.8 have a bug where the root qdisc lock is not released in tc_fill_qdisc (in sch_api.c) in some failure cases. This can cause a lockup when dumping cake stats from multiple qdiscs, for example. Fix this by explicitly unlocking the qdisc lock in the failure case.

[tohojo]

Do you have a reference to the upstream fix for this? And is it just a simple bug, because then we should get it fixed in the long-term releases (which will probably also make the compat fix more complicated, as an accidental double-unlock is no good)...

[tohojo]

Also, we already have cake_maybe_unlock() in compat, so maybe use that?

[heistp]

Right, unfortunately I'm not likely to submit an upstream fix myself soon for this due to lack of time. I totally understand if you wouldn't want to take it in to cake due to the possibility of a double unlock in the future if it's ever fixed. So if not, you can close it and if I ever do get to an upstream fix I'll let you know.

Meanwhile, without this change, cake fails when printing stats with more than a few qdiscs on 3.16.7. It's possible that some intervening change increased the default netlink buffer size to avoid the issue in kernels between 3.16.7 and 4.8, but I don't have a setup to test that.

[tohojo]

Pete Heist notifications@github.com writes:

Right, unfortunately I'm not likely to submit an upstream fix myself soon for this due to lack of time. I totally understand if you wouldn't want to take it in to cake due to the possibility of a double unlock in the future if it's ever fixed. So if not, you can close it and if I ever do get to an upstream fix I'll let you know.

That's fine, I can submit the patch. Just want to make sure this compat fix doesn't break later :)

Could you test if it works to condition the check on spin_is_locked()?

Something like:

if (spin_is_locked(qdisc_root_sleeping_lock(q))) sch_tree_unlock(q);

inside the ifdefs

(and drop the cake_maybe_unlock usage).

I think that should be safe even when upstream is fixed...

-Toke

[heistp]

Cool, that check works. Do you think it's safe without the ifdef?

I doubt that's encouraged as a general technique, but for a workaround for old kernels that only occurs in an error condition this seems like a reasonable solution. :)

[tohojo]

Pete Heist notifications@github.com writes:

Cool, that check works. Do you think it's safe without the ifdef?

Hmm, probably, but it just occurred to me that it won't actually help. If this is fixed upstream, the additional unlock won't be the one in inside cake, but in the upstream code.

So bugger, we need to do the ifdef dance :(

-Toke

[heistp]

Rats, that's right, because cake_dump_stats is called by tc_fill_qdisc. So it uses cake_maybe_unlock for now, and presumably that will need to change if there's an upstream fix. What a clean commit history I've left in my wake. :)

[tohojo]

Pete Heist notifications@github.com writes:

Rats, that's right, because cake_dump_stats is called by tc_fill_qdisc. So it uses cake_maybe_unlock for now, and presumably that will need to change if there's an upstream fix. What a clean commit history I've left in my wake. :)

Hehe, yes, a rebase+squash would be nice :)

-Toke

[heistp]

Woops, cake_maybe_unlock is defined to unlock for kernel versions >= 4.8.0, which is actually the opposite of what I want. Presumably that's working as it should for cake_dump_class_stats, so I'll just return the thing to an ifdef again, rebase and squash.

As they say, we Americans can always be counted on to do the right thing, after we've exhausted all other possibilities. :)

[tohojo]

Pete Heist notifications@github.com writes:

Woops, cake_maybe_unlock is defined to unlock for kernel versions >= 4.8.0, which is actually the opposite of what I want. Presumably that's working as it should for cake_dump_class_stats, so I'll just return the thing to an ifdef again, rebase and squash.

Ah, my bad; just saw that the version number was the same, didn't bother to check which way the < sign was pointing ;)

As they say, we Americans can always be counted on to do the right thing, after we've exhausted all other possibilities. :)

Haha, indeed :P

-Toke