Re: [PATCH v4 04/10] flow_offload: allow user to offload tc action to net device

On 2021-11-24 08:47, Baowen Zheng wrote:

On November 24, 2021 7:40 PM, Jamal Hadi Salim wrote:

quoted

On 2021-11-23 21:59, Baowen Zheng wrote:

quoted

Sorry for reply this message again.
On November 24, 2021 10:11 AM, Baowen Zheng wrote:

quoted

On November 24, 2021 3:04 AM, Jamal Hadi Salim wrote:

[..]

quoted

quoted

quoted

BTW: shouldnt extack be used here instead of returning just -EINVAL?
I didnt stare long enough but it seems extack is not passed when
deleting from hardware? I saw a NULL being passed in one of the patches.

Maybe I misunderstand what you mean previously, when I look through
the implement in flow_action_init, I did not found we use the extack to

make a log before return -EINVAL.

quoted

So could you please figure it out? Maybe I miss something or misunderstand

again.

I mean there are maybe 1-2 places where you called that function
flow_action_init() with extack being NULL but the others with legitimate extack.
I pointed to offload delete as an example. This may have existed before your
changes (but it is hard to tell from just eyeballing patches); regardless it is a
problem for debugging incase some delete offload fails, no?

Yes, you are right, for the most of the delete scenario, the extack is NULL since
The original implement to delete the action does not include an extack, so we will
Use extack when it is available.

You may have to go deeper in the code for this to work. I think if it is
tricky to do consider doing it as a followup patch.

quoted

BTW:
now that i am looking at the patches again - small details:
struct flow_offload_action is sometimes initialized and sometimes not (and
sometimes allocated and sometimes off the stack). Maybe to be consistent
pick one style and stick with it.

For this implement, it is because for the action offload process, we need items of
flow_action_entry in the flow_offload_action, then the size of flow_offload_action is
dependent on the action to be offloaded. But for delete case, we just need a pure
flow_offload_action, so we take it in stack.  You can refer to the implement in cls_flower,
it is similar with our case.
Do you think if it make sense to us?

I think it does. Let me see if i can explain it in my words:
For delete you dont have any attributes to pass but for create you need
to pass attributes which may be variable sized depending on the action.
And for that reason for create you need to allocate (but for delete
you can use a variable on the stack).
If yes, then at least make sure you are consistent on the stack values;
I think i saw some cases you initialize and in some you didnt.

cheers,
jamal