Page MenuHomeFreeBSD

Add support for batching interrupts in system interrupt code
Needs ReviewPublic

Authored by hselasky on Jan 24 2020, 11:05 AM.
Tags
None
Referenced Files
Unknown Object (File)
Feb 19 2024, 9:48 AM
Unknown Object (File)
Jan 30 2024, 12:52 AM
Unknown Object (File)
Jan 14 2024, 5:19 AM
Unknown Object (File)
Jan 7 2024, 11:02 AM
Unknown Object (File)
Jan 7 2024, 11:02 AM
Unknown Object (File)
Jan 7 2024, 10:59 AM
Unknown Object (File)
Jan 7 2024, 10:59 AM
Unknown Object (File)
Jan 7 2024, 10:59 AM
Subscribers

Details

Summary

Implement intr_handler_needs_execution() to serve the purpose of allowing high-frequency interrupts to be batched.
By calling intr_handler_needs_execution() the driver acknowledge interrupts that occurred after the invocation of the current interrupt handler call.

This change basically reverts r357004.

Interrupt handlers can do batching like this:

static void interrupt_handler(void *arg)
{
  int to = 1000;

  do {
       /* interrupt code */
   } while (to-- && intr_handler_needs_execution(ih_cookie));
}

Sponsored by: Mellanox Technologies

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 28943

Event Timeline

Fix compilation issue and make sure network epoch is available when used!

Some feedback:

  • This essentially removes the epoch optimization from all network drivers because it becomes opt-in, rather than automatic. I don't like that
  • You've replaced a flag with an 8 byte field in a struct which can be touched hundreds of thousands or millions of times per second. I don't like that.
  • You've removed interrupt storm epoch hogging protection. I don't like that.

To do what you want, can you instead make a negative flag for opting opt and leave most the rest of the code intact?

This revision now requires changes to proceed.Jan 24 2020, 2:11 PM

Hi,

This essentially removes the epoch optimization from all network drivers because it becomes opt-in, rather than automatic. I don't like that

Yes, this feature should be op-in until someone sits down and fixes all the network driver's input paths.

FreeBSD is a general purpose operating system. It shall not only be hard-optimised for network drivers, like the patch Gleb S. made.
There might be more subsystems which want another epoch than the network epoch and my patch addresses that.

You've replaced a flag with an 8 byte field in a struct which can be touched hundreds of thousands or millions of times per second. I don't like that.

This field should not be touched more than 8k times per second. Else you have a network card you should not use for heavy network traffic or you have not tuned the RX queues appropriately.

You've removed interrupt storm epoch hogging protection. I don't like that.

The epoch wrapper should be per-interrupt function call and that's it.

To do what you want, can you instead make a negative flag for opting opt and leave most the rest of the code intact?

I disagree.

To add some colors to the discussion, I disagree with both approaches. My main complain is that *net epoch* should not be married with the interrupts handlers infrastructure, neither in code nor in data. I agree with people that mark this as layering violations.

If you want automatic epoch enter, with possible rate or interval limiting, you need a hooks that are executed before and after the thread handler loop is performed. We already have examples of such hooks with pre- and post- filter and ithread hooks.

And yes, it should be opt-in and not automatic on INTR_TYPE_NET

hselasky retitled this revision from Add support for subsystem specific epoch flag when requesting interrupts to Add support for batching interrupts to system interrupt code.
hselasky edited the summary of this revision. (Show Details)
hselasky added a reviewer: mav.

Implement feedback.

hselasky retitled this revision from Add support for batching interrupts to system interrupt code to Add support for batching interrupts in system interrupt code.Jan 27 2020, 12:18 PM
hselasky edited the summary of this revision. (Show Details)