Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

transceivers: convert to counted_ringbufs #1780

Merged
merged 3 commits into from
May 15, 2024

Conversation

Aaron-Hartwig
Copy link
Contributor

@Aaron-Hartwig Aaron-Hartwig commented May 10, 2024

I'm going to use a counted_ringbuf in the transceivers task because there is a lot of activity that happens, specifically in the UDP handler. I also bumped up the size from 16 -> 32 of that one because there's nearly constant activity and each UDP message involves at least three entries.

Fixes #1778

@Aaron-Hartwig Aaron-Hartwig requested a review from hawkw May 10, 2024 21:29
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a couple suggestions. At some point we may also want to add #[count(children)] to some of the errors, so that we can generate a separate counter for each error variant, but it's up to you whether we ought to do that now, and which errors are worth counting individually.

drv/transceivers-server/src/udp.rs Show resolved Hide resolved
drv/transceivers-server/src/main.rs Show resolved Hide resolved
drv/transceivers-server/src/main.rs Outdated Show resolved Hide resolved
@Aaron-Hartwig Aaron-Hartwig force-pushed the aaron/transceivers-ringbuf-counters branch from f719f20 to 757b3cf Compare May 14, 2024 22:01
@Aaron-Hartwig
Copy link
Contributor Author

At some point we may also want to add #[count(children)] to some of the errors, so that we can generate a separate counter for each error variant, but it's up to you whether we ought to do that now, and which errors are worth counting individually.

@hawkw I did add it in a few places, but some of the other places I'd like to add it are blocked by #[derive(Count)] being limited to enums right now. Most of the places I'd like to add it would be keyed off an event happening at a particular port (currently a usize in the Trace, but could easily be made a LogicalPort which is a struct that wraps a u8). Thanks for the feedback as always!

@Aaron-Hartwig Aaron-Hartwig force-pushed the aaron/transceivers-ringbuf-counters branch from 757b3cf to f8c344b Compare May 14, 2024 22:05
@hawkw
Copy link
Member

hawkw commented May 14, 2024

I did add it in a few places, but some of the other places I'd like to add it are blocked by #[derive(Count)] being limited to enums right now. Most of the places I'd like to add it would be keyed off an event happening at a particular port (currently a usize in the Trace, but could easily be made a LogicalPort which is a struct that wraps a u8). Thanks for the feedback as always!

Yeah, I think that if you want to count events per LogicalPort, you would need to actually implement Count for that type manually. I would probably do it using an array of counters that's indexed by port, like this:

impl Count for LogicalPort {
    type Counters = [AtomicU32; NUM_PORTS],;

    #[allow(clippy::declare_interior_mutable_const)]
    const NEW_COUNTERS: Self::Counters = {
        const COUNTER_INIT: AtomicU32 = AtomicU32::new(0);
        [COUNTER_INIT; NUM_PORTS]
    };

    fn count(&self, counters: &Self::Counters) {
        let Some(ctr) = counters.get(self.0 as usize) else {
            // `LogicalPort`s should never index the counters array out of
            // bounds, but if that does ever happen, let's just bail instead of
            // panicking...
            return;
        }
        armv6m_atomic_hack::AtomicU32Ext::fetch_add(ctr, 1, Ordering::Relaxed);
    }
}

Although, OTTOMH, I'm not sure how well the humility code for reading counters would handle an array, so I might have to go back and fix that if you did do something like this...

@Aaron-Hartwig
Copy link
Contributor Author

@hawkw should we do the Count impl for LogicalPort in this PR? I don't want to force Humility work upon you if you've something more pressing to work on. I can always write an issue for this and handle it in The Future.

@hawkw
Copy link
Member

hawkw commented May 14, 2024

@hawkw should we do the Count impl for LogicalPort in this PR? I don't want to force Humility work upon you if you've something more pressing to work on. I can always write an issue for this and handle it in The Future.

My preference would be to go ahead and merge this as-is, and add something like that later --- I can think of a few other places where it would be nice to be able to use arrays for generating counters from a numeric index (e.g. SensorIds in task-sensors) if I get around to making it work nicely in humility!.

@Aaron-Hartwig
Copy link
Contributor Author

That works for me. I've opened oxidecomputer/humility#487 so we remember where we talked about this. If you give this a +1 I'll get it merged!

@Aaron-Hartwig Aaron-Hartwig merged commit 561b577 into master May 15, 2024
104 checks passed
@Aaron-Hartwig Aaron-Hartwig deleted the aaron/transceivers-ringbuf-counters branch May 15, 2024 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Have the transceivers task utilize the ringbuf counters
3 participants