Message ID | 20210517140434.59555-17-cian.ferriter@intel.com |
---|---|
State | Changes Requested |
Headers | show |
Series | DPIF Framework + Optimizations | expand |
> From: Harry van Haaren <harry.van.haaren@intel.com> > > Optimize for the best case here where all packets will be compatible > with 'netdev_flags'. > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > Co-authored-by: Cian Ferriter <cian.ferriter@intel.com> > Signed-off-by: Cian Ferriter <cian.ferriter@intel.com> 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? > --- > 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. 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Hi Ian, Thanks for the review. My responses are inline. > -----Original Message----- > From: Stokes, Ian <ian.stokes@intel.com> > Sent: Tuesday 15 June 2021 18:04 > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com> > Cc: i.maximets@ovn.org; Gaetan Rivet <gaetanr@nvidia.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> > Subject: RE: [ovs-dev] [v12 16/16] netdev: Optimize netdev_send_prepare_batch. > > > From: Harry van Haaren <harry.van.haaren@intel.com> > > > > Optimize for the best case here where all packets will be compatible > > with 'netdev_flags'. > > > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > Co-authored-by: Cian Ferriter <cian.ferriter@intel.com> > > Signed-off-by: Cian Ferriter <cian.ferriter@intel.com> > > 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 > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> Hi Ian, > > Thanks for the review. My responses are inline. > > > -----Original Message----- > > From: Stokes, Ian <ian.stokes@intel.com> > > Sent: Tuesday 15 June 2021 18:04 > > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van > Haaren, Harry <harry.van.haaren@intel.com> > > Cc: i.maximets@ovn.org; Gaetan Rivet <gaetanr@nvidia.com>; Sriharsha > Basavapatna <sriharsha.basavapatna@broadcom.com> > > Subject: RE: [ovs-dev] [v12 16/16] netdev: Optimize > netdev_send_prepare_batch. > > > > > From: Harry van Haaren <harry.van.haaren@intel.com> > > > > > > Optimize for the best case here where all packets will be compatible > > > with 'netdev_flags'. > > > > > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > > Co-authored-by: Cian Ferriter <cian.ferriter@intel.com> > > > Signed-off-by: Cian Ferriter <cian.ferriter@intel.com> > > > > 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. +1, again probably not a blocker for the DPIF framework, could be submitted separately from the series if required. Regards Ian > > > > --- > > > 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 > > > dev@openvswitch.org > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
> -----Original Message----- > From: Stokes, Ian <ian.stokes@intel.com> > Sent: Wednesday 16 June 2021 13:48 > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van Haaren, Harry <harry.van.haaren@intel.com> > Cc: i.maximets@ovn.org; Gaetan Rivet <gaetanr@nvidia.com>; Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com> > Subject: RE: [ovs-dev] [v12 16/16] netdev: Optimize netdev_send_prepare_batch. > > > Hi Ian, > > > > Thanks for the review. My responses are inline. > > > > > -----Original Message----- > > > From: Stokes, Ian <ian.stokes@intel.com> > > > Sent: Tuesday 15 June 2021 18:04 > > > To: Ferriter, Cian <cian.ferriter@intel.com>; ovs-dev@openvswitch.org; Van > > Haaren, Harry <harry.van.haaren@intel.com> > > > Cc: i.maximets@ovn.org; Gaetan Rivet <gaetanr@nvidia.com>; Sriharsha > > Basavapatna <sriharsha.basavapatna@broadcom.com> > > > Subject: RE: [ovs-dev] [v12 16/16] netdev: Optimize > > netdev_send_prepare_batch. > > > > > > > From: Harry van Haaren <harry.van.haaren@intel.com> > > > > > > > > Optimize for the best case here where all packets will be compatible > > > > with 'netdev_flags'. > > > > > > > > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com> > > > > Co-authored-by: Cian Ferriter <cian.ferriter@intel.com> > > > > Signed-off-by: Cian Ferriter <cian.ferriter@intel.com> > > > > > > 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. > > +1, again probably not a blocker for the DPIF framework, could be submitted separately from the series if required. > I'll submit this patch separately from the DPIF series. > Regards > Ian > > > > > > --- > > > > 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 > > > > dev@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
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. - 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); } } }