On 2/28/23 14:17, Chris Mi wrote:
> On 2/24/2023 4:46 AM, Ilya Maximets wrote:
>> On 2/23/23 12:27, Chris Mi wrote:
>>> Create a unique group ID to map the sFlow info when offloading sample
>>> action to TC. When showing the offloaded datapath flows, translate the
>>> group ID from TC sample action to sFlow info using the mapping.
>>>
>>> Signed-off-by: Chris Mi <[email protected]>
>>> Reviewed-by: Roi Dayan <[email protected]>
>>> ---
>>> lib/netdev-offload-tc.c | 244 +++++++++++++++++++++++++++++++++++++---
>>> lib/netdev-offload.h | 9 ++
>>> lib/tc.c | 62 +++++++++-
>>> lib/tc.h | 11 +-
>>> 4 files changed, 309 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
>>> index d7901fa68..ec4813147 100644
>>> --- a/lib/netdev-offload-tc.c
>>> +++ b/lib/netdev-offload-tc.c
>>> @@ -2099,6 +2118,172 @@ parse_match_ct_state_to_flower(struct tc_flower
>>> *flower, struct match *match)
>>> }
>>> }
>>>
>>> +static int
>>> +parse_userspace_attributes(const struct nlattr *actions,
>>> + struct offload_sflow *sflow)
>>> +{
>>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>>> + const struct nlattr *nla;
>>> + unsigned int left;
>>> +
>>> + NL_NESTED_FOR_EACH_UNSAFE (nla, left, actions) {
>>> + if (nl_attr_type(nla) == OVS_USERSPACE_ATTR_USERDATA) {
>>> + struct user_action_cookie *cookie;
>>> +
>>> + cookie = (struct user_action_cookie *) nl_attr_get(nla);
>>> + if (cookie->type == USER_ACTION_COOKIE_SFLOW) {
>> This pointer is likely misaligned.
> But the cookie is passed from ofproto. I believe it is aligned when
> allocating it.
Netlink attributes are always only 4-byte aligned. So, it could have
been misaligned if struct user_action_cookie had a higher alignment
requirement. Fortunately, it doesn't look like it.
>>> diff --git a/lib/netdev-offload.h b/lib/netdev-offload.h
>>> index e332555ee..fb35d77c4 100644
>>> --- a/lib/netdev-offload.h
>>> +++ b/lib/netdev-offload.h
>>> @@ -81,6 +81,15 @@ struct offload_info {
>>> bool tc_modify_flow_deleted; /* Indicate the tc modify flow put success
>>> * to delete the original flow. */
>>> odp_port_t orig_in_port; /* Originating in_port for tnl flows. */
>>> +
>>> + /* To avoid passing too many parameters when offloading sample action,
>>> + * aggregate them here.
>>> + */
>>> + struct {
>>> + const struct flow_tnl *tnl; /* Tunnel info. */
>>> + const ovs_u128 *ufid; /* Ufid. */
>>> + uint32_t gid; /* Generated gid. */
>>> + } sample;
>> I'm not sure why this structure exists. It is used internally
>> in netdev-offload-tc, so should not be public.
>>
>> The offload_info structure is for communication between layers,
>> not to store the local information.
> I removed all sample members in offload_info and introduced struct
> sample_param.
> If it is not ok. I think we have to add more params in several functions that
> already have
> too many params.
The point was that they shouldn't be part of struct offload_info.
How they are stored inside netdev-offload-tc is a different story.
(I didn't look at the updated code yet.)
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
