Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Session add/lookup using sync map #67

Merged
merged 8 commits into from
Jan 24, 2020
Merged

Conversation

krsna1729
Copy link
Contributor

@krsna1729 krsna1729 commented Jan 22, 2020

This PR addresses #62

The number of places session lookup touches is quite a bit. Things i am fuzzy around -

  • There is nothing to connect Session and the Conn, so we have to pass TEIDSessionMap OR we can add to Session struct (needed in NewSession)
  • TEID being unique per Conn (this PR) vs Conn+IfType (existing impl)
  • Need for mutexes around sync.Map
  • Test changes made

@krsna1729
Copy link
Contributor Author

Woops, need to update the examples

@krsna1729
Copy link
Contributor Author

Do let me know if this is a good approach.

@krsna1729
Copy link
Contributor Author

Package tests and benchmarks

ok  	github.com/wmnsk/go-gtp/v2	1.009s	coverage: 38.1% of statements
Success: Tests passed.
goos: linux
goarch: amd64
pkg: github.com/wmnsk/go-gtp/v2
BenchmarkAddSessionExist0-88      	   95989	     12159 ns/op	   17240 B/op	      20 allocs/op
BenchmarkAddSessionExist100-88    	   91687	     12796 ns/op	   17240 B/op	      20 allocs/op
BenchmarkAddSessionExist1K-88     	   99243	     10516 ns/op	   17240 B/op	      20 allocs/op
BenchmarkAddSessionExist10K-88    	  116122	      8886 ns/op	   17240 B/op	      20 allocs/op
PASS
ok  	github.com/wmnsk/go-gtp/v2	5.731s
Success: Benchmarks passed.

Signed-off-by: Saikrishna Edupuganti <saikrishna.edupuganti@intel.com>
@krsna1729 krsna1729 changed the title [WIP]: Session add/lookup using sync map Session add/lookup using sync map Jan 23, 2020
Signed-off-by: Saikrishna Edupuganti <saikrishna.edupuganti@intel.com>
Signed-off-by: Saikrishna Edupuganti <saikrishna.edupuganti@intel.com>
Copy link
Owner

@wmnsk wmnsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Could you check my inline comments?

v2/conn.go Outdated
defer c.mu.Unlock()
var teid uint32
// TODO: Name max retries
for i := 0; i < 100; i++ {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

100 times of retry seems insufficient in case of millions of subscribers exist. How about using 0xffff instead of 100 considering the unluckiest situation(only one value remaining unassigned)? Also, logging to let user know it's taking time at some point(e.g., when i = 1000, 10000, ...) can be helpful.

v2/conn.go Outdated
Comment on lines 54 to 56
*IMSISessionMap

*TEIDSessionMap
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these fields should not be exported as it's no use for manipulating from outside the package(rather, might be dangerous), and the new Session() method satisfies the purpose of removed Session field.

Signed-off-by: Saikrishna Edupuganti <saikrishna.edupuganti@intel.com>
Signed-off-by: Saikrishna Edupuganti <saikrishna.edupuganti@intel.com>
Signed-off-by: Saikrishna Edupuganti <saikrishna.edupuganti@intel.com>
v2/conn.go Outdated Show resolved Hide resolved
v2/conn.go Outdated Show resolved Hide resolved
Signed-off-by: Saikrishna Edupuganti <saikrishna.edupuganti@intel.com>
Signed-off-by: Saikrishna Edupuganti <saikrishna.edupuganti@intel.com>
Copy link
Owner

@wmnsk wmnsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for updating. Now everything looks good to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants