On 5/8/23 18:09, Ivan Malov wrote: > Hi Nole, Simon, Ilya, > > On Mon, 8 May 2023, Nole Zhang wrote: > >> >> >>> -----Original Message----- >>> From: Ilya Maximets <[email protected]> >>> Sent: Saturday, May 6, 2023 3:07 AM >>> To: Ivan Malov <[email protected]>; Simon Horman >>> <[email protected]> >>> Cc: [email protected]; [email protected]; Eli Britstein >>> <[email protected]>; Kevin Liu <[email protected]>; Chaoyong He >>> <[email protected]>; oss-drivers <[email protected]>; Nole >>> Zhang <[email protected]> >>> Subject: Re: [ovs-dev] [PATCH dpdk-latest v4 1/5] netdev-dpdk: use flow >>> transfer proxy >>> >>> On 5/3/23 16:00, Ivan Malov wrote: >>>> Hello Simon, >>>> >>>> This patch has me intrigued. By the looks of it, it bears uncanny >>>> resemblance to patch [1] by another author. Is your patch based on >>>> patch [1]? If yes, could you please comment on the following: >>>> >>>> 1) Your patch does not seem to reference the original author. >>>> Why is it so? Is there a problem, colleagues? >>> >>> When re-using someone else's work, please, retain the original authorship. >>> I >>> see there are changes made to the patch, but it's the same as the original >>> in >>> many parts. Since you made changes, you should add yourself as co-authors. >>> If you feel that changes made are more significant than the original ptch, >>> then you may swap the authorship, but you should add the original author to >>> the list of co-authors anyway. > > Thanks for the clarification Ilya. > >> >> @Ivan Malov sorry again. >> It is my fault, I'm very sorry for these. >> Next version, I will add signed-by. > > Glad we see eye to eye about this and can understand > each other after all. > >>> >>>> >>>> 2) Your patch does not seem to address review feedback [2]. >>>> There's a problem that has been indicated by Eli, >>>> regarding flow flush. Doesn't it still stand? >>> >>> In this version the rte_flow_flush() call is added instead of failing the >>> detach. >>> However, >>> >>> a. the flush operation should have already been executed from >>> the higher layer from do_del_port() in dpif-netdev. So, >>> it should not be needed. >>> >>> b. The problem doesn't apper to be addressed, because related >>> ports will not get their flows flushed. >> >> Ok, I think you are right, do you think if I use the rte_flow_flush() for >> related ports is OK? > > Please find Eli's review of the original patch where he explains > how it should be done: > > https://mail.openvswitch.org/pipermail/ovs-dev/2023-February/402172.html
Yep. The flushing should come from the netdev-offload layer or higher. We shouldn't blindly call rte_flow_flush() as that may leave resources in the netdev-offload layer. OTOH, I'm not really sure if it's safe to flush even from the netdev-offload layer, because we can have a concurrent flow_put operation executed from a different offload thread. And last I checked (a year ago), rte_flow API is not thread safe, even if documentation says so. We might need to pause all offload threads while the flush is in progress. And we're potentially leaving flow marks associated in dpif-netdev that might prevent flows from from being re-offloaded. Best regards, Ilya Maximets. > > Thank you. > >> >>> >>> Best regards, Ilya Maximets. >>> >>>> >>>> Interested to hear your input on this. Thank you. >>>> >>>> [1] >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-February/402152.ht >>>> ml >>>> >>>> [2] >>>> https://mail.openvswitch.org/pipermail/ovs-dev/2023-February/402172.ht >>>> ml >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
