lib/connections, lib/config: Bandwidth throttling per remote device (fixes #4516) #4603

qepasa · 2017-12-17T15:17:48Z

Purpose

Hello everyone. As described in the issue. This fix enables read/write bandwidth throttling per remote device alongside current global limiting. It can be set via device tab in advanced settings.

Testing

Testing was done using 2 remote devices. Tested scenarios include adding and removing devices and various configurations of r/w rates:
device1: 300/300, device2: unlimited/unlimited, global: unlimited/unlimited
device1: 300/300, device2: unlimited/unlimited, global: 150/150
device1: 300/300, device2: 50/50, global: 150/150
device1: unlimited/unlimited, device2: 50/50, global: 150/150
device1: unlimited/unlimited, device2: unlimited/unlimited, global: 700/700

Of course these tests may not be comprehensive. I'll be thankful for any additional suggestions or scenarios.

…ing#4516)

AudriusButkevicius · 2017-12-17T15:20:18Z

lib/connections/limiter.go

 }

 func (w *limitedWriter) Write(buf []byte) (int, error) {
 	if !w.isLAN || w.limiter.limitsLAN.get() {
-		take(w.limiter.write, len(buf))
+		deviceLimiter, ok := w.limiter.deviceWriteLimiter[w.remoteID]
+		if ok == false {


AudriusButkevicius · 2017-12-17T15:28:00Z

lib/connections/limiter.go

+		lim.deviceWriteLimiter[v.DeviceID] = rate.NewLimiter(rate.Inf, limiterBurstSize)
+		lim.deviceReadLimiter[v.DeviceID] = rate.NewLimiter(rate.Inf, limiterBurstSize)
+	}
+	l.Debugln("Rebuild finished")


This has no context, rebuild of what, where, why

AudriusButkevicius · 2017-12-17T15:28:20Z

lib/connections/limiter.go

 	l.CommitConfiguration(prev, cfg.RawCopy())
 	return l
 }

-func (lim *limiter) newReadLimiter(r io.Reader, isLAN bool) io.Reader {
-	return &limitedReader{reader: r, limiter: lim, isLAN: isLAN}
+func getInitialDevicesConfiguration(cfgCopy config.Configuration) config.DeviceConfigurationList {


This is only called once, I'd say that it's fine to wedge in the loop inline in the constructor.

AudriusButkevicius · 2017-12-17T15:29:10Z

lib/connections/limiter.go

}
+

Could just call rebuildMap here?

AudriusButkevicius · 2017-12-17T15:29:59Z

lib/connections/limiter.go

+// Compare read/write limits in configurations
+func (lim *limiter) checkDeviceLimits(from, to config.Configuration) bool {
+	for i := range from.Devices {
+		if from.Devices[i].DeviceID.Compare(to.Devices[i].DeviceID) != 0 {


You can just use equality for device ID's they should be comparable just fine.

AudriusButkevicius · 2017-12-17T15:31:19Z

lib/connections/limiter.go

 	if to.Options.MaxRecvKbps > 0 {
 		recvLimitStr = fmt.Sprintf("limit is %d KiB/s", to.Options.MaxRecvKbps)
 	}
-	l.Infof("Send rate %s, receive rate %s", sendLimitStr, recvLimitStr)
+	l.Infof("Global send rate %s, receive rate %s", sendLimitStr, recvLimitStr)


Overall or Total. Global usually talks about locations.

AudriusButkevicius · 2017-12-17T15:31:45Z

lib/connections/limiter.go

+		deviceLimiter, ok := r.limiter.deviceReadLimiter[r.remoteID]
+		if ok == false {
+			l.Debugln("deviceLimiter was not in the map")
+			deviceLimiter = rate.NewLimiter(rate.Inf, limiterBurstSize)


How can this happen?

It happens after removing remote device. I'm not sure about limitedWriter but I added it just to be safe.

AudriusButkevicius · 2017-12-17T15:32:36Z

lib/connections/limiter.go

 	"golang.org/x/net/context"
 	"golang.org/x/time/rate"
 )

+type deviceLimiters map[protocol.DeviceID]*rate.Limiter


This probably needs to be sync.Map (atomic map) as you have not implemented any locking, and there are definately multiple routines poking at it. Also, the atomic map should only be initialized once, because if you re-assign the map on the limiter struct, you still need a lock.

imsodin · 2017-12-18T16:15:24Z

Now you are using both locks and sync.Map, why?

The rebuildMap calls in several places seem overly complicated to me: Couldn't you just iterate over devices from old and new configuration in CommitConfiguration and delete/add/adjust limiters as necessary? When initializing you can then just call CommitConfiguration with empty maps as well.

AudriusButkevicius · 2017-12-18T16:05:13Z

lib/connections/limiter.go

+			continue
+		}
+
+		readLimiter, _ := lim.deviceReadLimiters.Load(v.DeviceID)


You should not throw away the ok, in case the value is not there.
Also, you are not locking here, so you might be accessing deviceReadLimiters as the map is being swapped out.

This is also a reason why rebuildMap should potentially do it without swapping the maps.

AudriusButkevicius · 2017-12-18T16:09:01Z

lib/connections/limiter.go

+	// A device has been added or removed
+	if len(from.Devices) != len(to.Devices) {
+		lim.rebuildMap(to)
+	}


Also, this is unnecessery if you handle it properly in checkDeviceLimits, or stop it from having side-effects.

AudriusButkevicius · 2017-12-18T16:07:03Z

lib/connections/limiter.go

+	lim.deviceWriteLimiters = deviceWriteLimiters
+	lim.deviceReadLimiters = deviceReadLimiters
+
+	l.Debugln("Rebuild of device limiters map finished")


Wouldn't it be better for rebuild to happen in place, this way getting rid of the lock that we have to get on every read and write.

AudriusButkevicius · 2017-12-18T16:08:46Z

lib/connections/limiter.go

+// Compare read/write limits in configurations
+func (lim *limiter) checkDeviceLimits(from, to config.Configuration) bool {
+	for i := range from.Devices {
+		if from.Devices[i].DeviceID != to.Devices[i].DeviceID {


This should blow up if a device is removed, as len(to) < len(from).

You should probably write a test case for this logic.
Test case for adding
Test case for removing
Test case for adding and removing in the same config commit cycle.

AudriusButkevicius · 2017-12-18T16:11:40Z

lib/connections/limiter.go

+	for i := range from.Devices {
+		if from.Devices[i].DeviceID != to.Devices[i].DeviceID {
+			// Something has changed in device configuration
+			lim.rebuildMap(to)


To be honest, this function as it states should only do checking, and not rebuilding (as a side-effect), and rebuilding and other cruft should happen in the CommitConfiguration.

AudriusButkevicius · 2017-12-18T16:14:42Z

lib/connections/limiter.go

+		if v.MaxRecvKbps > 0 {
+			recvLimitStr = fmt.Sprintf("limit is %d KiB/s", v.MaxRecvKbps)
+		}
+		l.Infof("Device %s: send rate %s, receive rate %s", v.DeviceID, sendLimitStr, recvLimitStr)


This will always print stuff, even if someones rate hasn't changed since before.
It will further always mess around with the rate limiters, regardless if the value changed or not.

AudriusButkevicius · 2017-12-18T16:15:29Z

lib/connections/limiter.go

+		r.limiter.mu.Lock()
+		deviceLimiter, ok := r.limiter.deviceReadLimiters.Load(r.remoteID)
+		r.limiter.mu.Unlock()
+		if !ok {


To be honest, if !ok, I'd panic (or do nothing), as it should clearly be there.
Now it's like a bandaid for a problem we can't explain.

I'd do a if deviceLimiter != nil check in take

AudriusButkevicius · 2017-12-18T16:16:38Z

lib/connections/limiter.go

 	}
 	return w.writer.Write(buf)
 }

 // take is a utility function to consume tokens from a rate.Limiter. No call
 // to WaitN can be larger than the limiter burst size so we split it up into
 // several calls when necessary.
-func take(l *rate.Limiter, tokens int) {
+func take(l, deviceLimiter *rate.Limiter, tokens int) {


the l could do with a better name now, and the doc string should be updated.

AudriusButkevicius · 2017-12-18T16:18:53Z

lib/connections/limiter.go

+
+	// copy *limiter in case remote device is still connected, when remote device is added we create new read/write limiters
+	for _, v := range to.Devices {
+		if readLimiter, ok := lim.deviceReadLimiters.Load(v.DeviceID); ok {


What if there are two rebuildMap calls in parallel due to frequent config save or something like that, this might be halfway through a swap too.

qepasa · 2017-12-18T16:46:35Z

@AudriusButkevicius You're right. I assumed (which was pretty stupid) that only one routine at the time visits CommitConfiguration. I guess the right way will be to rewrite it so that "rebuild" happens without swapping map instances and definitely implement mentioned tests and fix other issues pointed by you. Thanks for feedback!

@imsodin My intention was to use lock in order to ensure atomic assignment in rebuildMap.

AudriusButkevicius · 2017-12-18T17:05:24Z

@qepasa thanks for working on this.

calmh · 2017-12-24T20:52:23Z

lib/connections/limiter.go

+	read                *rate.Limiter
+	limitsLAN           atomicBool
+	deviceReadLimiters  *sync.Map
+	deviceWriteLimiters *sync.Map


sync.Map is Go 1.9 and we're currently targeting 1.8. This is the build failure reported by the tests.

Although we could bump our requirements I think there should be little enough churn on these that we can actually use regular maps and locking...

…with rwlocking, added tests

qepasa · 2017-12-27T11:41:13Z

Hello everyone. Sorry for delay with coding. I tried to keep your suggestions in mind. If there is anything you would like to be done differently I'll make sure to implement it faster this time.

imsodin · 2017-12-27T12:17:30Z

lib/connections/limiter.go

+		l.deviceWriteLimiters[value.DeviceID] = rate.NewLimiter(rate.Inf, limiterBurstSize)
+		l.deviceReadLimiters[value.DeviceID] = rate.NewLimiter(rate.Inf, limiterBurstSize)
+	}
+	prev := config.Configuration{Options: config.OptionsConfiguration{MaxRecvKbps: -1, MaxSendKbps: -1}, Devices: devices}


I believe the entire above block isn't necessary, as CommitConfiguration will add all missing devices anyway, so https://github.com/syncthing/syncthing/pull/4603/files#diff-da45d4f40dfa16a9aae760dd650fe79aL35 should be sufficient.

imsodin · 2017-12-27T12:19:16Z

lib/connections/limiter.go

+// Pass pointer to avoid copying. Pointer already points to copy of configuration
+// so we don't have to worry about modifying real config.
+func (lim *limiter) processDevicesConfiguration(from, to *config.Configuration) {
+


imsodin · 2017-12-27T12:33:35Z

lib/connections/limiter.go

+		if v.DeviceID != to.MyID {
+			devicesToRemove[v.DeviceID] = true
+		}
+	}


Could be map[protocol.DeviceID]struct{} and then later instead of = false do delete(devicesToRemove, v.DeviceID). Then you can just iterate over the final map in the end without checking a bool.

AudriusButkevicius · 2017-12-27T11:51:11Z

lib/connections/limiter.go

+	} else {
+		writeLimiter.SetLimit(1024 * rate.Limit(v.MaxSendKbps))
+	}
+}


Now this lives on the lim struct, but doesn't actually use it, meaning it should either be a utility function not part of lim, or use lim and reach into lim.device{Write,Read}Limiters directly.

AudriusButkevicius · 2017-12-27T11:51:28Z

lib/connections/limiter.go

+// Pass pointer to avoid copying. Pointer already points to copy of configuration
+// so we don't have to worry about modifying real config.
+func (lim *limiter) processDevicesConfiguration(from, to *config.Configuration) {
+


Noop space.

AudriusButkevicius · 2017-12-27T12:16:56Z

lib/connections/limiter.go

-func (lim *limiter) newWriteLimiter(w io.Writer, isLAN bool) io.Writer {
-	return &limitedWriter{writer: w, limiter: lim, isLAN: isLAN}
+// Compare read/write limits in configurations
+func (lim *limiter) checkDeviceLimits(from, to config.Configuration) bool {


Again, doesn't need to live on lim.
Also the function name doesn't explain well what this does. Should probably be called deviceLimitsChanged() and invert what it returns.

AudriusButkevicius · 2017-12-27T12:28:58Z

lib/connections/limiter.go

+	}
+	// len(from.Devices) == len(to.Devices) so we can do range from.Devices
+	for i := range from.Devices {
+		if from.Devices[i].DeviceID != to.Devices[i].DeviceID {


This is a nit-pick, but we don't really need to rely on the device order here. It's possible that rates aren't changed, just the order gets changed.

There should be some sort of cfg.DeviceMap() or something like that utility function.

Yes, that should be moved from model to config: https://github.com/syncthing/syncthing/blob/master/lib/model/model.go#L2569
That also makes it easier to remove limiters, just treat it like folders in models CommitConfiguration.

AudriusButkevicius · 2017-12-27T12:33:43Z

lib/connections/limiter.go

+// so we don't have to worry about modifying real config.
+func (lim *limiter) processDevicesConfiguration(from, to *config.Configuration) {
+
+	devicesToRemove := make(map[protocol.DeviceID]bool)


This could be map[DeviceID]struct{}, and devicesToRemove[x] = false is essentially delete(devicesToRemove, x)

Or better would be:

for dev in to.Devices { seen[dev] = struct{}{} } for dev in from.Devices { if _, ok := seen[dev]; !ok { lim.deleteLimiters(dev) } }

AudriusButkevicius · 2017-12-27T12:38:08Z

lib/connections/limiter.go

@@ -73,22 +217,25 @@ func (lim *limiter) CommitConfiguration(from, to config.Configuration) bool {

 	lim.limitsLAN.set(to.Options.LimitBandwidthInLan)

+	// Delete, add or update limiters for devices
+	lim.processDevicesConfiguration(&from, &to)


Why is this a pointer?

AudriusButkevicius · 2017-12-27T12:41:04Z

lib/connections/limiter.go

+				// already marked it as false in devicesToRemove
+				panic("broken symmetry in device read/write limiters")
+			}
+		}


This is too complicated. The only way these things get set are under a lock, so it's very unlikely they get out of sync.
Also, perhaps it's better to get getDevice{Read,Write}Limiter always return a new limiter even if one does not exists (and add it to the map).

I am thinking about a case where a new device gets added, and there is a race between CommitConfiguration here, and us making a connection. It's probably always better wrap every connection in a limiter (and create one if it doesn't exist) and then adjust the rates later.

AudriusButkevicius · 2017-12-27T12:42:45Z

lib/connections/limiter.go

+
+		l.Debugf("okR: %t, okW: %t, prevWriteLim: %d, prevReadLim: %d", okR, okW, previousReadLimit, previousWriteLimit)
+
+		lim.setLimitsForDevice(v, readLimiter, writeLimiter)


Is it not better not to do this if the limits are the same as previously?
As in, move this after the if?

imsodin · 2017-12-27T12:37:24Z

lib/connections/limiter.go

 }

 const limiterBurstSize = 4 * 128 << 10

-func newLimiter(cfg *config.Wrapper) *limiter {
+func newLimiter(deviceID protocol.DeviceID, cfg *config.Wrapper) *limiter {
+


imsodin · 2017-12-27T12:42:35Z

lib/connections/limiter.go

 	if from.Options.MaxRecvKbps == to.Options.MaxRecvKbps &&
 		from.Options.MaxSendKbps == to.Options.MaxSendKbps &&
-		from.Options.LimitBandwidthInLan == to.Options.LimitBandwidthInLan {
+		from.Options.LimitBandwidthInLan == to.Options.LimitBandwidthInLan &&
+		lim.checkDeviceLimits(from, to) {
 		return true
 	}


I think this global check is not very useful as global limits are reset even if just a device changed and whether device change is checked again later on. So I would either not check at all and just reset everything or check the different parameters separately.

imsodin · 2017-12-27T12:50:54Z

lib/connections/limiter.go

+	}
+	// len(from.Devices) == len(to.Devices) so we can do range from.Devices
+	for i := range from.Devices {
+		if from.Devices[i].DeviceID != to.Devices[i].DeviceID {


Yes, that should be moved from model to config: https://github.com/syncthing/syncthing/blob/master/lib/model/model.go#L2569
That also makes it easier to remove limiters, just treat it like folders in models CommitConfiguration.

imsodin · 2017-12-27T12:55:22Z

Argh this github reviews... Probably better look at the comments in the diff, this comment view is fractured badly.

Hello everyone. Sorry for delay with coding. I tried to keep your suggestions in mind. If there is anything you would like to be done differently I'll make sure to implement it faster this time.

Even if you weren't occupied with Christmas holidays and family reunions there is no reason at all to apologize for ~1week (and also if longer) followup times after a round of review. Reviewers will nag if it comes to the point of being too quiet ;)

imsodin · 2018-01-08T16:02:53Z

lib/config/config.go

+
+// mapDeviceConfigs returns a map of device ID to device configuration for the given
+// slice of folder configurations.
+func MapDeviceConfigs(devices []DeviceConfiguration) map[protocol.DeviceID]DeviceConfiguration {


I propose func (cfg *Configuration) DeviceMap() map[protocol.DeviceID]DeviceConfiguration.

Agree. And we do this in a few other places (in the model I think), so we should use this instead.

imsodin · 2018-01-08T16:06:21Z

lib/connections/limiter.go

-			from.Devices[i].MaxRecvKbps != to.Devices[i].MaxRecvKbps {
-			return false
+		if fromMap[k].MaxSendKbps != toMap[k].MaxSendKbps ||
+			fromMap[k].MaxRecvKbps != toMap[k].MaxRecvKbps {


No manual line breaks (I was/am a fan of them too, but it's not the Syncthing/Go style ;) ).

imsodin · 2018-01-08T16:35:14Z

lib/connections/limiter.go

+		// Nothing about this device has changed. Start processing next device
+		if okR && okW &&
+			rate.Limit(v.MaxRecvKbps)*1024 == previousReadLimit &&
+			rate.Limit(v.MaxSendKbps)*1024 == previousWriteLimit {


line breaks

imsodin · 2018-01-08T16:38:50Z

lib/connections/limiter.go

+	// to ensure atomic update of configuration
+	lim.mu.Lock()
+	defer lim.mu.Unlock()
+	if deviceLimitsChanged(from, to) {


I would remove this check. You already do the same checks in processDevicesConfiguration and skip any devices which don't change.

imsodin · 2018-01-08T16:40:17Z

lib/connections/limiter.go

+// This function handles removing, adding and updating of device limiters.
+// Pass pointer to avoid copying. Pointer already points to copy of configuration
+// so we don't have to worry about modifying real config.
+func (lim *limiter) processDevicesConfiguration(from, to *config.Configuration) {


Doesn't need to pass configs as pointers.

AudriusButkevicius · 2018-01-08T18:39:02Z

My previous comments on always creating limiters and always wrapping each device connection in two limiters still apply

qepasa · 2018-02-06T16:36:46Z

I added suggested fixes. Thank you guys for patience with this pull request and for quick reviews.

imsodin · 2018-02-06T16:51:35Z

lib/connections/limiter.go

-		readLimitStr := "is unlimited"
-		if dev.MaxRecvKbps > 0 {
-			readLimitStr = fmt.Sprintf("limit is %d KiB/s", dev.MaxRecvKbps)
+		if limitsChanged := lim.setLimits(dev); limitsChanged {


This can just be if lim.setLimits(dev) {.

AudriusButkevicius · 2018-02-06T16:58:51Z

lib/connections/limiter.go

+	lim.mu.Lock()
+	defer lim.mu.Unlock()
+	deviceLimiter := lim.getWriteLimiter(remoteID)
+	return &limitedWriter{writer: w, limiter: lim, deviceLimiter: deviceLimiter, isLAN: isLAN}


The fact that processblabla does locking outside the function, and this does inside, feels wrong.

@AudriusButkevicius We are starting to contradict each other once again x-): My reasoning to suggest this was that the locking now happens in CommitConfiguration and in these newLimited... functions, which are all "top-level", i.e. called "externally" (not all outside of the package, but outside of the limiter code). Also locking the entirety of CommitConfiguration was considered necessary to disallow concurrent change and thus with a single lock the locking cannot happen on get...Limiter anymore (single lock is not a requirement obviously, I just consider it cleaner).

Fine, but why can't we lock lim.mu before calling the newblabla twice in the connection service, this way we lock the same way in both places. Also a function that needs locking before called, we should add the locked word into the name like we do in the model.

Now I see.
@qepasa So locking should happen here instead and a whole bunch of functions needs the Locked suffix: setLimits, new..., get..., processD....

AudriusButkevicius · 2018-02-06T16:58:55Z

lib/connections/limiter.go

+		seen[dev.DeviceID] = struct{}{}
+
+		// Nothing about this device has changed. Start processing next device
+		if limitsChanged := lim.setLimits(dev); limitsChanged {


The variable assignment is pointless, you can straight do if setLimits(dev) {

AudriusButkevicius · 2018-02-06T17:00:40Z

lib/connections/limiter_test.go

+}
+
+func initConfig(t *testing.T) {
+	t.Helper()


Don't need t.helper as you are not asserting anything here

AudriusButkevicius · 2018-02-07T20:33:36Z

lib/connections/limiter_test.go

@@ -169,8 +169,7 @@ func TestAddAndRemove(t *testing.T) {
 	checkActualAndExpected(t, actualR, actualW, expectedR, expectedW)
 }

-func initConfig(t *testing.T) {
-	t.Helper()
+func initConfig() {


This can just return a config, rather than setting some global that leaks between the tests.

AudriusButkevicius · 2018-02-10T18:10:07Z

Implementation wise looks ok to me, needs to resolve merge conflicts and pass the tests (which I am not sure if they are failing due to anything this PR is doing).
Also perhaps needs UI to expose this in device editor.

imsodin · 2018-02-11T11:16:30Z

Also perhaps needs UI to expose this in device editor.

I agree that this is probably a nice enough functionality to get a place there. If @qepasa wants to do it in this PR, cool, but otherwise I think it's fine to leave it in advanced settings and open an enhancement issue for someone else (or qepasa) to do that eventually.

calmh

I'm sure you've already discussed this in the 90+ comments above, but all of this juggling of mutexes and maps and stuff does not seem worth it to me compared to just creating the limiter with the appropriate settings at connection time. Yeah then we can't change the per device rate limit on the fly, but this will almost never happen and we anyway disconnect peers most of the time when we change folders or devices. That would make this whole thing like a four line diff instead.

calmh · 2018-02-11T11:22:50Z

lib/connections/limiter.go

 	"golang.org/x/net/context"
 	"golang.org/x/time/rate"
+	"sync"


This should probably be our lib/sync

calmh · 2018-02-11T11:23:11Z

lib/connections/limiter.go

+	limitsLAN           atomicBool
+	deviceReadLimiters  map[protocol.DeviceID]*rate.Limiter
+	deviceWriteLimiters map[protocol.DeviceID]*rate.Limiter
+	mu                  *sync.Mutex


Just sync.Mutex, no need for a pointer (regardless of which sync package)

calmh · 2018-02-11T11:23:36Z

lib/connections/limiter.go

-		read:  rate.NewLimiter(rate.Inf, limiterBurstSize),
+		write:               rate.NewLimiter(rate.Inf, limiterBurstSize),
+		read:                rate.NewLimiter(rate.Inf, limiterBurstSize),
+		mu:                  &sync.Mutex{},


Adjust here to nothing at all (standard lib sync) or sync.NewMutex() (our sync)

calmh · 2018-02-11T11:25:21Z

lib/connections/limiter.go

-func (lim *limiter) newWriteLimiter(w io.Writer, isLAN bool) io.Writer {
-	return &limitedWriter{writer: w, limiter: lim, isLAN: isLAN}
+// This function handles removing, adding and updating of device limiters.
+// Pass pointer to avoid copying. Pointer already points to copy of configuration


Not clear what the pointer comments are about. The configuration parameters are not pointers.

calmh · 2018-02-11T11:29:20Z

lib/connections/service.go

@@ -265,8 +265,10 @@ next:
 		// keep up with config changes to the rate and whether or not LAN
 		// connections are limited.
 		isLAN := s.isLAN(c.RemoteAddr())
-		wr := s.limiter.newWriteLimiter(c, isLAN)
-		rd := s.limiter.newReadLimiter(c, isLAN)
+		s.limiter.mu.Lock()


This is unusual, we do don't want to take locks on mutexes inside other objects and then call their ...Locked functions. Wrap in a method.

rd, wr := s.limiter.getLimiters(remoteID, c, isLAN)

imsodin · 2018-02-11T11:50:29Z

[...] all of this juggling of mutexes and maps and stuff does not seem worth it to me compared to just creating the limiter with the appropriate settings at connection time. Yeah then we can't change the per device rate limit on the fly, but this will almost never happen and we anyway disconnect peers most of the time when we change folders or devices. [...]

@calmh Isn't this dropping of connection one of the long-standing issues nobody wants to tackle because it is so entangled? What I mean is that not needing a "complete reset" and thus at least not increasing the entanglement, seems to already provide quite a bit of merit.

calmh · 2018-02-11T13:12:40Z

I don't really think the connection resetting is a big deal, but on the other hand we already have the limiter that listens to config reloads, so fine lets roll with this.

calmh · 2018-02-11T13:17:31Z

LGTM apart from above mutex nits, and that the parameters to take could have better names (l being globalLimiter or something maybe).

Has anyone tested that this actually works?

AudriusButkevicius · 2018-02-13T13:45:51Z

lib/connections/limiter.go

@@ -173,6 +169,14 @@ func (lim *limiter) String() string {
 	return "connections.limiter"
 }

+func (lim *limiter) getLimiters(remoteID protocol.DeviceID, c internalConn, isLAN bool) (io.Reader, io.Writer) {
+	lim.mu.Lock()
+	defer lim.mu.Unlock()


no point using here defer as the scope is small, as this just prevents potential inlining.

AudriusButkevicius

Apart from the last nitpick this looks good to me, if it works :)

AudriusButkevicius · 2018-02-13T20:10:09Z

lib/connections/limiter.go

+
+func (lim *limiter) newLimitedWriterLocked(remoteID protocol.DeviceID, w io.Writer, isLAN bool) io.Writer {
+	deviceLimiter := lim.getWriteLimiterLocked(remoteID)
+	return &limitedWriter{writer: w, limiter: lim, deviceLimiter: deviceLimiter, isLAN: isLAN}


Merge these two two line functions into the callee to reduce the mental stack depth.

qepasa · 2018-02-14T20:32:32Z

I can try to make UI for this but I would prefer it to be a new issue.

AudriusButkevicius · 2018-03-25T20:02:24Z

@imsodin @calmh pls re-review and lets put this to bed.

The person has put an immense amount of effort for dealing with my (our?) comments so far, and we just left him in silence. I am happy with this.

old

calmh · 2018-03-26T10:01:42Z

I have nits to pick, but i'll pick them myself in a followup at some point

* master: (24 commits) lib/model: Prevent warning on request in paused folder (fixes syncthing#4870) docker: Add README from old Docker repo (fixes syncthing#4868) (syncthing#4869) cmd/strelaypoolsrv: Move metric scraping to the server itself (syncthing#4866) gui, man: Update docs & translations assets: Use icon from synctrayzor (ref syncthing#4839) (syncthing#4859) cmd/strelaypoolsrv: Handle portless X-Forwarded-For (syncthing#4856) lib/fs: Don't panic when watching a folder with symlinked root (syncthing#4846) cmd/syncthing: Set Content-Type header regardless of asset location (syncthing#4847) authors: Add fuzzybear3965 cmd/syncthing: Correct --paths in some cases (syncthing#4845) gui, man: Update docs & translations lib/osutil: Use unix lowprio implementation on Android (syncthing#4844) lib/scanner, lib/model: Actually assign version when un-ignoring (fixes syncthing#4841) (syncthing#4842) lib/scanner, lib/model: Actually assign version when un-ignoring (fixes syncthing#4841) (syncthing#4842) cmd/stdiscosrv, vendor: Remove remnants of golang.org/x/net/context (syncthing#4843) lib/connections: Wrong context snuck in somehow build: Add icon & file info to syncthing.exe (syncthing#4839) lib/connections: Slightly refactor limiter juggling lib/connections, lib/config: Bandwidth throttling per remote device (fixes syncthing#4516) (syncthing#4603) gui: Add folder label to global changes, use bootstrap table (fixes syncthing#4828) (syncthing#4838) ...

qepasa added 6 commits December 17, 2017 11:46

lib/connections: Bandwidth throttling per remote device (fixes syncth…

bf78111

…ing#4516)

Merge remote-tracking branch 'upstream/master' into device-throttling

7ca0546

device limiters are now working together with global limiter

e486481

go fmt changes

79f96e5

fixed panic when removing device

9b14e01

switched some logging to debug

c1665b3

AudriusButkevicius reviewed Dec 17, 2017

View reviewed changes

qepasa added 2 commits December 18, 2017 16:11

changed map to sync.Map

d175118

gofmt

1070dd6

AudriusButkevicius reviewed Dec 18, 2017

View reviewed changes

calmh reviewed Dec 24, 2017

View reviewed changes

qepasa added 2 commits December 27, 2017 12:32

updated processing of device configurations, changed sync.Map to map …

b52b326

…with rwlocking, added tests

Merge remote-tracking branch 'upstream/master' into device-throttling

ae58320

imsodin reviewed Dec 27, 2017

View reviewed changes

AudriusButkevicius reviewed Dec 27, 2017

View reviewed changes

imsodin reviewed Dec 27, 2017

View reviewed changes

qepasa added 3 commits January 8, 2018 16:45

adjustments after code review

b6d723d

go fmt changes

a33855f

Merge remote-tracking branch 'upstream/master' into device-throttling

8d637d6

imsodin reviewed Jan 8, 2018

View reviewed changes

clean up rate conversions and remove extra mutex

6967960

imsodin reviewed Feb 6, 2018

View reviewed changes

AudriusButkevicius reviewed Feb 6, 2018

View reviewed changes

code review adjustments

0298b9f

AudriusButkevicius reviewed Feb 7, 2018

View reviewed changes

qepasa added 2 commits February 10, 2018 15:13

code review adjustments

628ae2b

go fmt

56b471f

calmh previously requested changes Feb 11, 2018

View reviewed changes

qepasa added 4 commits February 13, 2018 14:25

code review adjustments

da98cfe

Merge remote-tracking branch 'upstream/master' into device-throttling

a38f5ea

removed unnecessary debug message

e160d1d

go fmt

c3facc1

AudriusButkevicius reviewed Feb 13, 2018

View reviewed changes

defer removed

d03b6ff

AudriusButkevicius approved these changes Feb 13, 2018

View reviewed changes

refactor newLimiter*

291a198

calmh merged commit 2621c6f into syncthing:master Mar 26, 2018

st-review added the frozen-due-to-age Issues closed and untouched for a long time, together with being locked for discussion label Mar 27, 2019

syncthing locked and limited conversation to collaborators Mar 27, 2019


		l.Debugf("okR: %t, okW: %t, prevWriteLim: %d, prevReadLim: %d", okR, okW, previousReadLimit, previousWriteLimit)

		lim.setLimitsForDevice(v, readLimiter, writeLimiter)

lib/connections, lib/config: Bandwidth throttling per remote device (fixes #4516) #4603

lib/connections, lib/config: Bandwidth throttling per remote device (fixes #4516) #4603

Conversation

qepasa commented Dec 17, 2017

Purpose

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsodin commented Dec 18, 2017

Choose a reason for hiding this comment

AudriusButkevicius Dec 18, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AudriusButkevicius Dec 18, 2017 • edited

Choose a reason for hiding this comment

qepasa commented Dec 18, 2017

AudriusButkevicius commented Dec 18, 2017

calmh Dec 24, 2017 • edited

Choose a reason for hiding this comment

qepasa commented Dec 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsodin Dec 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AudriusButkevicius Dec 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsodin commented Dec 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AudriusButkevicius commented Jan 8, 2018

qepasa commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsodin Feb 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AudriusButkevicius commented Feb 10, 2018

imsodin commented Feb 11, 2018

calmh left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imsodin commented Feb 11, 2018

calmh commented Feb 11, 2018

AudriusButkevicius Dec 18, 2017 •

edited

AudriusButkevicius Dec 18, 2017 •

edited

calmh Dec 24, 2017 •

edited

imsodin Dec 27, 2017 •

edited

AudriusButkevicius Dec 27, 2017 •

edited

imsodin commented Dec 27, 2017 •

edited

imsodin Feb 6, 2018 •

edited

calmh left a comment •

edited