Page MenuHomeFreeBSD

Set a better granularity and distribution on roundrobin protocol.
ClosedPublic

Authored by araujo on Aug 5 2014, 3:14 AM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

araujo updated this revision to Diff 958.Aug 5 2014, 3:14 AM
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.
thompsa added inline comments.Aug 5 2014, 11:40 PM
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.

thompsa edited edge metadata.Aug 6 2014, 12:52 AM
This comment was removed by thompsa.

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

araujo updated this revision to Diff 976.Aug 6 2014, 3:15 AM
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.

araujo added a comment.Aug 6 2014, 3:17 AM

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

thompsa added inline comments.Aug 6 2014, 3:47 AM
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);
araujo added inline comments.Aug 6 2014, 6:30 AM
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.

adrian added inline comments.Aug 6 2014, 6:13 PM
sys/net/if_lagg.c
1708 ↗(On Diff #976)

Yes, this also looks not quite right to me.

araujo added a comment.Aug 7 2014, 1:55 AM

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?

glebius edited edge metadata.Aug 8 2014, 10:45 AM

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.

melifaro edited edge metadata.Sep 19 2014, 4:46 PM
melifaro added a subscriber: melifaro.

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.
'''

adrian edited edge metadata.Oct 27 2014, 7:49 AM

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 accepted this revision.Dec 26 2014, 6:01 PM
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 updated this revision to Diff 12450.Jan 19 2016, 6:17 AM
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
araujo added a reviewer: gnn.
araujo updated this revision to Diff 12451.Jan 19 2016, 6:19 AM
  • Bump manpage to 19th of January.
araujo updated this object.Jan 19 2016, 6:23 AM
araujo edited edge metadata.
araujo updated this revision to Diff 12452.Jan 19 2016, 6:27 AM
  • Update copyright to 2016 instead of 2015.

Sorry the noise.

koobs added a subscriber: koobs.Jan 19 2016, 7:19 AM
wblock added a subscriber: wblock.Jan 19 2016, 2:10 PM
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 updated this revision to Diff 12487.Jan 20 2016, 2:11 AM
araujo edited edge metadata.

Address @wblock suggestion.
Bump date.

Thanks.

bapt accepted this revision.Jan 23 2016, 12:10 AM
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.