linuxkpi: dma_get_cache_alignment(): Fix off-by-one result
ClosedPublic
Actions

Authored by olce on Oct 17 2023, 3:02 PM.

Details

Reviewers

manu
bz
markj

Commits

rG89af1555b4ef: linuxkpi: dma_get_cache_alignment(): Fix off-by-one result
rGf946a5ec9692: linuxkpi: dma_get_cache_alignment(): Fix off-by-one result
rG2c7dd66d09a1: linuxkpi: dma_get_cache_alignment(): Fix off-by-one result

Summary

Substituting 'uma_align_cache' by the appropriately named accessor
uma_get_cache_align_mask() made apparent that dma_get_cache_alignment()
was off by one, since it was defined to be the mask derived from the
alignment value.

MFC after: 2 weeks
Sponsored by: The FreeBSD Foundation

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 54046
Build 50936: arc lint + arc unit

Event Timeline

olce created this revision.Oct 17 2023, 3:02 PM

Herald added subscribers: linuxkpi, imp. · View Herald TranscriptOct 17 2023, 3:02 PM

olce requested review of this revision.Oct 17 2023, 3:02 PM

Harbormaster completed remote builds in B54046: Diff 128930.Oct 17 2023, 3:02 PM

olce added a parent revision: D42263: uma: New check_align_mask(): Validate alignments (INVARIANTS).Oct 17 2023, 3:02 PM

olce added a child revision: D42265: uma: Permit specifying max of cache line and some custom alignment.Oct 17 2023, 3:05 PM

markj accepted this revision.Oct 17 2023, 3:17 PM

This revision is now accepted and ready to land.Oct 17 2023, 3:17 PM

Diff update, remove a superfluous TAB.

This revision now requires review to proceed.Oct 19 2023, 8:16 AM

Harbormaster completed remote builds in B54074: Diff 129064.Oct 19 2023, 8:16 AM

markj accepted this revision.Oct 19 2023, 12:57 PM

This revision is now accepted and ready to land.Oct 19 2023, 12:57 PM

Does this need a separate commit or why can't D42258 just get the fix given the "bug" was introduced there and it's still open?

This revision now requires changes to proceed.Oct 19 2023, 1:52 PM

In D42264#964954, @bz wrote:

Does this need a separate commit or why can't D42258 just get the fix given the "bug" was introduced there and it's still open?

The bug wasn't introduced there. uma_align_cache has the same problem, it's a mask.

In D42264#964959, @markj wrote:

In D42264#964954, @bz wrote:

Does this need a separate commit or why can't D42258 just get the fix given the "bug" was introduced there and it's still open?

The bug wasn't introduced there. uma_align_cache has the same problem, it's a mask.

Then either the commit message should be clearer, or better we should fix the current version before changing it so that the 10y old problem isn't first masked by a different change?

In D42264#964960, @bz wrote:

Then either the commit message should be clearer, (...)

The proposed commit message indeed makes sense only in the context of the previous commits of this series. Going to rework it.

In D42264#964960, @bz wrote:

or better we should fix the current version before changing it so that the 10y old problem isn't first masked by a different change?

I don't understand this line of reasoning. The bug isn't masked by a different change, on the contrary IMHO the previous change makes it more apparent. It's actually how I found it!

It's true that the fix could be made a standalone commit which could be committed right away, but a priori I don't think it's worth the effort, given it seems that the whole series is going to get in quite soon, and after all the bug has been there for 10 years as you say.