Page MenuHomeFreeBSD

Add support for generic backpressure indicator for ratelimited transmit queues aswell as non-ratelimited ones
ClosedPublic

Authored by hselasky on Jul 7 2017, 11:16 AM.

Details

Summary

Add the required structure bits in order to support a backpressure indication with ratelimited connections aswell as non-ratelimited ones. The backpressure indicator is a value between zero and 65535 inclusivly, indicating if the destination transmit queue is empty or full respectivly. Applications can use this value as a decision point for when to stop transmitting data to avoid endless ENOBUFS error codes upon transmitting an mbuf. This indicator is also useful to reduce the latency for ratelimited queues.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

hselasky created this revision.Jul 7 2017, 11:16 AM
gallatin accepted this revision.Jul 10 2017, 2:13 PM
This revision is now accepted and ready to land.Jul 10 2017, 2:13 PM
kib added a subscriber: kib.Aug 29 2017, 9:50 AM
kib added inline comments.
sys/net/if_var.h
202 ↗(On Diff #30535)

Why using uint32_t for a value declared to be 16bit ? Do you have plans for extending the values ?

Also, is this structure ever becomes the part of the user ABI ? Note that on 64bit platforms and on some 32bit plaftorms like ARMv6/7 there is implicit padding at the end which makes the structure to require translation for compat32.

sys/netinet/in_pcb.c
2820 ↗(On Diff #30535)

Why not

    if (ifp->if_snd_tag_query == NULL)
	  return (EOPNOTSUPP);

This reduces indent level for the ifp snd_tag_query call below and is in style of earlier code in the function.

np edited edge metadata.Aug 29 2017, 7:39 PM

Instead of using tags have you considered a modified if_transmit that returns the current occupancy-level of the tx queue (as a percentage or some normalized representation)? That would make it fully stateless and we wouldn't have to involve any inp in any of this. Any feature hooked up to inp will work only for IP connections that terminate on that node and not, say, packet forwarding use cases.

Instead of using tags have you considered a modified if_transmit that returns the current occupancy-level of the tx queue (as a percentage or some normalized representation)? That would make it fully stateless and we wouldn't have to involve any inp in any of this. Any feature hooked up to inp will work only for IP connections that terminate on that node and not, say, packet forwarding use cases.

@np : the current implementation has been very carefully thought through and yes, modifying if_transmit has been considered. Modifying if_transmit doesn't solve the problem, because the occupancy-level of the TX queue is not know until _after_ transmit and we need to know the value _before_ transmit. Relying on the current send tag implementation seems the most sensible involving the least number of races and overhead.

hselasky added inline comments.Sep 6 2017, 6:44 AM
sys/net/if_var.h
202 ↗(On Diff #30535)

Yes, you are right. I'll fix.

sys/netinet/in_pcb.c
2820 ↗(On Diff #30535)

I'll fix. Thank you!

hselasky updated this revision to Diff 32704.Sep 6 2017, 6:59 AM
This revision now requires review to proceed.Sep 6 2017, 6:59 AM
hselasky marked 4 inline comments as done.Sep 6 2017, 6:59 AM

Can everyone have a look at this patch again and press accept ? I want to push this patch unless there are any major blockers.

hselasky added inline comments.Sep 6 2017, 7:01 AM
sys/net/if_var.h
202 ↗(On Diff #30535)

It is not so common to use uint16_t in these kind of structures and I think access uint32_t is more efficient when it comes to performance.

kib accepted this revision.Sep 6 2017, 7:15 AM

It looks fine to me, but this is a view of the person who knows nothing about the network stack. I.e. it seems to be fine WRT ABI proposed or generic code flow.

This revision is now accepted and ready to land.Sep 6 2017, 7:15 AM
gnn accepted this revision.Sep 6 2017, 12:47 PM
This revision was automatically updated to reflect the committed changes.