Add Buffer Config Manager #417
Conversation
Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>
|
@yxieca to review |
cfgmgr/buffermgr.cpp
Outdated
| doCableTask(fvField(i), fvValue(i)); | ||
| } | ||
| if (table_name == CFG_PORT_TABLE_NAME && fvField(i) == "speed") | ||
| { |
There was a problem hiding this comment.
Will the field ever != "speed"?
If yes, why ignoring them is correct? Please at least add comment to state reason why it is safe to ignore them.
There was a problem hiding this comment.
yes, we receive updates for other fields as well. Actually every time at least one field in the table gets updated, all subscribers receive notifications about all fields. buffer manager is interested in speed only. I'll add a comment.
There was a problem hiding this comment.
Thanks for the explanation!
|
|
||
| namespace swss { | ||
|
|
||
| #define INGRESS_LOSSLESS_PG_POOL_NAME "ingress_lossless_pool" |
There was a problem hiding this comment.
Is assuming ingress lossless pg pool name the right thing to do?
Can this code support both shared ingress pg pool and separate pg pools for lossless and lossy traffic?
There was a problem hiding this comment.
currently we support buffers update for profiles which use ingress lossless pools only
There was a problem hiding this comment.
Does it prevent shared pg pool?
There was a problem hiding this comment.
I just had a discussion with Guohan, this change will affect Broadcom platform in negative way.
On some Broadcom platforms, due to the MMU memory size limitation, we have to allocate a shared buffer pool for all pgs (lossy and lossless), and create 2 buffer profiles on top of the shared pg pool.
I think by hard coding the pg pool name, here is a potential risk for Broadcom platforms that requires shared buffer pool.
There is a buffers.json file checked in for Broadcom TH chip: https://github.com/Azure/sonic-swss/blob/master/swssconfig/sample/th.64ports.buffers.json
Please make sure your code works on this scenario as well.
Thanks,
Ying
There was a problem hiding this comment.
ok, pool_name is yet another candidate to get to the new table with QOS/BUFFER configuration (besides pg/queue index to generate profile for)
But existing implementation will still work with this platform.
Since the configuration is static no new profiles will be generated on speed change.
There was a problem hiding this comment.
I've updated config for Force10-S6100 (previously by mistake used td2.32ports.buffers.json for it)
There was a problem hiding this comment.
I see. thanks for the update.
| namespace swss { | ||
|
|
||
| #define INGRESS_LOSSLESS_PG_POOL_NAME "ingress_lossless_pool" | ||
| #define LOSSLESS_PGS "3-4" |
There was a problem hiding this comment.
By defining lossless pgs to 3-4 in code, do we lose the flexibility of choosing any pg as lossless?
I know that in practice we are always have been using 3-4 as lossless pgs, but losing the capability of changing is a bit concerning.
There was a problem hiding this comment.
theoretically PGs numbers can be fetched from the PORT_QOS_MAP:pfc_enable.
@lguohan, can we rely on the fact that queues specified in PORT_QOS_MAP:pfc_enable ("3,4") will always be the ones for which we need to update ingress lossless profile on speed change?
There was a problem hiding this comment.
We had a discussion, we decided to move forward with this change for now and revisit this area and remove the hard coded pg numbers in the future.
| } | ||
| if (ret == Select::TIMEOUT) | ||
| { | ||
| buffmgr.doTask(); |
There was a problem hiding this comment.
Do you think calling drain() makes more sense?
There was a problem hiding this comment.
Probably, I'll check. Just used the solution similar to initmgrd.
There was a problem hiding this comment.
doTask() in base class calls drain:
void Orch::doTask()
{
for(auto &it : m_consumerMap)
{
it.second->drain();
}
}
There was a problem hiding this comment.
I see. Thanks for digging it up!
There was a problem hiding this comment.
I think doTask is virtual function and the call will be dispatched to derived class. but calling doTask() is fine.
There was a problem hiding this comment.
buffermgr does not override doTask(void) so we will go via Orch::doTask() anyway
|
I would really like to see swss integration tests on this PR. |
Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>
Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>
Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>
Signed-off-by: Andriy Moroz <c_andriym@mellanox.com>
Conflicts: cfgmgr/Makefile.am
|
VS test requires changes in VS docker (sonic-net/sonic-buildimage@ff8332f) which require changes from this PR |
|
Changes with the test reverted and moved to the separate PR |
|
It appears that this review might need to be updated before it can be merged? |
Signed-off-by: Andriy Moroz c_andriym@mellanox.com
What I did
PG Buffers update on port speed change
Why I did it
Added buffer manager to track port speed and update PG profiles
How I verified it
Checked PG profiles on port on different speed set
Manually tested typical setups configuration (t1, t1-lag, t0)
Details if related
See: https://github.com/Azure/SONiC/wiki/Run-Time-Buffers-Configuration-Update-design
NOTE1:
Dynamic profile deletion if not used is not implemented (requires more tests and investigation)
Does not affect main functionality. Suggest to implement it as an improvement.
NOTE2:
speed+cable-to-profile mapping implemented in .ini file not in an additional DB as described in the design. (will be in sonic-buildimage PR)