serialize map access #97
Conversation
ofctrl/fgraphSwitch.go
Outdated
| @@ -105,6 +105,9 @@ func (self *OFSwitch) DefaultTable() *Table { | |||
|
|
|||
| // Return a output graph element for the port | |||
| func (self *OFSwitch) OutputPort(portNo uint32) (*Output, error) { | |||
| self.portMux.Lock() | |||
| defer self.portMux.Unlock() | |||
|
|
|||
| if self.outputPorts[portNo] != nil { | |||
There was a problem hiding this comment.
Can we change this to the more standard:
if value, ok := self.outputPorts[portNo]; ok {
return value, nil
}😄 ?
| @@ -35,6 +36,7 @@ type OFSwitch struct { | |||
| dropAction *Output | |||
| sendToCtrler *Output | |||
| normalLookup *Output | |||
| portMux sync.Mutex | |||
| outputPorts map[uint32]*Output | |||
There was a problem hiding this comment.
Any reason to not use concurrent map?
https://github.com/contiv/ofnet/blob/master/ofctrl/ofSwitch.go#L41
There was a problem hiding this comment.
Good idea!
Go actually has a build in concurrent map now: https://golang.org/pkg/sync/#Map
(but we can't use it until we're compiling with 1.9)
There was a problem hiding this comment.
@rchirakk: This is a good idea. Can you modify the code to use the concurrent map, please?
There was a problem hiding this comment.
didn't use concurrent map because
- to keep the change to minimum.
- concurrent map requires string as key
- concurrent map is implemented as 32 maps with mutex, overkill for this case.
There was a problem hiding this comment.
@rchirakk: That makes sense. Thank you.
|
@rchirakk: Can you write a test which triggers the issue you've discovered, please? |
|
@unclejack it was detected in CI contiv/netplugin#972 |
|
@rchirakk: Ok, I'll take another look tomorrow. |
Signed-off-by: Ranjith <[email protected]>
|
👍 |
Signed-off-by: Ranjith [email protected]