Page MenuHomeFreeBSD

Set a better granularity and distribution on roundrobin protocol.
ClosedPublic

Authored by araujo on Aug 5 2014, 3:14 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 30, 3:22 AM
Unknown Object (File)
Mon, Nov 25, 6:56 PM
Unknown Object (File)
Fri, Nov 22, 9:14 PM
Unknown Object (File)
Fri, Nov 22, 9:14 PM
Unknown Object (File)
Fri, Nov 22, 9:13 PM
Unknown Object (File)
Fri, Nov 22, 9:13 PM
Unknown Object (File)
Fri, Nov 22, 9:12 PM
Unknown Object (File)
Fri, Nov 22, 9:12 PM

Details

Summary
  • Add a IOCTL rr_limit to let users fine tuning the number of packets to be sent using round robin mode. With this approach, the user can tuning the number of packets on round robin mode according with their application, and have a better throughput as it can reduce the unordered packets as well as reduce the SACK.

Relnotes: yes

Test Plan
  • Running iperf with several threads and printout the lp that is passed as parameter to lagg_enqueue().
  • Also, use ifstat to check the distribution among the interfaces.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

araujo retitled this revision from to Set a better granularity and distribution on roundrobin protocol..
araujo updated this object.
araujo edited the test plan for this revision. (Show Details)
araujo added reviewers: adrian, thompsa, glebius.
sys/net/if_lagg.c
1746 ↗(On Diff #958)

This is overly complicated and also is doing string operations in the fast path. This could all be avoided by a bucket count and only incrementing sc_seq when the bucket is empty.

This comment was removed by thompsa.

It should be able to be achieved with something like this (untested).

araujo edited edge metadata.

The patch that thompsa@ sent is much better and achieve the same goal. There is only one small bug, it is necessary set the sc_bkt with lagg_rr_packets, or otherwise it will never reach the atomic_cmpset_int().

And it is necessary set the sc_bkt only when it reach 0.

This is the final patch that includes also the lagg(4) manual page.

sys/net/if_lagg.c
195 ↗(On Diff #976)

This may want to be a sysctl function to check the bounds and also lower reset sc_bkt to the new value. Otherwise there is no way to correct a large count.

1698 ↗(On Diff #976)

This shouldnt be needed and may mess up the reference counting. Maybe add +1 in lagg_rr_init.

1708 ↗(On Diff #976)

This is still not quite right. atomics are hard, someone else to review is needed.

if (lagg_rr_packets > 1) {
    if (atomic_fetchadd_32(&sc->sc_bkt, -1) == 1) {
      atomic_add_int(&sc->sc_bkt, lagg_rr_packets);
      p = atomic_fetchadd_32(&sc->sc_seq, 1);
    } else {
      p = sc->sc_seq;
   }
  } else
    p = atomic_fetchadd_32(&sc->sc_seq, 1);
sys/net/if_lagg.c
195 ↗(On Diff #976)

Yes, in this case I totally agree.
I'm gonna change the patch to cover this case, but I'm not sure yet if a sysctl function is needed.

1698 ↗(On Diff #976)

Actually it is needed. Because lagg_rr_attach() is called only when we create the interface! If we create the interface and after set the lagg_rr_packets, at lagg_rr_start() it will never read the lagg_rr_packets again.

I can't see where it can mess up with the counter, at least in my tests it doesn't mess.

sys/net/if_lagg.c
1708 ↗(On Diff #976)

Yes, this also looks not quite right to me.

I came up if another patch, it fix the atomic operations when the user change the net.link.lagg.rr_packets value. It is necessary keep a reference in lagg_softc structure, when the user change the value of rr_packets, it can be applied in the right way.

At lagg_rr_start() function, I keep the previous code sent by thompsa@, there is no need to make any extra operation as suggested before.

And guys, the atomic(9) operations seems pretty simple to me, if it still has problem; could you please point out what is the main concern?

Using sysctl(9) to configure this feature is a bad style. This should be done by ifconfig in terms of a particular interface, not a global sysctl knob. Those sysctl knobs that exist should also be converted to ioctls submitted via ifconfig(8). Adrian promised to do that "really soon" (tm) when added these knobs.

Using sysctl(9) to configure this feature is a bad style. This should be done by ifconfig in terms of a particular interface, not a global sysctl knob. Those sysctl knobs that exist should also be converted to ioctls submitted via ifconfig(8). Adrian promised to do that "really soon" (tm) when added these knobs.

Adrian@ any news about the the comments bellow?

by glebius:
'''
Using sysctl(9) to configure this feature is a bad style. This should be done by ifconfig in terms of a particular interface, not a global sysctl knob. Those sysctl knobs that exist should also be converted to ioctls submitted via ifconfig(8). Adrian promised to do that "really soon" (tm) when added these knobs.
'''

heh - I was then fired from Netflix, and for some reason it slipped my mind :)

In D540#17, @adrian wrote:

heh - I was then fired from Netflix, and for some reason it slipped my mind :)

Wow, that is bad.
So, what we should do with this change D540? Do you have plan to make those ioctl stuff?

If you followed svn-src-head@, you would noticed that Hiroki Sato already did that. Thanks to him.

adrian edited edge metadata.

After re-re-re-reviewing this, I'm okay with it.

Round-robin mode is already out of order and you're not making any of the other lagg types out of order.)

(yes, this is an old review; I'm just making sure I'm recording my eventual decision on it.)

Thanks!

-adrian

This revision is now accepted and ready to land.Dec 26 2014, 6:01 PM

Thanks Adrian, I'm gonna rework this patch as gleb@ mention.

Best Regards,

araujo edited edge metadata.
  • Address @glebius concern and instead to use sysctl(8), use IOCTL to handle rr_limit per interface.
  • Remove the sysctl(8) entry.
  • Document with an example how to set the roundrobin limit.

Ex:

ifconfig lagg0 rr_limit 500
ifconfig -v lagg0

This revision now requires review to proceed.Jan 19 2016, 6:17 AM
  • Bump manpage to 19th of January.
araujo edited edge metadata.
  • Update copyright to 2016 instead of 2015.

Sorry the noise.

wblock added inline comments.
share/man/man4/lagg.4
113 ↗(On Diff #12452)

Please start new sentences on new lines.
Suggested edit to avoid indefinite "this" and pauses.

Using
.Ic roundrobin
mode can cause unordered packet arrival at the client.
Throughput might be limited as the client performs CPU-intensive
packet reordering.
araujo edited edge metadata.

Address @wblock suggestion.
Bump date.

Thanks.

bapt edited edge metadata.
This revision is now accepted and ready to land.Jan 23 2016, 12:10 AM
This revision was automatically updated to reflect the committed changes.