Lint Errors and spl_kmem_caches regression#236
Conversation
Context: Issue: pytest-dev/pytest#7473 PR that was just merged: pytest-dev/pytest#7476 Opened issue to get rid of this once `pytest` is updated: #235
Codecov Report
@@ Coverage Diff @@
## master #236 +/- ##
==========================================
+ Coverage 87.35% 87.38% +0.03%
==========================================
Files 60 60
Lines 2467 2473 +6
==========================================
+ Hits 2155 2161 +6
Misses 312 312
Continue to review full report at Codecov.
|
ahrens
left a comment
ahrens
left a comment
There was a problem hiding this comment.
Nice handling of both old and new ways of getting the allocation count!
| # doesn't exist (an AttributeError will be thrown). | ||
| # | ||
| return int(cache.skc_obj_alloc.value_()) | ||
| return int(cache.skc_obj_alloc.value_()) |
There was a problem hiding this comment.
This line is equivalent to what we run in the except AttributeError case, so rather than duplicating the two, would it be better to use:
except AttributeError:
pass
and let the code reach this line in that case?
Or perhaps we should more simply remove this line, since I think it's dead code?
There was a problem hiding this comment.
Actually thanks for pointing this out... this pointed out a bug in my code. We don't want to look at the percpu counter if this is not a linux_slub backed cache. I changed the code and retesting now.
| assert sdb.type_canonical_name(cache.type_) == 'struct spl_kmem_cache *' | ||
| try: | ||
| return int(drgn_percpu.percpu_counter_sum(cache.skc_linux_alloc)) | ||
| except AttributeError: |
There was a problem hiding this comment.
out of curiosity, is there no way to ask if the structure contains a given member? e.g. something like:
if 'skc_linux_alloc' in cache:
return int(drgn_percpu.percpu_counter_sum(cache.skc_linux_alloc))
else:
return int(cache.skc_obj_alloc.value_())
There was a problem hiding this comment.
Unfortunately I did look at drgn.Type API for that and there wasn't such a thing. The way to do this would be something like if 'skc_linux_alloc in [ x.name for x in type.members]`. I thought that the code for the loop was probably not worth it. Does this make sense? Do you think a variant of this alternative would be preferable?
There was a problem hiding this comment.
Thanks, got it. I'm good with what you have.
The following commit from ZoL introduced this counter: openzfs/zfs@ec1fea4
|
Updated the PR applying @prakashsurya 's suggestion and fixing a bug uncovered by it. |
4 participants
First Commit: Disable pylint false-positives for pytest
Context:
Issue: pytest-dev/pytest#7473
PR that was just merged: pytest-dev/pytest#7476
Opened issue to get rid of this once
pytestis updated:#235
Second Commit: Update spl_kmem_caches command to be aware of new percpu counter
The following commit from ZoL introduced this new counter to be used instead of what sdb was looking at before
openzfs/zfs@ec1fea4
Note that since our test crash dump was generated with old bits, we'd need a new crash dump to test that new condition. Until that happens I decided that the following manual testing should suffice.
= Manual testing
Before:
After:
Prakash pointed out a bug that was missed by my manual testing above where SPL-based caches would use the percpu counter. This was missed because I was only looking at SLUB-based caches in the above out. After re-iterating now both work as before.