Set a better granularity and distribution on roundrobin protocol.

[araujo]

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.

[thompsa]

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]

This comment was removed by thompsa.

[thompsa]

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

if_lagg_rr.diff1 KBDownload

[araujo]

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]

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

[thompsa]

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]

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]

sys/net/if_lagg.c

1708 ↗

(On Diff #976)

Yes, this also looks not quite right to me.

[araujo]

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?

if_lagg-rr2.patch4 KBDownload

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

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

[araujo]

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]

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

[araujo]

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?

[glebius]

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

[adrian]

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

[araujo]

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

Best Regards,

[araujo]

• 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

[araujo]

• Bump manpage to 19th of January.

[araujo]

• Update copyright to 2016 instead of 2015.

Sorry the noise.

[wblock]

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]

Address @wblock suggestion.
Bump date.

Thanks.