Hi Ian, Thanks for the review. My responses are inline.
> -----Original Message----- > From: Stokes, Ian <[email protected]> > Sent: Tuesday 15 June 2021 18:04 > To: Ferriter, Cian <[email protected]>; [email protected]; Van > Haaren, Harry <[email protected]> > Cc: [email protected]; Gaetan Rivet <[email protected]>; Sriharsha > Basavapatna <[email protected]> > Subject: RE: [ovs-dev] [v12 16/16] netdev: Optimize netdev_send_prepare_batch. > > > From: Harry van Haaren <[email protected]> > > > > Optimize for the best case here where all packets will be compatible > > with 'netdev_flags'. > > > > Signed-off-by: Harry van Haaren <[email protected]> > > Co-authored-by: Cian Ferriter <[email protected]> > > Signed-off-by: Cian Ferriter <[email protected]> > > Thanks for the patch Cian/Harry, comments inline. > > In general I have a concern of the impact on HWOL cases here and how much it > would affect that. Have you data on this? Any > thoughts from other HWOL folks here? >From our POV, we are optimizing the normal and error free case. We are >impacting the error case. When a packet is not compatible with netdev_flags >that packet will be deleted from the batch and a " VLOG_WARN_RL()" will be >called. This is quite costly as it is. I'm curious to see what other HWOL folks think. > > --- > > NEWS | 2 ++ > > lib/netdev.c | 31 ++++++++++++++++++++++--------- > > 2 files changed, 24 insertions(+), 9 deletions(-) > > > > diff --git a/NEWS b/NEWS > > index a7ec34af1..8ad3c98db 100644 > > --- a/NEWS > > +++ b/NEWS > > @@ -18,6 +18,8 @@ Post-v2.15.0 > > CPU supports it. This enhances performance by using the native > > vpopcount > > instructions, instead of the emulated version of vpopcount. > > * Optimize dp_netdev_output by enhancing compiler optimization > > potential. > > + * Optimize netdev sending by assuming the happy case, and using > > fallback > > + for if the netdev doesnt meet the required HWOL needs of a packet. > This sounds a bit too low level for a NEWS item. Would suggest changing to > single line Optimize netdev sending for best case scenario. > Agreed, your single line sounds better. I've fixed this in the next version. > BR > Ian > > > - ovs-ctl: > > * New option '--no-record-hostname' to disable hostname configuration > > in ovsdb on startup. > > diff --git a/lib/netdev.c b/lib/netdev.c > > index 91e91955c..29a5f1aa9 100644 > > --- a/lib/netdev.c > > +++ b/lib/netdev.c > > @@ -837,20 +837,33 @@ static void > > netdev_send_prepare_batch(const struct netdev *netdev, > > struct dp_packet_batch *batch) > > { > > - struct dp_packet *packet; > > - size_t i, size = dp_packet_batch_size(batch); > > + struct dp_packet *p; > > + uint32_t i, size = dp_packet_batch_size(batch); > > + char *err_msg = NULL; > > > > - DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, packet, batch) { > > - char *errormsg = NULL; > > + for (i = 0; i < size; i++) { > > + p = batch->packets[i]; > > + int pkt_ok = netdev_send_prepare_packet(netdev->ol_flags, p, > > &err_msg); > > > > - if (netdev_send_prepare_packet(netdev->ol_flags, packet, > > &errormsg)) { > > - dp_packet_batch_refill(batch, packet, i); > > + if (OVS_UNLIKELY(!pkt_ok)) { > > + goto refill_loop; > > + } > > + } > > + > > + return; > > + > > +refill_loop: > > + /* Loop through packets from the start of the batch again. This is the > > + * exceptional case where packets aren't compatible with > > 'netdev_flags'. */ > > + DP_PACKET_BATCH_REFILL_FOR_EACH (i, size, p, batch) { > > + if (netdev_send_prepare_packet(netdev->ol_flags, p, &err_msg)) { > > + dp_packet_batch_refill(batch, p, i); > > } else { > > - dp_packet_delete(packet); > > + dp_packet_delete(p); > > COVERAGE_INC(netdev_send_prepare_drops); > > VLOG_WARN_RL(&rl, "%s: Packet dropped: %s", > > - netdev_get_name(netdev), errormsg); > > - free(errormsg); > > + netdev_get_name(netdev), err_msg); > > + free(err_msg); > > } > > } > > } > > -- > > 2.31.1 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
