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

[syncd] Use hash as queue for fdb notifications #1060

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kcudnik
Copy link
Collaborator

@kcudnik kcudnik commented Jun 8, 2022

This will drop multiple fdb notifications in case of flood but will
preserve last one

This will drop multiple fdb notifications in case of flood but will
preserve last one
@lguohan
Copy link
Contributor

lguohan commented Jun 13, 2022

this could drop the aging event, what would be the impact to the orchagent.

@kcudnik
Copy link
Collaborator Author

kcudnik commented Jun 13, 2022

i'm not sure if OA is ready for dropped events, like dropping AGE event, we would need to discuss with @prsunny

@gechiang
Copy link
Contributor

gechiang commented Jun 20, 2022

Currently the MAC move storm looks as following:

  1. leant on port A
  2. aged from port A
  3. learnt on port B
  4. aged from port B
  5. learnt from port A
  6. aged from port A
  7. learnt from port B
  8. Aged from port B
  9. Learnt from port A

My understanding is that if the last event is:
"9", then it will be treated as NOP since MAC was already learnt on port A. So in between all the moves were ignored as the start and end are the same. Not sure if any protocol has a need to know MAC did move but end up at the same port at then end. probably not a big issue for this scenario.
"8", I think the current code does not pay attention to the port and will go ahead honor the Age and remove the MAC. which if that is the case it should also be fine as the final state for this MAC should be removed from MAC table.
"7", I think the current code would ignore this new learnt event to port B since the MAC was already learnt on port A. But MAC should be on port B. So this will post an issue...
"6", I think this would be fine. MAC would be removed correctly.
"5", This is similar to step "9" which we already covered earlier.

So, in all scenarios where the MAC move storm ended at step "7" would be the issue that needs to be solved.
Also, I do not think that the MAC checking does not involve port check. it only checks for that given VLAN... I think this is not correct but not sure if this is something we need to address it now or leave it at a future PR improvement... but fixing case ending at "7" may require to validate MAC event on both VLAN and Port...
@prsunny please help confirm if my listed scenarios are correct...

@prsunny
Copy link
Contributor

prsunny commented Jun 21, 2022

agree with @gechiang that currently orchagent does not handle learn event on another port. From the current implementation, other cases are expected to be handled.

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

Successfully merging this pull request may close these issues.

4 participants