locking: Add helpers for rwsem and osq locks #69

imran-kn · 2024-02-02T10:47:47Z

These changes add helpers for rwsem and osq locks.
Also change existing helpers when needed

imran-kn · 2024-02-15T09:43:17Z

I am also working on spinlock helpers and fall back mechanism for lock scanners (including the ones that have already been merged)when frame['var'] method fails to locate the lock. But this will take some more time .In the mean time , these helpers which have been ready for some time can be reviewed and merged . Once the spinlock helpers and other enhancements are ready I will create another PR for those

brenns10

I only got part of the way through this, I mostly focused on the osq helpers, and will need to read more about their application to the mutex and rwsem code after. My comments here are almost entirely nits or small suggestions, it's looking great so far, thank you!

drgn_tools/lock.py

drgn_tools/locking.py

brenns10

A few more small comments, the helpers are looking good. Nice job detecting the rwsem differences via a commit in the same release, that's a great way to do it!

brenns10 · 2024-02-23T22:22:33Z

drgn_tools/locking.py

+        print("rwsem is free.")
+        return NULL(prog, "struct task_struct *")
+
+    if is_rwsem_writer_owned(rwsem):
+        if (rwsem.owner.type_.type_name() != "atomic_long_t") and (
+            rwsem.owner.value_() & _RWSEM_ANONYMOUSLY_OWNED
+        ):
+            print("rwsem is owned by anonymous writer")


I think it would make sense to remove both of the print statements, since this seems to be generic helper code (not corelens module code which is supposed to be printing a report).

The idea behind keeping these prints was that when needed we can check owner of any standalone or embedded rwsem , from drgn shell or cli, using this helper and if the owner can't be obtained because of either of the above reasons, we get to know the reason. I have found this helpful when working side by side with crash and drgn , where sometime I locate a rwsem in crash and want to know who owns it. This is why I intend to keep these. Please let me know your thoughts.

I see. I agree that this makes most sense for doing interactive debugging, if you directly call the function, then getting the message printed on the command line makes sense.

If this is used as part of a more complex helper, then it may be a bit confusing to see a message printed out which is not part of the helper. For example, suppose we had a helper that printed a table of rwsems. Then the printout wouldn't be very helpful, because this line gets printed in between each row of the table.

What if we could make a slightly more complex return type, that expresses everything. For example:

import collections import enum class RwsemStateCode(enum.Enum): UNLOCKED = "unlocked" READER_OWNED = "reader owned" WRITER_OWNED = "writer owned" WRITER_ANONYMOUSLY_OWNED = "writer anonymously owned" class RwsemState(collections.namedtuple): state: RwsemStateCode owner: drgn.Object """A ``struct rwsem_waiter *``, non-NULL for WRITER_OWNED"""

Then we could have a single helper that can return all the necessary details. With that, you would be able to see on the command line that the rwsem was anonymously owned, but it wouldn't interfere with other helpers using that helper. In fact, it would make that info available to those helpers as well!

This is just an example, but I think something inspired by this might be good. I'm not opposed to having the print statements in drgn-tools, but I'm not confident they'll be acceptable in drgn upstream (I'm starting to look at that PR as well).

I agree @brenns10 and have modified get_rwsem_owner now. Interactive users should use get_rwsem_info but if they are just interested in owner info they can still use get_rwsem_owner as per its description. Let me know how it looks now. And thanks for looking into the upstream PR as well :)

I checked cd9c0e2 and that's great! Just what I was thinking and I think it makes the code a bit clearer.

Just double checking, is this ready for review and merge? I will take a deep look if so.

@brenns10 yes its ready for review and merge now

drgn_tools/locking.py

drgn_tools/util.py

imran-kn · 2024-03-14T03:57:45Z

A few more small comments, the helpers are looking good. Nice job detecting the rwsem differences via a commit in the same release, that's a great way to do it!

Thank @brenns10 ! I have addressed review comments received so far. I have left one comment about prints unresolved. Could you please have a look and let me know if the reasoning sounds good to you.

brenns10 · 2024-03-19T22:43:23Z

Imran, thanks! I've given a suggestion for the print statements. Let me know how you feel and I'll do a final round of review.

brenns10

This looks really excellent, thank you for these!

brenns10 · 2024-04-02T22:15:30Z

Oh, unfortunately there were some changes to the drgn_tools/lock.py file which need some attention. There was a pid and time option added to each of the scanning functions to filter the output. I think you'll need to merge or rebase on the latest main branch. Sorry for that.

…state. Signed-off-by: Imran Khan <[email protected]>

Signed-off-by: Imran Khan <[email protected]>

…lock(s). We have ran into situtaions where optimistic spin on sleeping lock by multiple CPUs causes high sys load, which further leads to other issues. This helper allows to check for such spinners. Some examples from a vmcore where system ran into fork-bomb: >>> scan_osq_node(prog) There are spinners on one or more osq_lock CPU(s): [21, 17, 59, 67, 6, 0, 28, 53, 4, 41, 55, 38, 63, 23, 27, 19, 36] are spinning on same osq_lock CPU(s): [42, 56, 35, 18, 22, 3, 1] are spinning on same osq_lock CPU(s): [70, 44, 9] are spinning on same osq_lock CPU(s): [16, 26] are spinning on same osq_lock >>> scan_osq_node(prog,1) There are spinners on one or more osq_lock CPU(s): [21, 17, 59, 67, 6, 0, 28, 53, 4, 41, 55, 38, 63, 23, 27, 19, 36] are spinning on same osq_lock CPU: 21 has been spinning for 1.2 seconds. CPU: 17 has been spinning for 1.3 seconds. CPU: 59 has been spinning for 1.3 seconds. CPU: 67 has been spinning for 1.5 seconds. ....................................... ....................................... CPU: 19 has been spinning for 3.004 seconds. CPU: 36 has been spinning for 3.1 seconds. ...................................... >>> scan_osq_node(prog,2) There are spinners on one or more osq_lock CPU(s): [21, 17, 59, 67, 6, 0, 28, 53, 4, 41, 55, 38, 63, 23, 27, 19, 36] are spinning on same osq_lock CPU: 21 has been spinning for 0.0 seconds. Call stack at cpu: 21 PID: 64691 TASK: ffff8fac01282f80 [R] CPU: 21! COMMAND: "bash" !# 0 [ffffa447d645fb58] node_cpu at 0xffffffff960f3748 kernel/locking/osq_lock.c:27:0 !# 1 [ffffa447d645fb58] osq_lock at 0xffffffff960f3748 kernel/locking/osq_lock.c:143:0 # 2 [ffffa447d645fb68] rwsem_optimistic_spin at 0xffffffff96899cc0 kernel/locking/rwsem-xadd.c:451:0 # 3 [ffffa447d645fb68] __rwsem_down_write_failed_common at 0xffffffff96899cc0 kernel/locking/rwsem-xadd.c:529:0 # 4 [ffffa447d645fb68] rwsem_down_write_failed at 0xffffffff96899cc0 kernel/locking/rwsem-xadd.c:617:0 # 5 [ffffa447d645fbf8] call_rwsem_down_write_failed at 0xffffffff96889537 arch/x86/lib/rwsem.S:117:0 # 6 [ffffa447d645fc40] __down_write at 0xffffffff96898bcd arch/x86/include/asm/rwsem.h:142:0 # 7 [ffffa447d645fc40] down_write at 0xffffffff96898bcd kernel/locking/rwsem.c:72:0 # 8 [ffffa447d645fc58] lock_anon_vma_root at 0xffffffff9623ae3c mm/rmap.c:239:0 # 9 [ffffa447d645fc58] unlink_anon_vmas at 0xffffffff9623ae3c mm/rmap.c:390:0 #10 [ffffa447d645fca8] free_pgtables at 0xffffffff96225231 mm/memory.c:406:0 #11 [ffffa447d645fce8] exit_mmap at 0xffffffff96230f48 mm/mmap.c:3212:0 #12 [ffffa447d645fd98] __mmput at 0xffffffff96097ab2 kernel/fork.c:969:0 #13 [ffffa447d645fd98] mmput at 0xffffffff96097ab2 kernel/fork.c:990:0 #14 [ffffa447d645fdb8] copy_process at 0xffffffff96098801 kernel/fork.c:2058:0 #15 [ffffa447d645fea0] copy_process at 0xffffffff96099db9 kernel/fork.c:1653:0 #16 [ffffa447d645fea0] _do_fork at 0xffffffff96099db9 kernel/fork.c:2153:0 #17 [ffffa447d645ff20] SYSC_clone at 0xffffffff9609a149 kernel/fork.c:2265:0 #18 [ffffa447d645ff20] SyS_clone at 0xffffffff9609a149 kernel/fork.c:2259:0 #19 [ffffa447d645ff30] do_syscall_64 at 0xffffffff96003ca9 arch/x86/entry/common.c:298:0 #20 [ffffa447d645ff58] entry_SYSCALL_64 at 0xffffffff96a001b1 arch/x86/entry/entry_64.S:238:0 ....................................... ....................................... CPU: 36 has been spinning for 3.1 seconds. Call stack at cpu: 36 PID: 16329 TASK: ffff8f85db930000 [R] CPU: 36! COMMAND: bash 0 [ffffa4481243fb68] rwsem_try_write_lock_unqueued at 0xffffffff96899d00 kernel/locking/rwsem-xadd.c:364:0 1 [ffffa4481243fb68] rwsem_optimistic_spin at 0xffffffff96899d00 kernel/locking/rwsem-xadd.c:465:0 2 [ffffa4481243fb68] __rwsem_down_write_failed_common at 0xffffffff96899d00 kernel/locking/rwsem-xadd.c:529:0 3 [ffffa4481243fb68] rwsem_down_write_failed at 0xffffffff96899d00 kernel/locking/rwsem-xadd.c:617:0 # 4 [ffffa4481243fbf8] call_rwsem_down_write_failed at 0xffffffff96889537 arch/x86/lib/rwsem.S:117:0 # 5 [ffffa4481243fc40] __down_write at 0xffffffff96898bcd arch/x86/include/asm/rwsem.h:142:0 # 6 [ffffa4481243fc40] down_write at 0xffffffff96898bcd kernel/locking/rwsem.c:72:0 # 7 [ffffa4481243fc58] lock_anon_vma_root at 0xffffffff9623ae3c mm/rmap.c:239:0 # 8 [ffffa4481243fc58] unlink_anon_vmas at 0xffffffff9623ae3c mm/rmap.c:390:0 # 9 [ffffa4481243fca8] free_pgtables at 0xffffffff96225231 mm/memory.c:406:0 #10 [ffffa4481243fce8] exit_mmap at 0xffffffff96230f48 mm/mmap.c:3212:0 #11 [ffffa4481243fd98] __mmput at 0xffffffff96097ab2 kernel/fork.c:969:0 #12 [ffffa4481243fd98] mmput at 0xffffffff96097ab2 kernel/fork.c:990:0 #13 [ffffa4481243fdb8] copy_process at 0xffffffff96098801 kernel/fork.c:2058:0 #14 [ffffa4481243fea0] copy_process at 0xffffffff96099db9 kernel/fork.c:1653:0 #15 [ffffa4481243fea0] _do_fork at 0xffffffff96099db9 kernel/fork.c:2153:0 #16 [ffffa4481243ff20] SYSC_clone at 0xffffffff9609a149 kernel/fork.c:2265:0 #17 [ffffa4481243ff20] SyS_clone at 0xffffffff9609a149 kernel/fork.c:2259:0 #18 [ffffa4481243ff30] do_syscall_64 at 0xffffffff96003ca9 arch/x86/entry/common.c:298:0 #19 [ffffa4481243ff58] entry_SYSCALL_64 at 0xffffffff96a001b1 arch/x86/entry/entry_64.S:238:0 Signed-off-by: Imran Khan <[email protected]> Conflicts: drgn_tools/lock.py

Signed-off-by: Imran Khan <[email protected]>

Also include waiters wait time, last run cpu and its state. Signed-off-by: Imran Khan <[email protected]>

Signed-off-by: Imran Khan <[email protected]> Conflicts: drgn_tools/lock.py

Also add some missing fixes into rwsem helpers. Signed-off-by: Imran Khan <[email protected]> Conflicts: drgn_tools/lock.py

Signed-off-by: Imran Khan <[email protected]>

Modify get_rwsem_owner to return type of owner as well and use this info in users of get_rwsem_owner. This would also mean that interactive users of get_rwsem_owner would need to further decode its returned value, as desribed for the return type of this function. This should not be a problem and also its expected that interactive users will use get_rwsem_info and that would do the decoding part as well. Signed-off-by: Imran Khan <[email protected]>

imran-kn · 2024-04-03T02:23:05Z

Oh, unfortunately there were some changes to the drgn_tools/lock.py file which need some attention. There was a pid and time option added to each of the scanning functions to filter the output. I think you'll need to merge or rebase on the latest main branch. Sorry for that.

No probs @brenns10 . I have rebased wrt to main branch and no conflicts now.

brenns10

I want to merge this as-is. However, there's a follow-up item we'll need to deal with. The merge conflicts may be resolved, but the corelens lock command now has --pid and --time filters, which this code doesn't implement.

The problem is that the rwsem doesn't have clear owners -- at least not in the anonymous/reader owned case. We could implement some filtering by pid, but it would miss reader owners. The same would go for the time filter as a result.

I think the minimum cleanup we'd want to do is something like:

if pid or time:
    print("warning: skipping rwsem, since reliable owner info cannot be determined")
else:
    scan_rwsem_lock(...)

imran-kn · 2024-04-03T23:06:05Z

I want to merge this as-is. However, there's a follow-up item we'll need to deal with. The merge conflicts may be resolved, but the corelens lock command now has --pid and --time filters, which this code doesn't implement.

The problem is that the rwsem doesn't have clear owners -- at least not in the anonymous/reader owned case. We could implement some filtering by pid, but it would miss reader owners. The same would go for the time filter as a result.

I think the minimum cleanup we'd want to do is something like:
if pid or time:
    print("warning: skipping rwsem, since reliable owner info cannot be determined")
else:
    scan_rwsem_lock(...)

Thanks @brenns10 and yes I looked at pid and time based filtering but my idea was that since this PR was created before the introduction of pid and time based filtering and these filters are optional, I will get this PR merged and can add these filters in a separate PR. So that I don't introduce new work in an already reviewed code.
Let me know if this sounds good to you.

Since pid based filtering is happening for waiters we are fine for anonymously owned or reader owned rwsems too. If its okay I can add the change in this PR or create a separate one as mentioned earlier

Signed-off-by: Imran Khan <[email protected]>

imran-kn · 2024-04-04T01:10:51Z

I want to merge this as-is. However, there's a follow-up item we'll need to deal with. The merge conflicts may be resolved, but the corelens lock command now has --pid and --time filters, which this code doesn't implement.
The problem is that the rwsem doesn't have clear owners -- at least not in the anonymous/reader owned case. We could implement some filtering by pid, but it would miss reader owners. The same would go for the time filter as a result.
I think the minimum cleanup we'd want to do is something like:
if pid or time:
    print("warning: skipping rwsem, since reliable owner info cannot be determined")
else:
    scan_rwsem_lock(...)
Thanks @brenns10 and yes I looked at pid and time based filtering but my idea was that since this PR was created before the introduction of pid and time based filtering and these filters are optional, I will get this PR merged and can add these filters in a separate PR. So that I don't introduce new work in an already reviewed code. Let me know if this sounds good to you.

Since pid based filtering is happening for waiters we are fine for anonymously owned or reader owned rwsems too. If its okay I can add the change in this PR or create a separate one as mentioned earlier

@brenns10 anyways I went ahead and added pid and time based filtering as well. Now that box is ticked. So if you could just review 923ea5ddf8cc , we should be ready to merge this

oracle-contributor-agreement bot added the OCA Verified All contributors have signed the Oracle Contributor Agreement. label Feb 2, 2024

imran-kn changed the title ~~locking: Add helpers for rwsem, spinlocks and osq locks~~ [DRAFT]:locking: Add helpers for rwsem, spinlocks and osq locks Feb 2, 2024

imran-kn changed the title ~~[DRAFT]:locking: Add helpers for rwsem, spinlocks and osq locks~~ locking: Add helpers for rwsem and osq locks Feb 15, 2024

imran-kn requested review from brenns10, biger410 and pssatapathy-oracle February 15, 2024 09:37

brenns10 requested changes Feb 21, 2024

View reviewed changes

brenns10 requested changes Feb 23, 2024

View reviewed changes

imran-kn force-pushed the lock-work branch from 372258f to 3fd9252 Compare March 14, 2024 03:08

brenns10 previously approved these changes Apr 2, 2024

View reviewed changes

imran-kn added 11 commits April 3, 2024 11:37

locking: mutex: sanitize mutex_owner and add a helper to check mutex …

2973c4c

…state. Signed-off-by: Imran Khan <[email protected]>

util: Add helper for per-cpu to cpu conversion.

7a18de5

Signed-off-by: Imran Khan <[email protected]>

locking: osq: add helpers for MCS locks.

a3cd619

Signed-off-by: Imran Khan <[email protected]>

locking: rwsem: add helpers for rwsem locks.

5506eec

Signed-off-by: Imran Khan <[email protected]>

locking: rwsem: Include waiter call stack in waiter info.

40e894c

Also include waiters wait time, last run cpu and its state. Signed-off-by: Imran Khan <[email protected]>

lock: Add rwsem scanner.

9cb0c82

Signed-off-by: Imran Khan <[email protected]> Conflicts: drgn_tools/lock.py

locking: address review comments.

a680d66

Also add some missing fixes into rwsem helpers. Signed-off-by: Imran Khan <[email protected]> Conflicts: drgn_tools/lock.py

lock: Use bt_has_any in place of bt_has in rwsem scanner.

a69659e

Signed-off-by: Imran Khan <[email protected]>

locking: return an empty list for free osq_lock.

e79b78e

Signed-off-by: Imran Khan <[email protected]>

imran-kn dismissed brenns10’s stale review via eb12789 April 3, 2024 02:12

imran-kn force-pushed the lock-work branch from cd9c0e2 to eb12789 Compare April 3, 2024 02:12

imran-kn requested a review from brenns10 April 3, 2024 02:25

brenns10 previously approved these changes Apr 3, 2024

View reviewed changes

imran-kn dismissed brenns10’s stale review via ea2bddd April 4, 2024 00:37

lock: add pid and time based filtering for rwsem scanner.

923ea5d

Signed-off-by: Imran Khan <[email protected]>

imran-kn force-pushed the lock-work branch from ea2bddd to 923ea5d Compare April 4, 2024 00:54

brenns10 approved these changes Apr 4, 2024

View reviewed changes

brenns10 merged commit 564772d into main Apr 4, 2024
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

locking: Add helpers for rwsem and osq locks #69

locking: Add helpers for rwsem and osq locks #69

imran-kn commented Feb 2, 2024 •

edited

Loading

imran-kn commented Feb 15, 2024

brenns10 left a comment

brenns10 left a comment

brenns10 Feb 23, 2024

imran-kn Feb 25, 2024

brenns10 Mar 19, 2024

imran-kn Mar 23, 2024

brenns10 Mar 29, 2024

imran-kn Apr 2, 2024

imran-kn commented Mar 14, 2024

brenns10 commented Mar 19, 2024

brenns10 left a comment

brenns10 commented Apr 2, 2024

imran-kn commented Apr 3, 2024

brenns10 left a comment

imran-kn commented Apr 3, 2024 •

edited

Loading

imran-kn commented Apr 4, 2024

locking: Add helpers for rwsem and osq locks #69

locking: Add helpers for rwsem and osq locks #69

Conversation

imran-kn commented Feb 2, 2024 • edited Loading

imran-kn commented Feb 15, 2024

brenns10 left a comment

Choose a reason for hiding this comment

brenns10 left a comment

Choose a reason for hiding this comment

brenns10 Feb 23, 2024

Choose a reason for hiding this comment

imran-kn Feb 25, 2024

Choose a reason for hiding this comment

brenns10 Mar 19, 2024

Choose a reason for hiding this comment

imran-kn Mar 23, 2024

Choose a reason for hiding this comment

brenns10 Mar 29, 2024

Choose a reason for hiding this comment

imran-kn Apr 2, 2024

Choose a reason for hiding this comment

imran-kn commented Mar 14, 2024

brenns10 commented Mar 19, 2024

brenns10 left a comment

Choose a reason for hiding this comment

brenns10 commented Apr 2, 2024

imran-kn commented Apr 3, 2024

brenns10 left a comment

Choose a reason for hiding this comment

imran-kn commented Apr 3, 2024 • edited Loading

imran-kn commented Apr 4, 2024

imran-kn commented Feb 2, 2024 •

edited

Loading

imran-kn commented Apr 3, 2024 •

edited

Loading