On 6 Jan 2023, at 11:37, Ilya Maximets wrote: > On 1/6/23 11:12, Eelco Chaudron wrote: >> >> >> On 6 Jan 2023, at 10:49, wangchuanlei wrote: >> >>>> On 1/5/23 16:40, Eelco Chaudron wrote: >>>>> >>>>> >>>>> On 5 Jan 2023, at 2:52, wangchuanlei wrote: >>>>> >>>>>> Add support to count upall packets, when kmod of openvswitch >>>>>> upcall to count the number of packets for upcall succeed and >>>>>> failed, which is a better way to see how many packets upcalled on every >>>>>> interfaces. >>>>>> >>>>>> Signed-off-by: wangchuanlei <[email protected]>> >>>>> >>>>> The code works, and I see no other problems, so I???m ok to ACK it. >>>>> >>>>> However, before I do, I do think we should get some feedback on how the >>>>> stats are displayed. >>>>> >>>>> This is what it looks like now: >>>>> >>>>> port 0: my (internal) >>>>> RX packets:10 errors:0 dropped:0 overruns:0 frame:0 >>>>> upcall success:1 upcall fail:0 >>>>> TX packets:0 errors:0 dropped:0 aborted:0 carrier:0 >>>>> collisions:0 >>>>> RX bytes:1230 TX bytes:0 >>>>> >>>>> >>>>> It???s a bit confusing with the space in the name. Should we maybe >>>>> separate upcall stats completely? >>>>> Something like: >>>>> >>>>> port 0: my (internal) >>>>> RX packets:10 errors:0 dropped:0 overruns:0 frame:0 >>>>> TX packets:0 errors:0 dropped:0 aborted:0 carrier:0 >>>>> collisions:0 >>>>> RX bytes:1230 TX bytes:0 >>>>> UPCALL packets:1 failed:0 >>>>> >>>>> Also, note I used ???packets??? and ???failed??? to be more in line with >>>>> existing stats. >>>> >>>> I like the 'UPCALL ...' approach better. We may even go further and have >>>> 'UPCALL packets:X dropped:Y', i.e. 'dropped' instead of 'failed'. Not >>>> sure. >>> >>> Yes, 'UPCALL ...' approach better, may be 'UPCALL packets:X errors:Y' is >>> better, after all, failed/dropped/errors means the kernel datapath stopped >>> upcall to userspce when it find errors in the info of packet. >> >> Ok, so let???s do the UPCALL, and I also think error would fit best. > > +1 > 'UPCALL packets:X errors:Y' indeed looks better. > > One other thought here is that 'success' and 'failure' are meaningful > from the kernel side of things, but a bit ambiguous from the OVS point > of view, because upcall may fail somewhere in the flow translation in > userspace and packet will be dropped, but the counter will say 'upcall > success', which is a bit misleading. 'errors' from that point of view > is a bit better term. Not ideal, but better than other options. > >> >>>>> >>>>> And maybe not even display it when the feature is not supported? >>>> >>>> It should be OK to print these as long as we correctly print '?' >>>> on ports that do not support that counter. Did you test that? >>> >>> I already tested it, if datapath didn't support the feature, it will print >>> '?'. >> >> ACK, did test this also. > > OK, good. Thanks! > >> >>>> >>>>> Anyone any thoughts!? >>>> >>>> This patch is missing the stats propagation to the database in the >>>> iface_refresh_stats(). That should be added. >>> >>> Ok! >>> >>>> >>>> OTOH, a point can be made that these stats should not be part of the main >>>> netdev_stats and hence should not be reported along side of them at all. >>>> It might be better to report them via 'custom stats' mechanism, since not >>>> all the ports will have them and it might be difficult to add support for >>>> upcall counting in that form to userspace datapath, so it will always >>>> report question marks. Custom stats will be accessed via database or via >>>> OpenFlow 1.4+ version of dump-ports request. >>>> >>>> Don't have a strong opinion on this though. >>> >>> Userspace datapath is not count in dpctl command, we can do not include >>> this patch ? > > As I mentioned above, it might be hard to track upcalls this way for > userpsace datapath. The reason is that upcalls are happening on the > datapath level (dpif-netdev), but statistics are queried from the > netdev level. Getting upcall stats from the dpif-netdev down to > netdev stats might be tricky from both the logical and performance > point of view. So, I think, it's OK to always report question marks > for ports in userspace datapath for now. No extra changes required. > >>> >>> Any ideas??? >> >> I guess you can just add them in iface_refresh_stats() ???#define >> IFACE_STATS??? definition, and they will be included if not set the >> UINT64_MAX. Or do I miss anything? > > Yes, we only need to update the IFACE_STATS definition. We should > probably alter the names of the counters though the same way we agreed > to do for the dpctl output, i.e. report 'upcall_success' > as 'upcall_packets' and 'upcall_fail' as 'upcall_errors'. > Doe that make sense?
> Yes it does. I agree, Thank you for review, Eelco & Ilya! Best regards! wangchuanlei
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
