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

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

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.

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?

cheers,
jamal