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

On November 24, 2021 10:59 PM, Jamal Hadi Salim wrote:

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

quoted

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.

quoted

quoted

quoted

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.

quoted

quoted

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?

quoted

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

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.

quoted

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.

For the initialization, Vald mentioned this in another message, we will add the
Initialization in action delete, thanks.

cheers,
jamal