On 6 Mar 2023, at 9:19, Chris Mi wrote:
> On 3/1/2023 9:23 PM, Ilya Maximets wrote: >> On 3/1/23 14:21, Ilya Maximets wrote: >>> On 3/1/23 14:08, Chris Mi wrote: >>>> On 3/1/2023 8:44 PM, Ilya Maximets wrote: >>>>> On 2/28/23 14:05, Chris Mi wrote: >>>>>> On 2/24/2023 4:16 AM, Ilya Maximets wrote: >>>>>>> On 2/23/23 12:26, Chris Mi wrote: >>>>>>>> Initialize psample socket. Add sFlow recv API to receive sampled >>>>>>>> packets from psample socket. Add sFow recv wait API to add psample >>>>>>>> socket fd to poll list. >>>>>>>> >>>>>>>> Signed-off-by: Chris Mi<[email protected]> >>>>>>>> Reviewed-by: Roi Dayan<[email protected]> >>>>>>>> --- >>>>>>>> lib/dpif.h | 7 ++ >>>>>>>> lib/netdev-offload-provider.h | 11 ++ >>>>>>>> lib/netdev-offload-tc.c | 188 >>>>>>>> ++++++++++++++++++++++++++++++++++ >>>>>>>> 3 files changed, 206 insertions(+) >>>>>>>> >>>>> <snip> >>>>> >>>>>>>> +{ >>>>>>>> + int read_tries = 0; >>>>>>>> + >>>>>>>> + if (!psample_sock) { >>>>>>>> + return ENOENT; >>>>>>>> + } >>>>>>>> + >>>>>>>> + for (;;) { >>>>>>>> + struct offload_psample psample; >>>>>>>> + struct offload_sflow sflow; >>>>>>>> + int error; >>>>>>>> + >>>>>>>> + if (++read_tries > 50) { >>>>>>>> + return EAGAIN; >>>>>>>> + } >>>>>>>> + >>>>>>>> + error = nl_sock_recv(psample_sock, buf, NULL, false); >>>>>>>> + if (error == ENOBUFS) { >>>>>>>> + continue; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (error) { >>>>>>>> + if (error == EAGAIN) { >>>>>>>> + break; >>>>>>>> + } >>>>>>>> + return error; >>>>>>>> + } >>>>>>>> + >>>>>>>> + error = psample_from_ofpbuf(&psample, buf); >>>>>>>> + if (!error) { >>>>>>>> + return psample_parse_packet(&psample, &sflow, upcall); >>>>>>>> + } else if (error) { >>>>>>> Condition here is always true. >>>>>> I copied from dpif_netlink_recv_cpu_dispatch(). And I also think it is >>>>>> ok. >>>>> The 'if' condition in dpif_netlink_recv_cpu_dispatch() is different >>>>> and so the 'else if (error)' is not always true there. But it is >>>>> always true here, hence makes no practical sense. >>>> OK, I see. I'll change the code like this: >>>> >>>> error = psample_from_ofpbuf(&psample, buf); >>>> if (!error) { >>>> return psample_parse_packet(&psample, upcall); >>>> } else { >>>> return error; >>>> } >>> The 'else' condition is unnecessary, since we do always return >>> at this point. In the dpif-netlink code there is a case where >>> we continue, but here there is no such case. >>> >>> So, I'd suggest: >>> >>> error = psample_from_ofpbuf(&psample, buf); >>> if (error) { >>> return error; >>> } >>> >>> return psample_parse_packet(&psample, upcall); >>> >>> Or even just a ternary operator: >>> >>> error = psample_from_ofpbuf(&psample, buf); >>> >>> return error ? error : psample_parse_packet(&psample, upcall); >> Just in case, please wait for a proper review on v24 from someone >> before sending a new version. There is no point to re-spin the >> set just for this. > OK. > > Eelco, > > You reviewed my previous versions and acked most of them. But since Ilya > suggested > a new design, the code was changed greatly. So I removed the Acked-by for > you. I'm wondering > if you have time to review again recently? Yes, it’s on my todo for this week, but might slip into next week… //Eelco _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
