| Message ID | 202407022236347791543@chinatelecom.cn |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] bond: Fix inaccurate log info in bond_shift_load. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On 7/2/24 16:36, Han Ding wrote: > When the delta is less than 1024 in bond_shift_load, it print "shift 0kB of load". > Like this: > bond dpdkbond0: shift 0kB of load (with hash 71) from nic1 to nic2 (now carrying 20650165kB and 8311662kB load, respectively) Hi, Han. Thanks for the patch! I'm curious, is this information about movements that small practically useful? > > Signed-off-by: Han Ding <handing@chinatelecom.cn> > --- > ofproto/bond.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/ofproto/bond.c b/ofproto/bond.c > index c31869a..5b1975d 100644 > --- a/ofproto/bond.c > +++ b/ofproto/bond.c > @@ -1192,13 +1192,23 @@ bond_shift_load(struct bond_entry *hash, struct bond_member *to) > struct bond *bond = from->bond; > uint64_t delta = hash->tx_bytes; > > - VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") " > - "from %s to %s (now carrying %"PRIu64"kB and " > - "%"PRIu64"kB load, respectively)", > - bond->name, delta / 1024, hash - bond->hash, > - from->name, to->name, > - (from->tx_bytes - delta) / 1024, > - (to->tx_bytes + delta) / 1024); > + if (delta >= 1024) { > + VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") " > + "from %s to %s (now carrying %"PRIu64"kB and " > + "%"PRIu64"kB load, respectively)", > + bond->name, delta / 1024, hash - bond->hash, > + from->name, to->name, > + (from->tx_bytes - delta) / 1024, > + (to->tx_bytes + delta) / 1024); > + } else { > + VLOG_INFO("bond %s: shift %"PRIu64"B of load (with hash %"PRIdPTR") " > + "from %s to %s (now carrying %"PRIu64"kB and " > + "%"PRIu64"kB load, respectively)", > + bond->name, delta, hash - bond->hash, > + from->name, to->name, > + (from->tx_bytes - delta) / 1024, > + (to->tx_bytes + delta) / 1024); > + } I'd suggest instead of copying the whole thing, just replace "kB" with another %s and use a couple of ternary operators to produce correct value and "kB" or "B" depending on the delta. Best regards, Ilya Maximets.
On Fri, Aug 02, 2024 at 06:38:19PM GMT, Ilya Maximets wrote: > On 7/2/24 16:36, Han Ding wrote: > > When the delta is less than 1024 in bond_shift_load, it print "shift 0kB of load". > > Like this: > > bond dpdkbond0: shift 0kB of load (with hash 71) from nic1 to nic2 (now carrying 20650165kB and 8311662kB load, respectively) > > Hi, Han. Thanks for the patch! > > I'm curious, is this information about movements that small > practically useful? > I had the same thought. I guess it should not happen very often given how the algorithm works but OTOH, printing "0kB" is definitely not useful. > > > > Signed-off-by: Han Ding <handing@chinatelecom.cn> > > --- > > ofproto/bond.c | 24 +++++++++++++++++------- > > 1 file changed, 17 insertions(+), 7 deletions(-) > > > > diff --git a/ofproto/bond.c b/ofproto/bond.c > > index c31869a..5b1975d 100644 > > --- a/ofproto/bond.c > > +++ b/ofproto/bond.c > > @@ -1192,13 +1192,23 @@ bond_shift_load(struct bond_entry *hash, struct bond_member *to) > > struct bond *bond = from->bond; > > uint64_t delta = hash->tx_bytes; > > > > - VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") " > > - "from %s to %s (now carrying %"PRIu64"kB and " > > - "%"PRIu64"kB load, respectively)", > > - bond->name, delta / 1024, hash - bond->hash, > > - from->name, to->name, > > - (from->tx_bytes - delta) / 1024, > > - (to->tx_bytes + delta) / 1024); > > + if (delta >= 1024) { > > + VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") " > > + "from %s to %s (now carrying %"PRIu64"kB and " > > + "%"PRIu64"kB load, respectively)", > > + bond->name, delta / 1024, hash - bond->hash, > > + from->name, to->name, > > + (from->tx_bytes - delta) / 1024, > > + (to->tx_bytes + delta) / 1024); > > + } else { > > + VLOG_INFO("bond %s: shift %"PRIu64"B of load (with hash %"PRIdPTR") " > > + "from %s to %s (now carrying %"PRIu64"kB and " Apart from Ilya's comment, missing one more indentation space in this line (and all others). Thanks. > > + "%"PRIu64"kB load, respectively)", > > + bond->name, delta, hash - bond->hash, > > + from->name, to->name, > > + (from->tx_bytes - delta) / 1024, > > + (to->tx_bytes + delta) / 1024); > > + } > > I'd suggest instead of copying the whole thing, just replace "kB" > with another %s and use a couple of ternary operators to produce > correct value and "kB" or "B" depending on the delta. > > Best regards, Ilya Maximets. > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/ofproto/bond.c b/ofproto/bond.c index c31869a..5b1975d 100644 --- a/ofproto/bond.c +++ b/ofproto/bond.c @@ -1192,13 +1192,23 @@ bond_shift_load(struct bond_entry *hash, struct bond_member *to) struct bond *bond = from->bond; uint64_t delta = hash->tx_bytes; - VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") " - "from %s to %s (now carrying %"PRIu64"kB and " - "%"PRIu64"kB load, respectively)", - bond->name, delta / 1024, hash - bond->hash, - from->name, to->name, - (from->tx_bytes - delta) / 1024, - (to->tx_bytes + delta) / 1024); + if (delta >= 1024) { + VLOG_INFO("bond %s: shift %"PRIu64"kB of load (with hash %"PRIdPTR") " + "from %s to %s (now carrying %"PRIu64"kB and " + "%"PRIu64"kB load, respectively)", + bond->name, delta / 1024, hash - bond->hash, + from->name, to->name, + (from->tx_bytes - delta) / 1024, + (to->tx_bytes + delta) / 1024); + } else { + VLOG_INFO("bond %s: shift %"PRIu64"B of load (with hash %"PRIdPTR") " + "from %s to %s (now carrying %"PRIu64"kB and " + "%"PRIu64"kB load, respectively)", + bond->name, delta, hash - bond->hash, + from->name, to->name, + (from->tx_bytes - delta) / 1024, + (to->tx_bytes + delta) / 1024); + } /* Shift load away from 'from' to 'to'. */ from->tx_bytes -= delta;
When the delta is less than 1024 in bond_shift_load, it print "shift 0kB of load". Like this: bond dpdkbond0: shift 0kB of load (with hash 71) from nic1 to nic2 (now carrying 20650165kB and 8311662kB load, respectively) Signed-off-by: Han Ding <handing@chinatelecom.cn> --- ofproto/bond.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)