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

Process flex counters requests in separate thread #483

Merged
merged 2 commits into from
Jul 17, 2019

Conversation

kcudnik
Copy link
Collaborator

@kcudnik kcudnik commented Jul 1, 2019

No description provided.

@kcudnik kcudnik requested a review from lguohan July 1, 2019 10:32
@kcudnik kcudnik requested a review from wendani July 1, 2019 10:35
@lguohan
Copy link
Contributor

lguohan commented Jul 2, 2019

@wendani , can you take a look?

{
// event was not successfully processed, put it again to the queue

pushFlexCounterEvent(kco);
Copy link
Contributor

Choose a reason for hiding this comment

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

This may upset the event sequence in the processing failure case. The processing failure case, however, is the problem you would like to solve.

For the flex counter table in particular, it typically contains the counter id list that is expected to remain static and unchanged after SET. We should not see any two operations on the same key in the queue at the same time, although there leaves the possibility of running into such a sequence problem.

If we enqueue at the front and retry, we can avoid the event sequence problem. However, we may experience head of line blocking until the event processing retry succeeds.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i don't get the problem you described here, all those issues with counters are present here which we want to workaround because they are executed on the same channel and can be picked up by select independently, and i stated at the beginning that subscribe to the counter is broken design and of course you can came up with scenario that can break even this workaround, since you need to wait for RIF to be created and for example potentially not destroyed and created again, which all would be solved when syncd would become synchronous on which i already send PR

}
}

sleep(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we wake up every 1 second only if there is a processing event failure? In other cases, we wake up on an enqueue event. Flex counter table event should be very rare after the initialization.

Copy link
Collaborator Author

@kcudnik kcudnik Jul 4, 2019

Choose a reason for hiding this comment

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

this is actually overkill to make other thread and condition variable to wait for specific event, also making that event, also don't guarantee that this event will be processed after second select will create rif port, so there must be wait somewhere anyway.
Even if those counter events are rare, this is generic case, i don't know whether orchagent will want to create/destroy next counter in 2 days of running or any other time

@lguohan
Copy link
Contributor

lguohan commented Jul 16, 2019

@kcudnik , can you resolve the conflict?

@lguohan lguohan merged commit bd33da9 into sonic-net:master Jul 17, 2019
kcudnik added a commit to kcudnik/sonic-sairedis that referenced this pull request Aug 19, 2019
pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
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.

3 participants