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.

Sign up for GitHub

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

Jump to bottom

Update SAI pointer to latest master #408

Merged
merged 2 commits into from Jan 18, 2019
Merged

Update SAI pointer to latest master #408

merged 2 commits into from Jan 18, 2019

Conversation

marian-pritsak
Copy link
Contributor

Signed-off-by: Marian Pritsak marianp@mellanox.com

@marian-pritsak
Update SAI pointer to latest master
eb60ec2 
Signed-off-by: Marian Pritsak <marianp@mellanox.com>
@marian-pritsak
Copy link
Contributor Author

Depends on opencomputeproject/SAI#909

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 14, 2019

overall those changes looks good, please corect the error:
saimetadatatest.c:2140:16: error: array subscript is above array bounds [-Werror=array-bounds]
checked[(int)SAI_OBJECT_TYPE_TABLE_VNET_ENTRY] = SAI_OBJECT_TYPE_TABLE_VNET_ENTRY;
^
saimetadatatest.c:2160:16: error: array subscript is above array bounds [-Werror=array-bounds]
checked[(int)SAI_OBJECT_TYPE_TABLE_TUNNEL_ROUTE_ENTRY] = SAI_OBJECT_TYPE_TABLE_TUNNEL_ROUTE_ENTRY;
^

@marian-pritsak
Use SAI_EXTENSIONS_TYPE_MAX
dfbd428 
Signed-off-by: Marian Pritsak <marianp@mellanox.com>
@lguohan lguohan requested a review from kcudnik January 17, 2019 17:06
kcudnik
kcudnik reviewed Jan 17, 2019
View reviewed changes
lib/inc/sai_redis_internal.h
MUTEX(); \
SWSS_LOG_ENTER(); \
return meta_sai_create_oid( \
(sai_object_type_t)SAI_OBJECT_TYPE_ ## OBJECT_TYPE, \
Copy link
Collaborator

Choose a reason for hiding this comment

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

is cast needed because extensions object types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it is

kcudnik
kcudnik reviewed Jan 17, 2019
View reviewed changes
lib/inc/sai_redis_internal.h
@@ -184,7 +184,7 @@
sai_status_t redis_clear_ ## object_type ## _stats( \
_In_ sai_object_id_t object_type ## _id, \
_In_ uint32_t number_of_counters, \
_In_ const sai_ ## object_type ## _stat_t *counter_ids) \
_In_ const sai_stat_id_t *counter_ids) \
Copy link
Collaborator

Choose a reason for hiding this comment

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

i preffered to have separate type for each stats, this gave extra check for type, so user would not pass different stats for given objects at compile time
is there any reason you are changing this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm afraid it's not possible to keep the old type, because this function then cannot be casted to a function pointer of the SAI API, that is using sai_stat_id_t.

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, each new function pointer needs to be created for each stat, and this is how it was so far

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now in SAI stat functions look like this:

typedef sai_status_t (*sai_clear_bfd_session_stats_fn)(
        _In_ sai_object_id_t bfd_session_id,
        _In_ uint32_t number_of_counters,
        _In_ const **sai_stat_id_t** *counter_ids);

So if I leave macros as is, it will fail to compile with following error:

error: invalid conversion from 'sai_status_t (*)(sai_object_id_t, uint32_t, const sai_bfd_session_stat_t*) {aka int (*)(long unsigned int, unsigned int, const _sai_bfd_session_stat_t*)}' to 'sai_clear_bfd_session_stats_fn {aka int (*)(long unsigned int, unsigned int, const unsigned int*)}' [-fpermissive]

Maybe I'm missing your point, but doesn't redis SAI API have to look exactly like SAI api?
These changes to sairedis are done with an assumption that SAI API is taken as is.

kcudnik
kcudnik reviewed Jan 17, 2019
View reviewed changes
syncd/syncd_flex_counter.cpp
@@ -799,13 +799,13 @@ void FlexCounter::collectCounters(
status = sai_metadata_sai_buffer_api->get_ingress_priority_group_stats(
priorityGroupId,
static_cast<uint32_t>(priorityGroupCounterIds.size()),
priorityGroupCounterIds.data(),
(const sai_stat_id_t *)priorityGroupCounterIds.data(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

from my perspective it would be better to have each stats accepting separate stat's enum but i guess this is already changed on the SAI api's

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, SAI APIs were changed, so I had to do this cast.

kcudnik
kcudnik reviewed Jan 17, 2019
View reviewed changes
vslib/src/tests.cpp
@@ -297,7 +297,7 @@ void test_set_stats_via_redis()
sai_port_stat_t ids[1] = { SAI_PORT_STAT_IF_IN_ERRORS };
uint64_t counters[1] = { 0 } ;

SUCCESS(sai_metadata_sai_port_api->get_port_stats(ports.at(0), 1, ids, counters));
SUCCESS(sai_metadata_sai_port_api->get_port_stats(ports.at(0), 1, (const sai_stat_id_t *)ids, counters));
Copy link
Collaborator

Choose a reason for hiding this comment

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

previously we had each type for each stats, and cast was not necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I'm adapting to the new SAI APIs.
I don't know what was the reason behind changing all of them to the same type, but now this is a necessary change.

kcudnik
kcudnik approved these changes Jan 17, 2019
View reviewed changes
Copy link
Collaborator

@kcudnik kcudnik left a comment

Choose a reason for hiding this comment

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

overall looks good, can you respond on comments i gave ?

@marian-pritsak
Copy link
Contributor Author

@kcudnik I agree with you that changes related to sai_stat_id_t seem odd, but they are necessary because sairedis compilation is constrained by SAI heders definition. I would suggest revisiting this uniform declaration of stat functions using sai_stat_id_t because it breaks a lot of code.

@kcudnik
Copy link
Collaborator

kcudnik commented Jan 18, 2019

yes, agree

@lguohan lguohan merged commit 8793562 into sonic-net:master Jan 18, 2019
zhenggen-xu pushed a commit to zhenggen-xu/sonic-sairedis that referenced this pull request Apr 18, 2019
@marian-pritsak
Update SAI pointer to latest master (sonic-net#408)
87d013f 
* Update SAI pointer to latest master

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

* Use SAI_EXTENSIONS_TYPE_MAX

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

Conflicts:
	SAI
zhenggen-xu pushed a commit to zhenggen-xu/sonic-sairedis that referenced this pull request Jun 18, 2019
@marian-pritsak @zhenggen-xu
Update SAI pointer to latest master (sonic-net#408)
dc4ccfe 
* Update SAI pointer to latest master

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

* Use SAI_EXTENSIONS_TYPE_MAX

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

Conflicts:
	SAI
lguohan pushed a commit to lguohan/sonic-sairedis that referenced this pull request Jun 24, 2019
@marian-pritsak @lguohan
Update SAI pointer to latest master (sonic-net#408)
905b580 
* Update SAI pointer to latest master

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

* Use SAI_EXTENSIONS_TYPE_MAX

Signed-off-by: Marian Pritsak <marianp@mellanox.com>
qiluo-msft pushed a commit that referenced this pull request Jun 26, 2019
@marian-pritsak @qiluo-msft
Update SAI pointer to latest master (#408)
4910be8 
* Update SAI pointer to latest master

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

* Use SAI_EXTENSIONS_TYPE_MAX

Signed-off-by: Marian Pritsak <marianp@mellanox.com>
qiluo-msft pushed a commit that referenced this pull request Jun 29, 2019
@marian-pritsak @qiluo-msft
Update SAI pointer to latest master (#408)
9084920 
* Update SAI pointer to latest master

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

* Use SAI_EXTENSIONS_TYPE_MAX

Signed-off-by: Marian Pritsak <marianp@mellanox.com>
volodymyrsamotiy pushed a commit to volodymyrsamotiy/sonic-sairedis that referenced this pull request Jul 30, 2019
@marian-pritsak
Update SAI pointer to latest master (sonic-net#408)
a2c3422 
* Update SAI pointer to latest master

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

* Use SAI_EXTENSIONS_TYPE_MAX

Signed-off-by: Marian Pritsak <marianp@mellanox.com>
pettershao-ragilenetworks pushed a commit to pettershao-ragilenetworks/sonic-sairedis that referenced this pull request Nov 18, 2022
@marian-pritsak @lguohan
Update SAI pointer to latest master (sonic-net#408)
74cc572 
* Update SAI pointer to latest master

Signed-off-by: Marian Pritsak <marianp@mellanox.com>

* Use SAI_EXTENSIONS_TYPE_MAX

Signed-off-by: Marian Pritsak <marianp@mellanox.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kcudnik kcudnik kcudnik approved these changes

@lguohan lguohan lguohan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@marian-pritsak @kcudnik @lguohan