Opened 9 years ago

Closed 8 years ago

#4402 closed Bug (duplicate)

Transmission Bandwidth allocation getting overflows

Reported by: sathya.phoenix Owned by: jordan
Priority: Normal Milestone: 2.60
Component: libtransmission Version: 2.33
Severity: Normal Keywords: bandwidth allocation
Cc: sathya@…, boondoklife@…

Description

The allocateBandwidth function was computing the fraction of desired speed it want to use in the next pulse by first multiplying and then dividing by a big number. This was creating overflows once the desired speed was set to close to 8193 KB/s .. 8193*1000*500/1000 was overflowing and this would totally shut down the up/down speed effectively close to 0 .. I fixed this and in general changed the desiredSpeed to uint64_t and changed it in all the associated places..

One of my use cases at work, I am using transmission to go till huge speeds.. I am getting some bottlenecks, one of them being this. Any help would be appreciated.

To trigger the bug:

When no limits are set, you can observe it transferring at huge speeds more than 20MB/s but when you set the speed limit, you can observer the following pattern.

8,000 => 8,000; 9,000 => 400; 10,000 => 1,376; 16,000 => 7,236; 32,000 => 6,084; 40,000 => 5507

The left column is the limit i set to in KB/s and right column is the speed at which i see the sending client choked.

You can also reproduce this by using transmission-remote and trying to set the limits. same behaviour noticed. I used the python transmission rpc.

Attachments (3)

overflow.patch (18.7 KB) - added by sathya.phoenix 9 years ago.
The patch to fix this and port the maximum limits to 64 bit .. int is 32 bit by default in all machines i guess
overflow-2.diff (22.3 KB) - added by jordan 9 years ago.
4402-uint-bpsoverflow.patch (18.3 KB) - added by gvdl 9 years ago.
32bit clean patch as requested

Download all attachments as: .zip

Change History (21)

Changed 9 years ago by sathya.phoenix

The patch to fix this and port the maximum limits to 64 bit .. int is 32 bit by default in all machines i guess

comment:1 Changed 9 years ago by sathya.phoenix

gentle bounce on this one ..

comment:2 Changed 9 years ago by jordan

  • Milestone changed from None Set to 2.40
  • Owner set to jordan
  • Status changed from new to assigned

thanks for the patch :)

comment:3 Changed 9 years ago by jordan

  • Component changed from Transmission to libtransmission

comment:4 Changed 9 years ago by jordan

Sathya,

attached is a revision that propagates the use of uint64_t out a little further -- peer-io.c's getDesiredOutputBufferSize() uses tr_bandwidthGetPieceSpeedBps(), and tr_peerIoGetWriteBufferSpace uses getDesiredOutputBufferSize(). Likewise, tr_webseedGetSpeed_Bps() now uses uint64_t.

Does this look correct to you?

Changed 9 years ago by jordan

comment:5 Changed 9 years ago by sathya.phoenix

Jordan,

looks fine to me. Thanks.

I am also interested in adding the "building a .torrent" feature to transmission . Any reason why this hasnt been there till now ? When I wanted to distribute huge data, I had to manually build the torrent using ctorrent and then add it in transmission and pass a flag to skip verification so that it can start seeding the data(which otherwise would spend an hour checksumming something which was built by ctorren). Think if it had a building the torrent feature, this would be much simplified. Or am I unaware of some feature of transmission which already does this?

Sathya.

comment:6 Changed 9 years ago by livings124

Transmission allows you to create torrent files, and please keep it to one issue per ticket.

comment:7 Changed 9 years ago by trtrmitya

Sathya, you can create a .torrent file using transmission-create command.

How do you "pass a flag to skip verification"? I also want transmission to skip verification of newly added torrents.

Thanks.

comment:8 Changed 9 years ago by livings124

  • Milestone changed from 2.40 to None Set

comment:9 Changed 9 years ago by sathya.phoenix

@trttmitya : I had a --skip-verify argument passed to the command line argument of transmission-daemon and after that any torrent added , I changed the source code so that the verififcation doesnt happpen or a dummy verification is done .. like setting the timestamp on the pieces manually etc . It was very non-standard and tecnically shoudlnt be done this way ... but it worked for me and saved hours ..

Does create-torrent work through RPC .. that is what I am more interested in ...

comment:10 Changed 9 years ago by trtrmitya

Guys,

Is there any reason why this patch can not be included into next transmission release?

Problem seems rather severe.

Thanks!

comment:11 follow-up: Changed 9 years ago by jordan

The reason I haven't pushed this through is that I'm not convinced that it's the right approach, but I haven't gone back to re-evaluate.

In short, I think we ought to try to resolve the problem without changing the public API. Both of our patches add uint64_t types to bytes-per-second arguments in the public API, and at the heart of it I really don't think that makes sense.

So IMO what should happen on this ticket is the smallest set of code that can overflow should be addressed, and its private API adapted accordingly. I don't think anything outside the libtransmission/ directory should notice the change.

comment:12 Changed 9 years ago by trtrmitya

Would be nice to fix that, kinda annoying bug. Thanks.

comment:13 in reply to: ↑ 11 ; follow-up: Changed 9 years ago by gvdl

Replying to jordan:

In short, I think we ought to try to resolve the problem without changing the public API. Both of our patches add uint64_t types to bytes-per-second arguments in the public API, and at the heart of it I really don't think that makes sense.

I'm looking at some localised patches for this bug.

But if we don't go to 64bit we will be limited to 4E9 bytes (I'm assuming Bps is bytes per second). That is the equivalent of 35GigE. We may just have to bite the bullet.

Changed 9 years ago by gvdl

32bit clean patch as requested

comment:14 in reply to: ↑ 13 Changed 9 years ago by x190

Replying to gvdl:

Replying to jordan:

In short, I think we ought to try to resolve the problem without changing the public API. Both of our patches add uint64_t types to bytes-per-second arguments in the public API, and at the heart of it I really don't think that makes sense.

I'm looking at some localised patches for this bug.

But if we don't go to 64bit we will be limited to 4E9 bytes (I'm assuming Bps is bytes per second). That is the equivalent of 35GigE. We may just have to bite the bullet.

Please see the following forum thread as it may be a variation of this issue.

https://forum.transmissionbt.com/viewtopic.php?f=2&t=13459#p60468

comment:15 Changed 9 years ago by jordan

  • Milestone changed from None Set to 2.60

very nice patch!

r13361

comment:16 Changed 9 years ago by jordan

  • Resolution set to fixed
  • Status changed from assigned to closed

comment:17 Changed 8 years ago by boondoklife

  • Cc boondoklife@… added
  • Resolution fixed deleted
  • Status changed from closed to reopened

This is listed as closed, was it supposed to be resolved in 2.60? I am still seeing this issue with 2.60 (13375) and can reproduce it all the time. I am able to play nice with the limits until I choose something higher than 8589Kbps; once this line is crossed the bucket seems to reset, 8590Kbps effectively limits the traffic to 0Kbps.

comment:18 Changed 8 years ago by jordan

  • Resolution set to duplicate
  • Status changed from reopened to closed

This appears to be the same issue as #5267 ("Effectively one cannot set limit upper than 8589 kB/s.") which was fixed with a really nice-one liner contributed by user 'const' :)

Note: See TracTickets for help on using tickets.