| Message ID | 1445154799-31083-5-git-send-email-leonid.bloch@ravellosystems.com |
|---|---|
| State | New |
| Headers | show |
On 10/18/2015 03:53 PM, Leonid Bloch wrote: > Previously, the lower parts of these counters (TORL, TOTL) were > resetting after reaching their maximal values, and since the continuation > of counting in the higher parts (TORH, TOTH) was triggered by an > overflow event of the lower parts, the count was not correct. > > Additionally, TORH and TOTH were counting the corresponding frames, and > not the octets, as they supposed to do. > > Additionally, these 64-bit registers did not stick at their maximal > values when (and if) they reached them. > > This fix resolves all the issues mentioned above, and makes the octet > counters behave according to Intel's specs. > > Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com> > Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> > --- > hw/net/e1000.c | 34 ++++++++++++++++++++++++++-------- > 1 file changed, 26 insertions(+), 8 deletions(-) > > diff --git a/hw/net/e1000.c b/hw/net/e1000.c > index 5530285..7f977b6 100644 > --- a/hw/net/e1000.c > +++ b/hw/net/e1000.c > @@ -583,6 +583,28 @@ inc_reg_if_not_full(E1000State *s, int index) > } > } > > +static void > +grow_8reg_if_not_full(E1000State *s, int index, int size) > +{ > + uint32_t lo = s->mac_reg[index]; > + uint32_t hi = s->mac_reg[index+1]; > + > + if (lo == 0xffffffff) { > + if ((hi += size) > s->mac_reg[index+1]) { > + s->mac_reg[index+1] = hi; > + } else if (s->mac_reg[index+1] != 0xffffffff) { > + s->mac_reg[index+1] = 0xffffffff; > + } > + } else { > + if (((lo += size) < s->mac_reg[index]) > + && (s->mac_reg[index] = 0xffffffff)) { /* setting low to full */ > + s->mac_reg[index+1] += ++lo; > + } else { > + s->mac_reg[index] = lo; > + } > + } > +} How about something easier: uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] <<32; if (sum + size < sum) { sum = 0xffffffffffffffff; } else { sum += size; } s->max_reg[index] = sum; s->max_reg[index+1] = sum >> 32; > + > static inline int > vlan_enabled(E1000State *s) > { > @@ -632,7 +654,7 @@ static void > xmit_seg(E1000State *s) > { > uint16_t len, *sp; > - unsigned int frames = s->tx.tso_frames, css, sofar, n; > + unsigned int frames = s->tx.tso_frames, css, sofar; > struct e1000_tx *tp = &s->tx; > > if (tp->tse && tp->cptse) { > @@ -678,10 +700,8 @@ xmit_seg(E1000State *s) > } else > e1000_send_packet(s, tp->data, tp->size); > inc_reg_if_not_full(s, TPT); > + grow_8reg_if_not_full(s, TOTL, s->tx.size); > s->mac_reg[GPTC] = s->mac_reg[TPT]; > - n = s->mac_reg[TOTL]; > - if ((s->mac_reg[TOTL] += s->tx.size) < n) > - s->mac_reg[TOTH]++; > } > > static void > @@ -1096,11 +1116,9 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt) > /* TOR - Total Octets Received: > * This register includes bytes received in a packet from the <Destination > * Address> field through the <CRC> field, inclusively. > + * Always include FCS length (4) in size. > */ > - n = s->mac_reg[TORL] + size + /* Always include FCS length. */ 4; > - if (n < s->mac_reg[TORL]) > - s->mac_reg[TORH]++; > - s->mac_reg[TORL] = n; > + grow_8reg_if_not_full(s, TORL, size+4); > > n = E1000_ICS_RXT0; > if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH])
On Tue, Oct 20, 2015 at 9:16 AM, Jason Wang <jasowang@redhat.com> wrote: > > > On 10/18/2015 03:53 PM, Leonid Bloch wrote: >> Previously, the lower parts of these counters (TORL, TOTL) were >> resetting after reaching their maximal values, and since the continuation >> of counting in the higher parts (TORH, TOTH) was triggered by an >> overflow event of the lower parts, the count was not correct. >> >> Additionally, TORH and TOTH were counting the corresponding frames, and >> not the octets, as they supposed to do. >> >> Additionally, these 64-bit registers did not stick at their maximal >> values when (and if) they reached them. >> >> This fix resolves all the issues mentioned above, and makes the octet >> counters behave according to Intel's specs. >> >> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com> >> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> >> --- >> hw/net/e1000.c | 34 ++++++++++++++++++++++++++-------- >> 1 file changed, 26 insertions(+), 8 deletions(-) >> >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c >> index 5530285..7f977b6 100644 >> --- a/hw/net/e1000.c >> +++ b/hw/net/e1000.c >> @@ -583,6 +583,28 @@ inc_reg_if_not_full(E1000State *s, int index) >> } >> } >> >> +static void >> +grow_8reg_if_not_full(E1000State *s, int index, int size) >> +{ >> + uint32_t lo = s->mac_reg[index]; >> + uint32_t hi = s->mac_reg[index+1]; >> + >> + if (lo == 0xffffffff) { >> + if ((hi += size) > s->mac_reg[index+1]) { >> + s->mac_reg[index+1] = hi; >> + } else if (s->mac_reg[index+1] != 0xffffffff) { >> + s->mac_reg[index+1] = 0xffffffff; >> + } >> + } else { >> + if (((lo += size) < s->mac_reg[index]) >> + && (s->mac_reg[index] = 0xffffffff)) { /* setting low to full */ >> + s->mac_reg[index+1] += ++lo; >> + } else { >> + s->mac_reg[index] = lo; >> + } >> + } >> +} > > How about something easier: > > uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] <<32; > if (sum + size < sum) { > sum = 0xffffffffffffffff; > } else { > sum += size; > } > s->max_reg[index] = sum; > s->max_reg[index+1] = sum >> 32; Yes, that is better! Few small changes: uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] << 32; if (sum + size < sum) { sum = ~0; } else { sum += size; } s->mac_reg[index] = sum; s->mac_reg[index+1] = sum >> 32; > >> + >> static inline int >> vlan_enabled(E1000State *s) >> { >> @@ -632,7 +654,7 @@ static void >> xmit_seg(E1000State *s) >> { >> uint16_t len, *sp; >> - unsigned int frames = s->tx.tso_frames, css, sofar, n; >> + unsigned int frames = s->tx.tso_frames, css, sofar; >> struct e1000_tx *tp = &s->tx; >> >> if (tp->tse && tp->cptse) { >> @@ -678,10 +700,8 @@ xmit_seg(E1000State *s) >> } else >> e1000_send_packet(s, tp->data, tp->size); >> inc_reg_if_not_full(s, TPT); >> + grow_8reg_if_not_full(s, TOTL, s->tx.size); >> s->mac_reg[GPTC] = s->mac_reg[TPT]; >> - n = s->mac_reg[TOTL]; >> - if ((s->mac_reg[TOTL] += s->tx.size) < n) >> - s->mac_reg[TOTH]++; >> } >> >> static void >> @@ -1096,11 +1116,9 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt) >> /* TOR - Total Octets Received: >> * This register includes bytes received in a packet from the <Destination >> * Address> field through the <CRC> field, inclusively. >> + * Always include FCS length (4) in size. >> */ >> - n = s->mac_reg[TORL] + size + /* Always include FCS length. */ 4; >> - if (n < s->mac_reg[TORL]) >> - s->mac_reg[TORH]++; >> - s->mac_reg[TORL] = n; >> + grow_8reg_if_not_full(s, TORL, size+4); >> >> n = E1000_ICS_RXT0; >> if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH]) >
On 10/21/2015 08:20 PM, Leonid Bloch wrote: > On Tue, Oct 20, 2015 at 9:16 AM, Jason Wang <jasowang@redhat.com> wrote: >> > >> > >> > On 10/18/2015 03:53 PM, Leonid Bloch wrote: >>> >> Previously, the lower parts of these counters (TORL, TOTL) were >>> >> resetting after reaching their maximal values, and since the continuation >>> >> of counting in the higher parts (TORH, TOTH) was triggered by an >>> >> overflow event of the lower parts, the count was not correct. >>> >> >>> >> Additionally, TORH and TOTH were counting the corresponding frames, and >>> >> not the octets, as they supposed to do. >>> >> >>> >> Additionally, these 64-bit registers did not stick at their maximal >>> >> values when (and if) they reached them. >>> >> >>> >> This fix resolves all the issues mentioned above, and makes the octet >>> >> counters behave according to Intel's specs. >>> >> >>> >> Signed-off-by: Leonid Bloch <leonid.bloch@ravellosystems.com> >>> >> Signed-off-by: Dmitry Fleytman <dmitry.fleytman@ravellosystems.com> >>> >> --- >>> >> hw/net/e1000.c | 34 ++++++++++++++++++++++++++-------- >>> >> 1 file changed, 26 insertions(+), 8 deletions(-) >>> >> >>> >> diff --git a/hw/net/e1000.c b/hw/net/e1000.c >>> >> index 5530285..7f977b6 100644 >>> >> --- a/hw/net/e1000.c >>> >> +++ b/hw/net/e1000.c >>> >> @@ -583,6 +583,28 @@ inc_reg_if_not_full(E1000State *s, int index) >>> >> } >>> >> } >>> >> >>> >> +static void >>> >> +grow_8reg_if_not_full(E1000State *s, int index, int size) >>> >> +{ >>> >> + uint32_t lo = s->mac_reg[index]; >>> >> + uint32_t hi = s->mac_reg[index+1]; >>> >> + >>> >> + if (lo == 0xffffffff) { >>> >> + if ((hi += size) > s->mac_reg[index+1]) { >>> >> + s->mac_reg[index+1] = hi; >>> >> + } else if (s->mac_reg[index+1] != 0xffffffff) { >>> >> + s->mac_reg[index+1] = 0xffffffff; >>> >> + } >>> >> + } else { >>> >> + if (((lo += size) < s->mac_reg[index]) >>> >> + && (s->mac_reg[index] = 0xffffffff)) { /* setting low to full */ >>> >> + s->mac_reg[index+1] += ++lo; >>> >> + } else { >>> >> + s->mac_reg[index] = lo; >>> >> + } >>> >> + } >>> >> +} >> > >> > How about something easier: >> > >> > uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] <<32; >> > if (sum + size < sum) { >> > sum = 0xffffffffffffffff; >> > } else { >> > sum += size; >> > } >> > s->max_reg[index] = sum; >> > s->max_reg[index+1] = sum >> 32; > Yes, that is better! Few small changes: > > uint64_t sum = s->mac_reg[index] | (uint64_t)s->mac_reg[index+1] << 32; > > if (sum + size < sum) { > sum = ~0; > } else { > sum += size; > } > s->mac_reg[index] = sum; > s->mac_reg[index+1] = sum >> 32; > >> > Looks good to me.
diff --git a/hw/net/e1000.c b/hw/net/e1000.c index 5530285..7f977b6 100644 --- a/hw/net/e1000.c +++ b/hw/net/e1000.c @@ -583,6 +583,28 @@ inc_reg_if_not_full(E1000State *s, int index) } } +static void +grow_8reg_if_not_full(E1000State *s, int index, int size) +{ + uint32_t lo = s->mac_reg[index]; + uint32_t hi = s->mac_reg[index+1]; + + if (lo == 0xffffffff) { + if ((hi += size) > s->mac_reg[index+1]) { + s->mac_reg[index+1] = hi; + } else if (s->mac_reg[index+1] != 0xffffffff) { + s->mac_reg[index+1] = 0xffffffff; + } + } else { + if (((lo += size) < s->mac_reg[index]) + && (s->mac_reg[index] = 0xffffffff)) { /* setting low to full */ + s->mac_reg[index+1] += ++lo; + } else { + s->mac_reg[index] = lo; + } + } +} + static inline int vlan_enabled(E1000State *s) { @@ -632,7 +654,7 @@ static void xmit_seg(E1000State *s) { uint16_t len, *sp; - unsigned int frames = s->tx.tso_frames, css, sofar, n; + unsigned int frames = s->tx.tso_frames, css, sofar; struct e1000_tx *tp = &s->tx; if (tp->tse && tp->cptse) { @@ -678,10 +700,8 @@ xmit_seg(E1000State *s) } else e1000_send_packet(s, tp->data, tp->size); inc_reg_if_not_full(s, TPT); + grow_8reg_if_not_full(s, TOTL, s->tx.size); s->mac_reg[GPTC] = s->mac_reg[TPT]; - n = s->mac_reg[TOTL]; - if ((s->mac_reg[TOTL] += s->tx.size) < n) - s->mac_reg[TOTH]++; } static void @@ -1096,11 +1116,9 @@ e1000_receive_iov(NetClientState *nc, const struct iovec *iov, int iovcnt) /* TOR - Total Octets Received: * This register includes bytes received in a packet from the <Destination * Address> field through the <CRC> field, inclusively. + * Always include FCS length (4) in size. */ - n = s->mac_reg[TORL] + size + /* Always include FCS length. */ 4; - if (n < s->mac_reg[TORL]) - s->mac_reg[TORH]++; - s->mac_reg[TORL] = n; + grow_8reg_if_not_full(s, TORL, size+4); n = E1000_ICS_RXT0; if ((rdt = s->mac_reg[RDT]) < s->mac_reg[RDH])