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. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
