Details

Reviewers

• rodrigc
hrs
glebius
thompsa
bapt
gnn
adrian

Group Reviewers

manpages

Commits

rS294615: Add an IOCTL rr_limit to let users fine tuning the number of packets to be

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

No Lint Coverage

Unit

No Test Coverage

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

if_lagg_rr.diff1 KBDownload

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.

thompsa added inline comments.Aug 6 2014, 3:47 AM

sys/net/if_lagg.c
195	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	This shouldnt be needed and may mess up the reference counting. Maybe add +1 in lagg_rr_init.
1708	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	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	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	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?

if_lagg-rr2.patch4 KBDownload

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

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.

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,

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

Herald added a reviewer: manpages. · View Herald TranscriptJan 19 2016, 6:17 AM

Herald added a subscriber: imp. · View Herald Transcript

araujo added reviewers: bapt, • rodrigc, hrs.Jan 19 2016, 6:18 AM

araujo added a reviewer: gnn.

Bump manpage to 19th of January.

Herald edited edge metadata. · View Herald TranscriptJan 19 2016, 6:19 AM

araujo updated this object.Jan 19 2016, 6:23 AM

araujo edited edge metadata.

Sorry the noise.

Herald edited edge metadata. · View Herald TranscriptJan 19 2016, 6:27 AM

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

Address @wblock suggestion.
Bump date.

Thanks.

Herald edited edge metadata. · View Herald TranscriptJan 20 2016, 2:11 AM

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

Closed by commit rS294615: Add an IOCTL rr_limit to let users fine tuning the number of packets to be (authored by araujo). · Explain WhyJan 23 2016, 4:19 AM

This revision was automatically updated to reflect the committed changes.

Set a better granularity and distribution on roundrobin protocol.
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 958

share/man/man4/lagg.4

sys/net/if_lagg.h

sys/net/if_lagg.c

Set a better granularity and distribution on roundrobin protocol.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 958

share/man/man4/lagg.4

sys/net/if_lagg.h

sys/net/if_lagg.c

Set a better granularity and distribution on roundrobin protocol.
ClosedPublic
Actions

Revision Contents
Changeset List