| Message ID | 1491213657-23183-2-git-send-email-bogdan.pricope@linaro.org |
|---|---|
| State | Accepted |
| Commit | ea134fe159c0d249e4bed12b7269e8236afa0262 |
| Headers | show |
Ping! On 3 April 2017 at 13:00, Bogdan Pricope <bogdan.pricope@linaro.org> wrote: > Signed-off-by: Bogdan Pricope <bogdan.pricope@linaro.org> > --- > example/generator/odp_generator.c | 89 ++++++++++++++++++--------------------- > 1 file changed, 41 insertions(+), 48 deletions(-) > > diff --git a/example/generator/odp_generator.c b/example/generator/odp_generator.c > index 95fb543..9c49d94 100644 > --- a/example/generator/odp_generator.c > +++ b/example/generator/odp_generator.c > @@ -119,7 +119,6 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args); > static void print_info(char *progname, appl_args_t *appl_args); > static void usage(char *progname); > static int scan_ip(char *buf, unsigned int *paddr); > -static void tv_sub(struct timeval *recvtime, struct timeval *sendtime); > static void print_global_stats(int num_workers); > > /** > @@ -348,7 +347,7 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t pool, odp_packet_t pkt_ref) > char *buf; > odph_ipv4hdr_t *ip; > odph_icmphdr_t *icmp; > - struct timeval tval; > + uint64_t tval; > uint8_t *tval_d; > unsigned short seq; > > @@ -372,12 +371,12 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t pool, odp_packet_t pkt_ref) > /* icmp */ > icmp = (odph_icmphdr_t *)(buf + ODPH_ETHHDR_LEN + ODPH_IPV4HDR_LEN); > icmp->un.echo.sequence = ip->id; > + > tval_d = (uint8_t *)(buf + ODPH_ETHHDR_LEN + ODPH_IPV4HDR_LEN + > ODPH_ICMPHDR_LEN); > - /* TODO This should be changed to use an > - * ODP timer API once one exists. */ > - gettimeofday(&tval, NULL); > - memcpy(tval_d, &tval, sizeof(struct timeval)); > + tval = odp_time_to_ns(odp_time_local()); > + memcpy(tval_d, &tval, sizeof(uint64_t)); > + > icmp->chksum = 0; > icmp->chksum = odph_chksum(icmp, args->appl.payload + ODPH_ICMPHDR_LEN); > > @@ -594,6 +593,40 @@ static int gen_send_thread(void *arg) > } > > /** > + * Process icmp packets > + * > + * @param icmp icmp header address > + * @param msg output buffer > + */ > + > +static void process_icmp_pkt(odph_icmphdr_t *icmp, char *msg) > +{ > + uint64_t trecv; > + uint64_t tsend; > + uint64_t rtt_ms, rtt_us; > + > + msg[0] = 0; > + > + if (icmp->type == ICMP_ECHOREPLY) { > + odp_atomic_inc_u64(&counters.icmp); > + > + memcpy(&tsend, (uint8_t *)icmp + ODPH_ICMPHDR_LEN, > + sizeof(uint64_t)); > + trecv = odp_time_to_ns(odp_time_local()); > + rtt_ms = (trecv - tsend) / ODP_TIME_MSEC_IN_NS; > + rtt_us = (trecv - tsend) / ODP_TIME_USEC_IN_NS - > + 1000 * rtt_ms; > + sprintf(msg, > + "ICMP Echo Reply seq %d time %" > + PRIu64 ".%.03" PRIu64" ms", > + odp_be_to_cpu_16(icmp->un.echo.sequence), > + rtt_ms, rtt_us); > + } else if (icmp->type == ICMP_ECHO) { > + sprintf(msg, "Icmp Echo Request"); > + } > +} > + > +/** > * Print odp packets > * > * @param thr worker id > @@ -606,16 +639,12 @@ static void print_pkts(int thr, odp_packet_t pkt_tbl[], unsigned len) > char *buf; > odph_ipv4hdr_t *ip; > odph_icmphdr_t *icmp; > - struct timeval tvrecv; > - struct timeval tvsend; > - double rtt; > unsigned i; > size_t offset; > char msg[1024]; > - int rlen; > + > for (i = 0; i < len; ++i) { > pkt = pkt_tbl[i]; > - rlen = 0; > > /* only ip pkts */ > if (!odp_packet_has_ipv4(pkt)) > @@ -634,26 +663,8 @@ static void print_pkts(int thr, odp_packet_t pkt_tbl[], unsigned len) > /* icmp */ > if (ip->proto == ODPH_IPPROTO_ICMP) { > icmp = (odph_icmphdr_t *)(buf + offset); > - /* echo reply */ > - if (icmp->type == ICMP_ECHOREPLY) { > - odp_atomic_inc_u64(&counters.icmp); > - memcpy(&tvsend, buf + offset + ODPH_ICMPHDR_LEN, > - sizeof(struct timeval)); > - /* TODO This should be changed to use an > - * ODP timer API once one exists. */ > - gettimeofday(&tvrecv, NULL); > - tv_sub(&tvrecv, &tvsend); > - rtt = tvrecv.tv_sec*1000 + tvrecv.tv_usec/1000; > - rlen += sprintf(msg + rlen, > - "ICMP Echo Reply seq %d time %.1f ", > - odp_be_to_cpu_16(icmp->un.echo.sequence) > - , rtt); > - } else if (icmp->type == ICMP_ECHO) { > - rlen += sprintf(msg + rlen, > - "Icmp Echo Request"); > - } > > - msg[rlen] = '\0'; > + process_icmp_pkt(icmp, msg); > printf(" [%02i] %s\n", thr, msg); > } > } > @@ -1392,21 +1403,3 @@ static void usage(char *progname) > "\n", NO_PATH(progname), NO_PATH(progname) > ); > } > -/** > - * calc time period > - * > - *@param recvtime start time > - *@param sendtime end time > -*/ > -static void tv_sub(struct timeval *recvtime, struct timeval *sendtime) > -{ > - long sec = recvtime->tv_sec - sendtime->tv_sec; > - long usec = recvtime->tv_usec - sendtime->tv_usec; > - if (usec >= 0) { > - recvtime->tv_sec = sec; > - recvtime->tv_usec = usec; > - } else { > - recvtime->tv_sec = sec - 1; > - recvtime->tv_usec = -usec; > - } > -} > -- > 1.9.1 >
Hi, Bogdan Consider endianness for this uint64_t time value carried in ICMP packets, need htonl and ntohl here. Best Regards, Yi On 11 April 2017 at 19:31, Bogdan Pricope <bogdan.pricope@linaro.org> wrote: > Ping! > > > On 3 April 2017 at 13:00, Bogdan Pricope <bogdan.pricope@linaro.org> > wrote: > > Signed-off-by: Bogdan Pricope <bogdan.pricope@linaro.org> > > --- > > example/generator/odp_generator.c | 89 ++++++++++++++++++------------ > --------- > > 1 file changed, 41 insertions(+), 48 deletions(-) > > > > diff --git a/example/generator/odp_generator.c b/example/generator/odp_ > generator.c > > index 95fb543..9c49d94 100644 > > --- a/example/generator/odp_generator.c > > +++ b/example/generator/odp_generator.c > > @@ -119,7 +119,6 @@ static void parse_args(int argc, char *argv[], > appl_args_t *appl_args); > > static void print_info(char *progname, appl_args_t *appl_args); > > static void usage(char *progname); > > static int scan_ip(char *buf, unsigned int *paddr); > > -static void tv_sub(struct timeval *recvtime, struct timeval *sendtime); > > static void print_global_stats(int num_workers); > > > > /** > > @@ -348,7 +347,7 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t pool, > odp_packet_t pkt_ref) > > char *buf; > > odph_ipv4hdr_t *ip; > > odph_icmphdr_t *icmp; > > - struct timeval tval; > > + uint64_t tval; > > uint8_t *tval_d; > > unsigned short seq; > > > > @@ -372,12 +371,12 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t pool, > odp_packet_t pkt_ref) > > /* icmp */ > > icmp = (odph_icmphdr_t *)(buf + ODPH_ETHHDR_LEN + > ODPH_IPV4HDR_LEN); > > icmp->un.echo.sequence = ip->id; > > + > > tval_d = (uint8_t *)(buf + ODPH_ETHHDR_LEN + ODPH_IPV4HDR_LEN + > > ODPH_ICMPHDR_LEN); > > - /* TODO This should be changed to use an > > - * ODP timer API once one exists. */ > > - gettimeofday(&tval, NULL); > > - memcpy(tval_d, &tval, sizeof(struct timeval)); > > + tval = odp_time_to_ns(odp_time_local()); > > + memcpy(tval_d, &tval, sizeof(uint64_t)); > > + > > icmp->chksum = 0; > > icmp->chksum = odph_chksum(icmp, args->appl.payload + > ODPH_ICMPHDR_LEN); > > > > @@ -594,6 +593,40 @@ static int gen_send_thread(void *arg) > > } > > > > /** > > + * Process icmp packets > > + * > > + * @param icmp icmp header address > > + * @param msg output buffer > > + */ > > + > > +static void process_icmp_pkt(odph_icmphdr_t *icmp, char *msg) > > +{ > > + uint64_t trecv; > > + uint64_t tsend; > > + uint64_t rtt_ms, rtt_us; > > + > > + msg[0] = 0; > > + > > + if (icmp->type == ICMP_ECHOREPLY) { > > + odp_atomic_inc_u64(&counters.icmp); > > + > > + memcpy(&tsend, (uint8_t *)icmp + ODPH_ICMPHDR_LEN, > > + sizeof(uint64_t)); > > + trecv = odp_time_to_ns(odp_time_local()); > > + rtt_ms = (trecv - tsend) / ODP_TIME_MSEC_IN_NS; > > + rtt_us = (trecv - tsend) / ODP_TIME_USEC_IN_NS - > > + 1000 * rtt_ms; > > + sprintf(msg, > > + "ICMP Echo Reply seq %d time %" > > + PRIu64 ".%.03" PRIu64" ms", > > + odp_be_to_cpu_16(icmp->un.echo.sequence), > > + rtt_ms, rtt_us); > > + } else if (icmp->type == ICMP_ECHO) { > > + sprintf(msg, "Icmp Echo Request"); > > + } > > +} > > + > > +/** > > * Print odp packets > > * > > * @param thr worker id > > @@ -606,16 +639,12 @@ static void print_pkts(int thr, odp_packet_t > pkt_tbl[], unsigned len) > > char *buf; > > odph_ipv4hdr_t *ip; > > odph_icmphdr_t *icmp; > > - struct timeval tvrecv; > > - struct timeval tvsend; > > - double rtt; > > unsigned i; > > size_t offset; > > char msg[1024]; > > - int rlen; > > + > > for (i = 0; i < len; ++i) { > > pkt = pkt_tbl[i]; > > - rlen = 0; > > > > /* only ip pkts */ > > if (!odp_packet_has_ipv4(pkt)) > > @@ -634,26 +663,8 @@ static void print_pkts(int thr, odp_packet_t > pkt_tbl[], unsigned len) > > /* icmp */ > > if (ip->proto == ODPH_IPPROTO_ICMP) { > > icmp = (odph_icmphdr_t *)(buf + offset); > > - /* echo reply */ > > - if (icmp->type == ICMP_ECHOREPLY) { > > - odp_atomic_inc_u64(&counters.icmp); > > - memcpy(&tvsend, buf + offset + > ODPH_ICMPHDR_LEN, > > - sizeof(struct timeval)); > > - /* TODO This should be changed to use an > > - * ODP timer API once one exists. */ > > - gettimeofday(&tvrecv, NULL); > > - tv_sub(&tvrecv, &tvsend); > > - rtt = tvrecv.tv_sec*1000 + > tvrecv.tv_usec/1000; > > - rlen += sprintf(msg + rlen, > > - "ICMP Echo Reply seq %d time > %.1f ", > > - odp_be_to_cpu_16(icmp->un. > echo.sequence) > > - , rtt); > > - } else if (icmp->type == ICMP_ECHO) { > > - rlen += sprintf(msg + rlen, > > - "Icmp Echo Request"); > > - } > > > > - msg[rlen] = '\0'; > > + process_icmp_pkt(icmp, msg); > > printf(" [%02i] %s\n", thr, msg); > > } > > } > > @@ -1392,21 +1403,3 @@ static void usage(char *progname) > > "\n", NO_PATH(progname), NO_PATH(progname) > > ); > > } > > -/** > > - * calc time period > > - * > > - *@param recvtime start time > > - *@param sendtime end time > > -*/ > > -static void tv_sub(struct timeval *recvtime, struct timeval *sendtime) > > -{ > > - long sec = recvtime->tv_sec - sendtime->tv_sec; > > - long usec = recvtime->tv_usec - sendtime->tv_usec; > > - if (usec >= 0) { > > - recvtime->tv_sec = sec; > > - recvtime->tv_usec = usec; > > - } else { > > - recvtime->tv_sec = sec - 1; > > - recvtime->tv_usec = -usec; > > - } > > -} > > -- > > 1.9.1 > > >
Hi Yi, I disagree: bytes sent in Data field of Echo request are opaque and have no meaning for any other entity then ‘me’ (the sender of echo request / receiver of echo reply). The only obligation of the receiver of echo request, as stated by RFC 792 is: “The data received in the echo message must be returned in the echo reply message.” There is no standardization on what should be there (some even using it to convey commands/data – see „Loki ICMP Tunneling”) so it can be whatever sender wants –endianness transformations are not useful in this case. BR, Bogdan On 19 April 2017 at 06:08, Yi He <yi.he@linaro.org> wrote: > Hi, Bogdan > > Consider endianness for this uint64_t time value carried in ICMP packets, > need htonl and ntohl here. > > Best Regards, Yi > > > > On 11 April 2017 at 19:31, Bogdan Pricope <bogdan.pricope@linaro.org> wrote: >> >> Ping! >> >> >> On 3 April 2017 at 13:00, Bogdan Pricope <bogdan.pricope@linaro.org> >> wrote: >> > Signed-off-by: Bogdan Pricope <bogdan.pricope@linaro.org> >> > --- >> > example/generator/odp_generator.c | 89 >> > ++++++++++++++++++--------------------- >> > 1 file changed, 41 insertions(+), 48 deletions(-) >> > >> > diff --git a/example/generator/odp_generator.c >> > b/example/generator/odp_generator.c >> > index 95fb543..9c49d94 100644 >> > --- a/example/generator/odp_generator.c >> > +++ b/example/generator/odp_generator.c >> > @@ -119,7 +119,6 @@ static void parse_args(int argc, char *argv[], >> > appl_args_t *appl_args); >> > static void print_info(char *progname, appl_args_t *appl_args); >> > static void usage(char *progname); >> > static int scan_ip(char *buf, unsigned int *paddr); >> > -static void tv_sub(struct timeval *recvtime, struct timeval *sendtime); >> > static void print_global_stats(int num_workers); >> > >> > /** >> > @@ -348,7 +347,7 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t pool, >> > odp_packet_t pkt_ref) >> > char *buf; >> > odph_ipv4hdr_t *ip; >> > odph_icmphdr_t *icmp; >> > - struct timeval tval; >> > + uint64_t tval; >> > uint8_t *tval_d; >> > unsigned short seq; >> > >> > @@ -372,12 +371,12 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t pool, >> > odp_packet_t pkt_ref) >> > /* icmp */ >> > icmp = (odph_icmphdr_t *)(buf + ODPH_ETHHDR_LEN + >> > ODPH_IPV4HDR_LEN); >> > icmp->un.echo.sequence = ip->id; >> > + >> > tval_d = (uint8_t *)(buf + ODPH_ETHHDR_LEN + ODPH_IPV4HDR_LEN + >> > ODPH_ICMPHDR_LEN); >> > - /* TODO This should be changed to use an >> > - * ODP timer API once one exists. */ >> > - gettimeofday(&tval, NULL); >> > - memcpy(tval_d, &tval, sizeof(struct timeval)); >> > + tval = odp_time_to_ns(odp_time_local()); >> > + memcpy(tval_d, &tval, sizeof(uint64_t)); >> > + >> > icmp->chksum = 0; >> > icmp->chksum = odph_chksum(icmp, args->appl.payload + >> > ODPH_ICMPHDR_LEN); >> > >> > @@ -594,6 +593,40 @@ static int gen_send_thread(void *arg) >> > } >> > >> > /** >> > + * Process icmp packets >> > + * >> > + * @param icmp icmp header address >> > + * @param msg output buffer >> > + */ >> > + >> > +static void process_icmp_pkt(odph_icmphdr_t *icmp, char *msg) >> > +{ >> > + uint64_t trecv; >> > + uint64_t tsend; >> > + uint64_t rtt_ms, rtt_us; >> > + >> > + msg[0] = 0; >> > + >> > + if (icmp->type == ICMP_ECHOREPLY) { >> > + odp_atomic_inc_u64(&counters.icmp); >> > + >> > + memcpy(&tsend, (uint8_t *)icmp + ODPH_ICMPHDR_LEN, >> > + sizeof(uint64_t)); >> > + trecv = odp_time_to_ns(odp_time_local()); >> > + rtt_ms = (trecv - tsend) / ODP_TIME_MSEC_IN_NS; >> > + rtt_us = (trecv - tsend) / ODP_TIME_USEC_IN_NS - >> > + 1000 * rtt_ms; >> > + sprintf(msg, >> > + "ICMP Echo Reply seq %d time %" >> > + PRIu64 ".%.03" PRIu64" ms", >> > + odp_be_to_cpu_16(icmp->un.echo.sequence), >> > + rtt_ms, rtt_us); >> > + } else if (icmp->type == ICMP_ECHO) { >> > + sprintf(msg, "Icmp Echo Request"); >> > + } >> > +} >> > + >> > +/** >> > * Print odp packets >> > * >> > * @param thr worker id >> > @@ -606,16 +639,12 @@ static void print_pkts(int thr, odp_packet_t >> > pkt_tbl[], unsigned len) >> > char *buf; >> > odph_ipv4hdr_t *ip; >> > odph_icmphdr_t *icmp; >> > - struct timeval tvrecv; >> > - struct timeval tvsend; >> > - double rtt; >> > unsigned i; >> > size_t offset; >> > char msg[1024]; >> > - int rlen; >> > + >> > for (i = 0; i < len; ++i) { >> > pkt = pkt_tbl[i]; >> > - rlen = 0; >> > >> > /* only ip pkts */ >> > if (!odp_packet_has_ipv4(pkt)) >> > @@ -634,26 +663,8 @@ static void print_pkts(int thr, odp_packet_t >> > pkt_tbl[], unsigned len) >> > /* icmp */ >> > if (ip->proto == ODPH_IPPROTO_ICMP) { >> > icmp = (odph_icmphdr_t *)(buf + offset); >> > - /* echo reply */ >> > - if (icmp->type == ICMP_ECHOREPLY) { >> > - odp_atomic_inc_u64(&counters.icmp); >> > - memcpy(&tvsend, buf + offset + >> > ODPH_ICMPHDR_LEN, >> > - sizeof(struct timeval)); >> > - /* TODO This should be changed to use an >> > - * ODP timer API once one exists. */ >> > - gettimeofday(&tvrecv, NULL); >> > - tv_sub(&tvrecv, &tvsend); >> > - rtt = tvrecv.tv_sec*1000 + >> > tvrecv.tv_usec/1000; >> > - rlen += sprintf(msg + rlen, >> > - "ICMP Echo Reply seq %d time >> > %.1f ", >> > - >> > odp_be_to_cpu_16(icmp->un.echo.sequence) >> > - , rtt); >> > - } else if (icmp->type == ICMP_ECHO) { >> > - rlen += sprintf(msg + rlen, >> > - "Icmp Echo Request"); >> > - } >> > >> > - msg[rlen] = '\0'; >> > + process_icmp_pkt(icmp, msg); >> > printf(" [%02i] %s\n", thr, msg); >> > } >> > } >> > @@ -1392,21 +1403,3 @@ static void usage(char *progname) >> > "\n", NO_PATH(progname), NO_PATH(progname) >> > ); >> > } >> > -/** >> > - * calc time period >> > - * >> > - *@param recvtime start time >> > - *@param sendtime end time >> > -*/ >> > -static void tv_sub(struct timeval *recvtime, struct timeval *sendtime) >> > -{ >> > - long sec = recvtime->tv_sec - sendtime->tv_sec; >> > - long usec = recvtime->tv_usec - sendtime->tv_usec; >> > - if (usec >= 0) { >> > - recvtime->tv_sec = sec; >> > - recvtime->tv_usec = usec; >> > - } else { >> > - recvtime->tv_sec = sec - 1; >> > - recvtime->tv_usec = -usec; >> > - } >> > -} >> > -- >> > 1.9.1 >> > > >
Ping. On 3 May 2017 at 11:45, Bogdan Pricope <bogdan.pricope@linaro.org> wrote: > Hi Yi, > > I disagree: bytes sent in Data field of Echo request are opaque and > have no meaning for any other entity then ‘me’ (the sender of echo > request / receiver of echo reply). > > The only obligation of the receiver of echo request, as stated by RFC 792 is: > “The data received in the echo message must be returned in the echo > reply message.” > > There is no standardization on what should be there (some even using > it to convey commands/data – see „Loki ICMP Tunneling”) so it can be > whatever sender wants –endianness transformations are not useful in > this case. > > BR, > Bogdan > > On 19 April 2017 at 06:08, Yi He <yi.he@linaro.org> wrote: >> Hi, Bogdan >> >> Consider endianness for this uint64_t time value carried in ICMP packets, >> need htonl and ntohl here. >> >> Best Regards, Yi >> >> >> >> On 11 April 2017 at 19:31, Bogdan Pricope <bogdan.pricope@linaro.org> wrote: >>> >>> Ping! >>> >>> >>> On 3 April 2017 at 13:00, Bogdan Pricope <bogdan.pricope@linaro.org> >>> wrote: >>> > Signed-off-by: Bogdan Pricope <bogdan.pricope@linaro.org> >>> > --- >>> > example/generator/odp_generator.c | 89 >>> > ++++++++++++++++++--------------------- >>> > 1 file changed, 41 insertions(+), 48 deletions(-) >>> > >>> > diff --git a/example/generator/odp_generator.c >>> > b/example/generator/odp_generator.c >>> > index 95fb543..9c49d94 100644 >>> > --- a/example/generator/odp_generator.c >>> > +++ b/example/generator/odp_generator.c >>> > @@ -119,7 +119,6 @@ static void parse_args(int argc, char *argv[], >>> > appl_args_t *appl_args); >>> > static void print_info(char *progname, appl_args_t *appl_args); >>> > static void usage(char *progname); >>> > static int scan_ip(char *buf, unsigned int *paddr); >>> > -static void tv_sub(struct timeval *recvtime, struct timeval *sendtime); >>> > static void print_global_stats(int num_workers); >>> > >>> > /** >>> > @@ -348,7 +347,7 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t pool, >>> > odp_packet_t pkt_ref) >>> > char *buf; >>> > odph_ipv4hdr_t *ip; >>> > odph_icmphdr_t *icmp; >>> > - struct timeval tval; >>> > + uint64_t tval; >>> > uint8_t *tval_d; >>> > unsigned short seq; >>> > >>> > @@ -372,12 +371,12 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t pool, >>> > odp_packet_t pkt_ref) >>> > /* icmp */ >>> > icmp = (odph_icmphdr_t *)(buf + ODPH_ETHHDR_LEN + >>> > ODPH_IPV4HDR_LEN); >>> > icmp->un.echo.sequence = ip->id; >>> > + >>> > tval_d = (uint8_t *)(buf + ODPH_ETHHDR_LEN + ODPH_IPV4HDR_LEN + >>> > ODPH_ICMPHDR_LEN); >>> > - /* TODO This should be changed to use an >>> > - * ODP timer API once one exists. */ >>> > - gettimeofday(&tval, NULL); >>> > - memcpy(tval_d, &tval, sizeof(struct timeval)); >>> > + tval = odp_time_to_ns(odp_time_local()); >>> > + memcpy(tval_d, &tval, sizeof(uint64_t)); >>> > + >>> > icmp->chksum = 0; >>> > icmp->chksum = odph_chksum(icmp, args->appl.payload + >>> > ODPH_ICMPHDR_LEN); >>> > >>> > @@ -594,6 +593,40 @@ static int gen_send_thread(void *arg) >>> > } >>> > >>> > /** >>> > + * Process icmp packets >>> > + * >>> > + * @param icmp icmp header address >>> > + * @param msg output buffer >>> > + */ >>> > + >>> > +static void process_icmp_pkt(odph_icmphdr_t *icmp, char *msg) >>> > +{ >>> > + uint64_t trecv; >>> > + uint64_t tsend; >>> > + uint64_t rtt_ms, rtt_us; >>> > + >>> > + msg[0] = 0; >>> > + >>> > + if (icmp->type == ICMP_ECHOREPLY) { >>> > + odp_atomic_inc_u64(&counters.icmp); >>> > + >>> > + memcpy(&tsend, (uint8_t *)icmp + ODPH_ICMPHDR_LEN, >>> > + sizeof(uint64_t)); >>> > + trecv = odp_time_to_ns(odp_time_local()); >>> > + rtt_ms = (trecv - tsend) / ODP_TIME_MSEC_IN_NS; >>> > + rtt_us = (trecv - tsend) / ODP_TIME_USEC_IN_NS - >>> > + 1000 * rtt_ms; >>> > + sprintf(msg, >>> > + "ICMP Echo Reply seq %d time %" >>> > + PRIu64 ".%.03" PRIu64" ms", >>> > + odp_be_to_cpu_16(icmp->un.echo.sequence), >>> > + rtt_ms, rtt_us); >>> > + } else if (icmp->type == ICMP_ECHO) { >>> > + sprintf(msg, "Icmp Echo Request"); >>> > + } >>> > +} >>> > + >>> > +/** >>> > * Print odp packets >>> > * >>> > * @param thr worker id >>> > @@ -606,16 +639,12 @@ static void print_pkts(int thr, odp_packet_t >>> > pkt_tbl[], unsigned len) >>> > char *buf; >>> > odph_ipv4hdr_t *ip; >>> > odph_icmphdr_t *icmp; >>> > - struct timeval tvrecv; >>> > - struct timeval tvsend; >>> > - double rtt; >>> > unsigned i; >>> > size_t offset; >>> > char msg[1024]; >>> > - int rlen; >>> > + >>> > for (i = 0; i < len; ++i) { >>> > pkt = pkt_tbl[i]; >>> > - rlen = 0; >>> > >>> > /* only ip pkts */ >>> > if (!odp_packet_has_ipv4(pkt)) >>> > @@ -634,26 +663,8 @@ static void print_pkts(int thr, odp_packet_t >>> > pkt_tbl[], unsigned len) >>> > /* icmp */ >>> > if (ip->proto == ODPH_IPPROTO_ICMP) { >>> > icmp = (odph_icmphdr_t *)(buf + offset); >>> > - /* echo reply */ >>> > - if (icmp->type == ICMP_ECHOREPLY) { >>> > - odp_atomic_inc_u64(&counters.icmp); >>> > - memcpy(&tvsend, buf + offset + >>> > ODPH_ICMPHDR_LEN, >>> > - sizeof(struct timeval)); >>> > - /* TODO This should be changed to use an >>> > - * ODP timer API once one exists. */ >>> > - gettimeofday(&tvrecv, NULL); >>> > - tv_sub(&tvrecv, &tvsend); >>> > - rtt = tvrecv.tv_sec*1000 + >>> > tvrecv.tv_usec/1000; >>> > - rlen += sprintf(msg + rlen, >>> > - "ICMP Echo Reply seq %d time >>> > %.1f ", >>> > - >>> > odp_be_to_cpu_16(icmp->un.echo.sequence) >>> > - , rtt); >>> > - } else if (icmp->type == ICMP_ECHO) { >>> > - rlen += sprintf(msg + rlen, >>> > - "Icmp Echo Request"); >>> > - } >>> > >>> > - msg[rlen] = '\0'; >>> > + process_icmp_pkt(icmp, msg); >>> > printf(" [%02i] %s\n", thr, msg); >>> > } >>> > } >>> > @@ -1392,21 +1403,3 @@ static void usage(char *progname) >>> > "\n", NO_PATH(progname), NO_PATH(progname) >>> > ); >>> > } >>> > -/** >>> > - * calc time period >>> > - * >>> > - *@param recvtime start time >>> > - *@param sendtime end time >>> > -*/ >>> > -static void tv_sub(struct timeval *recvtime, struct timeval *sendtime) >>> > -{ >>> > - long sec = recvtime->tv_sec - sendtime->tv_sec; >>> > - long usec = recvtime->tv_usec - sendtime->tv_usec; >>> > - if (usec >= 0) { >>> > - recvtime->tv_sec = sec; >>> > - recvtime->tv_usec = usec; >>> > - } else { >>> > - recvtime->tv_sec = sec - 1; >>> > - recvtime->tv_usec = -usec; >>> > - } >>> > -} >>> > -- >>> > 1.9.1 >>> > >> >>
O, sorry Bogdan missed your reply, I saw the time value in ICMP message was filled(sending) by "memcpy(tval_d, ..., sizeof(uint64_t));" and processed(receiving) as "memcpy(&tsend, ..., sizeof(uint64_t));" If the sending peer is little endian and the receiving peer is big endian I think it will cause wrong interpretation, right? Best Regards, Yi On 24 May 2017 at 15:48, Bogdan Pricope <bogdan.pricope@linaro.org> wrote: > Ping. > > On 3 May 2017 at 11:45, Bogdan Pricope <bogdan.pricope@linaro.org> wrote: > > Hi Yi, > > > > I disagree: bytes sent in Data field of Echo request are opaque and > > have no meaning for any other entity then ‘me’ (the sender of echo > > request / receiver of echo reply). > > > > The only obligation of the receiver of echo request, as stated by RFC > 792 is: > > “The data received in the echo message must be returned in the echo > > reply message.” > > > > There is no standardization on what should be there (some even using > > it to convey commands/data – see „Loki ICMP Tunneling”) so it can be > > whatever sender wants –endianness transformations are not useful in > > this case. > > > > BR, > > Bogdan > > > > On 19 April 2017 at 06:08, Yi He <yi.he@linaro.org> wrote: > >> Hi, Bogdan > >> > >> Consider endianness for this uint64_t time value carried in ICMP > packets, > >> need htonl and ntohl here. > >> > >> Best Regards, Yi > >> > >> > >> > >> On 11 April 2017 at 19:31, Bogdan Pricope <bogdan.pricope@linaro.org> > wrote: > >>> > >>> Ping! > >>> > >>> > >>> On 3 April 2017 at 13:00, Bogdan Pricope <bogdan.pricope@linaro.org> > >>> wrote: > >>> > Signed-off-by: Bogdan Pricope <bogdan.pricope@linaro.org> > >>> > --- > >>> > example/generator/odp_generator.c | 89 > >>> > ++++++++++++++++++--------------------- > >>> > 1 file changed, 41 insertions(+), 48 deletions(-) > >>> > > >>> > diff --git a/example/generator/odp_generator.c > >>> > b/example/generator/odp_generator.c > >>> > index 95fb543..9c49d94 100644 > >>> > --- a/example/generator/odp_generator.c > >>> > +++ b/example/generator/odp_generator.c > >>> > @@ -119,7 +119,6 @@ static void parse_args(int argc, char *argv[], > >>> > appl_args_t *appl_args); > >>> > static void print_info(char *progname, appl_args_t *appl_args); > >>> > static void usage(char *progname); > >>> > static int scan_ip(char *buf, unsigned int *paddr); > >>> > -static void tv_sub(struct timeval *recvtime, struct timeval > *sendtime); > >>> > static void print_global_stats(int num_workers); > >>> > > >>> > /** > >>> > @@ -348,7 +347,7 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t > pool, > >>> > odp_packet_t pkt_ref) > >>> > char *buf; > >>> > odph_ipv4hdr_t *ip; > >>> > odph_icmphdr_t *icmp; > >>> > - struct timeval tval; > >>> > + uint64_t tval; > >>> > uint8_t *tval_d; > >>> > unsigned short seq; > >>> > > >>> > @@ -372,12 +371,12 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t > pool, > >>> > odp_packet_t pkt_ref) > >>> > /* icmp */ > >>> > icmp = (odph_icmphdr_t *)(buf + ODPH_ETHHDR_LEN + > >>> > ODPH_IPV4HDR_LEN); > >>> > icmp->un.echo.sequence = ip->id; > >>> > + > >>> > tval_d = (uint8_t *)(buf + ODPH_ETHHDR_LEN + > ODPH_IPV4HDR_LEN + > >>> > ODPH_ICMPHDR_LEN); > >>> > - /* TODO This should be changed to use an > >>> > - * ODP timer API once one exists. */ > >>> > - gettimeofday(&tval, NULL); > >>> > - memcpy(tval_d, &tval, sizeof(struct timeval)); > >>> > + tval = odp_time_to_ns(odp_time_local()); > >>> > + memcpy(tval_d, &tval, sizeof(uint64_t)); > >>> > + > >>> > icmp->chksum = 0; > >>> > icmp->chksum = odph_chksum(icmp, args->appl.payload + > >>> > ODPH_ICMPHDR_LEN); > >>> > > >>> > @@ -594,6 +593,40 @@ static int gen_send_thread(void *arg) > >>> > } > >>> > > >>> > /** > >>> > + * Process icmp packets > >>> > + * > >>> > + * @param icmp icmp header address > >>> > + * @param msg output buffer > >>> > + */ > >>> > + > >>> > +static void process_icmp_pkt(odph_icmphdr_t *icmp, char *msg) > >>> > +{ > >>> > + uint64_t trecv; > >>> > + uint64_t tsend; > >>> > + uint64_t rtt_ms, rtt_us; > >>> > + > >>> > + msg[0] = 0; > >>> > + > >>> > + if (icmp->type == ICMP_ECHOREPLY) { > >>> > + odp_atomic_inc_u64(&counters.icmp); > >>> > + > >>> > + memcpy(&tsend, (uint8_t *)icmp + ODPH_ICMPHDR_LEN, > >>> > + sizeof(uint64_t)); > >>> > + trecv = odp_time_to_ns(odp_time_local()); > >>> > + rtt_ms = (trecv - tsend) / ODP_TIME_MSEC_IN_NS; > >>> > + rtt_us = (trecv - tsend) / ODP_TIME_USEC_IN_NS - > >>> > + 1000 * rtt_ms; > >>> > + sprintf(msg, > >>> > + "ICMP Echo Reply seq %d time %" > >>> > + PRIu64 ".%.03" PRIu64" ms", > >>> > + odp_be_to_cpu_16(icmp->un.echo.sequence), > >>> > + rtt_ms, rtt_us); > >>> > + } else if (icmp->type == ICMP_ECHO) { > >>> > + sprintf(msg, "Icmp Echo Request"); > >>> > + } > >>> > +} > >>> > + > >>> > +/** > >>> > * Print odp packets > >>> > * > >>> > * @param thr worker id > >>> > @@ -606,16 +639,12 @@ static void print_pkts(int thr, odp_packet_t > >>> > pkt_tbl[], unsigned len) > >>> > char *buf; > >>> > odph_ipv4hdr_t *ip; > >>> > odph_icmphdr_t *icmp; > >>> > - struct timeval tvrecv; > >>> > - struct timeval tvsend; > >>> > - double rtt; > >>> > unsigned i; > >>> > size_t offset; > >>> > char msg[1024]; > >>> > - int rlen; > >>> > + > >>> > for (i = 0; i < len; ++i) { > >>> > pkt = pkt_tbl[i]; > >>> > - rlen = 0; > >>> > > >>> > /* only ip pkts */ > >>> > if (!odp_packet_has_ipv4(pkt)) > >>> > @@ -634,26 +663,8 @@ static void print_pkts(int thr, odp_packet_t > >>> > pkt_tbl[], unsigned len) > >>> > /* icmp */ > >>> > if (ip->proto == ODPH_IPPROTO_ICMP) { > >>> > icmp = (odph_icmphdr_t *)(buf + offset); > >>> > - /* echo reply */ > >>> > - if (icmp->type == ICMP_ECHOREPLY) { > >>> > - odp_atomic_inc_u64(&counters.icmp); > >>> > - memcpy(&tvsend, buf + offset + > >>> > ODPH_ICMPHDR_LEN, > >>> > - sizeof(struct timeval)); > >>> > - /* TODO This should be changed to > use an > >>> > - * ODP timer API once one exists. */ > >>> > - gettimeofday(&tvrecv, NULL); > >>> > - tv_sub(&tvrecv, &tvsend); > >>> > - rtt = tvrecv.tv_sec*1000 + > >>> > tvrecv.tv_usec/1000; > >>> > - rlen += sprintf(msg + rlen, > >>> > - "ICMP Echo Reply seq %d time > >>> > %.1f ", > >>> > - > >>> > odp_be_to_cpu_16(icmp->un.echo.sequence) > >>> > - , rtt); > >>> > - } else if (icmp->type == ICMP_ECHO) { > >>> > - rlen += sprintf(msg + rlen, > >>> > - "Icmp Echo Request"); > >>> > - } > >>> > > >>> > - msg[rlen] = '\0'; > >>> > + process_icmp_pkt(icmp, msg); > >>> > printf(" [%02i] %s\n", thr, msg); > >>> > } > >>> > } > >>> > @@ -1392,21 +1403,3 @@ static void usage(char *progname) > >>> > "\n", NO_PATH(progname), NO_PATH(progname) > >>> > ); > >>> > } > >>> > -/** > >>> > - * calc time period > >>> > - * > >>> > - *@param recvtime start time > >>> > - *@param sendtime end time > >>> > -*/ > >>> > -static void tv_sub(struct timeval *recvtime, struct timeval > *sendtime) > >>> > -{ > >>> > - long sec = recvtime->tv_sec - sendtime->tv_sec; > >>> > - long usec = recvtime->tv_usec - sendtime->tv_usec; > >>> > - if (usec >= 0) { > >>> > - recvtime->tv_sec = sec; > >>> > - recvtime->tv_usec = usec; > >>> > - } else { > >>> > - recvtime->tv_sec = sec - 1; > >>> > - recvtime->tv_usec = -usec; > >>> > - } > >>> > -} > >>> > -- > >>> > 1.9.1 > >>> > > >> > >> >
Hi Yi, As I said in my reply, the sender is also the interpreter of the data. The peer must only send the payload unmodified in echo reply - it is not relevant if it understands or not the payload (it is opaque data for peer): you can ping la Ubuntu or FreeBSD machine... don't expect them to understand the payload but only to send it back unmodified. Best regards, Bogdan On 24 May 2017 at 11:03, Yi He <yi.he@linaro.org> wrote: > O, sorry Bogdan missed your reply, > > I saw the time value in ICMP message was filled(sending) by "memcpy(tval_d, > ..., sizeof(uint64_t));" and processed(receiving) as "memcpy(&tsend, ..., > sizeof(uint64_t));" > > If the sending peer is little endian and the receiving peer is big endian I > think it will cause wrong interpretation, right? > > Best Regards, Yi > > On 24 May 2017 at 15:48, Bogdan Pricope <bogdan.pricope@linaro.org> wrote: >> >> Ping. >> >> On 3 May 2017 at 11:45, Bogdan Pricope <bogdan.pricope@linaro.org> wrote: >> > Hi Yi, >> > >> > I disagree: bytes sent in Data field of Echo request are opaque and >> > have no meaning for any other entity then ‘me’ (the sender of echo >> > request / receiver of echo reply). >> > >> > The only obligation of the receiver of echo request, as stated by RFC >> > 792 is: >> > “The data received in the echo message must be returned in the echo >> > reply message.” >> > >> > There is no standardization on what should be there (some even using >> > it to convey commands/data – see „Loki ICMP Tunneling”) so it can be >> > whatever sender wants –endianness transformations are not useful in >> > this case. >> > >> > BR, >> > Bogdan >> > >> > On 19 April 2017 at 06:08, Yi He <yi.he@linaro.org> wrote: >> >> Hi, Bogdan >> >> >> >> Consider endianness for this uint64_t time value carried in ICMP >> >> packets, >> >> need htonl and ntohl here. >> >> >> >> Best Regards, Yi >> >> >> >> >> >> >> >> On 11 April 2017 at 19:31, Bogdan Pricope <bogdan.pricope@linaro.org> >> >> wrote: >> >>> >> >>> Ping! >> >>> >> >>> >> >>> On 3 April 2017 at 13:00, Bogdan Pricope <bogdan.pricope@linaro.org> >> >>> wrote: >> >>> > Signed-off-by: Bogdan Pricope <bogdan.pricope@linaro.org> >> >>> > --- >> >>> > example/generator/odp_generator.c | 89 >> >>> > ++++++++++++++++++--------------------- >> >>> > 1 file changed, 41 insertions(+), 48 deletions(-) >> >>> > >> >>> > diff --git a/example/generator/odp_generator.c >> >>> > b/example/generator/odp_generator.c >> >>> > index 95fb543..9c49d94 100644 >> >>> > --- a/example/generator/odp_generator.c >> >>> > +++ b/example/generator/odp_generator.c >> >>> > @@ -119,7 +119,6 @@ static void parse_args(int argc, char *argv[], >> >>> > appl_args_t *appl_args); >> >>> > static void print_info(char *progname, appl_args_t *appl_args); >> >>> > static void usage(char *progname); >> >>> > static int scan_ip(char *buf, unsigned int *paddr); >> >>> > -static void tv_sub(struct timeval *recvtime, struct timeval >> >>> > *sendtime); >> >>> > static void print_global_stats(int num_workers); >> >>> > >> >>> > /** >> >>> > @@ -348,7 +347,7 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t >> >>> > pool, >> >>> > odp_packet_t pkt_ref) >> >>> > char *buf; >> >>> > odph_ipv4hdr_t *ip; >> >>> > odph_icmphdr_t *icmp; >> >>> > - struct timeval tval; >> >>> > + uint64_t tval; >> >>> > uint8_t *tval_d; >> >>> > unsigned short seq; >> >>> > >> >>> > @@ -372,12 +371,12 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t >> >>> > pool, >> >>> > odp_packet_t pkt_ref) >> >>> > /* icmp */ >> >>> > icmp = (odph_icmphdr_t *)(buf + ODPH_ETHHDR_LEN + >> >>> > ODPH_IPV4HDR_LEN); >> >>> > icmp->un.echo.sequence = ip->id; >> >>> > + >> >>> > tval_d = (uint8_t *)(buf + ODPH_ETHHDR_LEN + >> >>> > ODPH_IPV4HDR_LEN + >> >>> > ODPH_ICMPHDR_LEN); >> >>> > - /* TODO This should be changed to use an >> >>> > - * ODP timer API once one exists. */ >> >>> > - gettimeofday(&tval, NULL); >> >>> > - memcpy(tval_d, &tval, sizeof(struct timeval)); >> >>> > + tval = odp_time_to_ns(odp_time_local()); >> >>> > + memcpy(tval_d, &tval, sizeof(uint64_t)); >> >>> > + >> >>> > icmp->chksum = 0; >> >>> > icmp->chksum = odph_chksum(icmp, args->appl.payload + >> >>> > ODPH_ICMPHDR_LEN); >> >>> > >> >>> > @@ -594,6 +593,40 @@ static int gen_send_thread(void *arg) >> >>> > } >> >>> > >> >>> > /** >> >>> > + * Process icmp packets >> >>> > + * >> >>> > + * @param icmp icmp header address >> >>> > + * @param msg output buffer >> >>> > + */ >> >>> > + >> >>> > +static void process_icmp_pkt(odph_icmphdr_t *icmp, char *msg) >> >>> > +{ >> >>> > + uint64_t trecv; >> >>> > + uint64_t tsend; >> >>> > + uint64_t rtt_ms, rtt_us; >> >>> > + >> >>> > + msg[0] = 0; >> >>> > + >> >>> > + if (icmp->type == ICMP_ECHOREPLY) { >> >>> > + odp_atomic_inc_u64(&counters.icmp); >> >>> > + >> >>> > + memcpy(&tsend, (uint8_t *)icmp + ODPH_ICMPHDR_LEN, >> >>> > + sizeof(uint64_t)); >> >>> > + trecv = odp_time_to_ns(odp_time_local()); >> >>> > + rtt_ms = (trecv - tsend) / ODP_TIME_MSEC_IN_NS; >> >>> > + rtt_us = (trecv - tsend) / ODP_TIME_USEC_IN_NS - >> >>> > + 1000 * rtt_ms; >> >>> > + sprintf(msg, >> >>> > + "ICMP Echo Reply seq %d time %" >> >>> > + PRIu64 ".%.03" PRIu64" ms", >> >>> > + odp_be_to_cpu_16(icmp->un.echo.sequence), >> >>> > + rtt_ms, rtt_us); >> >>> > + } else if (icmp->type == ICMP_ECHO) { >> >>> > + sprintf(msg, "Icmp Echo Request"); >> >>> > + } >> >>> > +} >> >>> > + >> >>> > +/** >> >>> > * Print odp packets >> >>> > * >> >>> > * @param thr worker id >> >>> > @@ -606,16 +639,12 @@ static void print_pkts(int thr, odp_packet_t >> >>> > pkt_tbl[], unsigned len) >> >>> > char *buf; >> >>> > odph_ipv4hdr_t *ip; >> >>> > odph_icmphdr_t *icmp; >> >>> > - struct timeval tvrecv; >> >>> > - struct timeval tvsend; >> >>> > - double rtt; >> >>> > unsigned i; >> >>> > size_t offset; >> >>> > char msg[1024]; >> >>> > - int rlen; >> >>> > + >> >>> > for (i = 0; i < len; ++i) { >> >>> > pkt = pkt_tbl[i]; >> >>> > - rlen = 0; >> >>> > >> >>> > /* only ip pkts */ >> >>> > if (!odp_packet_has_ipv4(pkt)) >> >>> > @@ -634,26 +663,8 @@ static void print_pkts(int thr, odp_packet_t >> >>> > pkt_tbl[], unsigned len) >> >>> > /* icmp */ >> >>> > if (ip->proto == ODPH_IPPROTO_ICMP) { >> >>> > icmp = (odph_icmphdr_t *)(buf + offset); >> >>> > - /* echo reply */ >> >>> > - if (icmp->type == ICMP_ECHOREPLY) { >> >>> > - odp_atomic_inc_u64(&counters.icmp); >> >>> > - memcpy(&tvsend, buf + offset + >> >>> > ODPH_ICMPHDR_LEN, >> >>> > - sizeof(struct timeval)); >> >>> > - /* TODO This should be changed to >> >>> > use an >> >>> > - * ODP timer API once one exists. */ >> >>> > - gettimeofday(&tvrecv, NULL); >> >>> > - tv_sub(&tvrecv, &tvsend); >> >>> > - rtt = tvrecv.tv_sec*1000 + >> >>> > tvrecv.tv_usec/1000; >> >>> > - rlen += sprintf(msg + rlen, >> >>> > - "ICMP Echo Reply seq %d time >> >>> > %.1f ", >> >>> > - >> >>> > odp_be_to_cpu_16(icmp->un.echo.sequence) >> >>> > - , rtt); >> >>> > - } else if (icmp->type == ICMP_ECHO) { >> >>> > - rlen += sprintf(msg + rlen, >> >>> > - "Icmp Echo >> >>> > Request"); >> >>> > - } >> >>> > >> >>> > - msg[rlen] = '\0'; >> >>> > + process_icmp_pkt(icmp, msg); >> >>> > printf(" [%02i] %s\n", thr, msg); >> >>> > } >> >>> > } >> >>> > @@ -1392,21 +1403,3 @@ static void usage(char *progname) >> >>> > "\n", NO_PATH(progname), NO_PATH(progname) >> >>> > ); >> >>> > } >> >>> > -/** >> >>> > - * calc time period >> >>> > - * >> >>> > - *@param recvtime start time >> >>> > - *@param sendtime end time >> >>> > -*/ >> >>> > -static void tv_sub(struct timeval *recvtime, struct timeval >> >>> > *sendtime) >> >>> > -{ >> >>> > - long sec = recvtime->tv_sec - sendtime->tv_sec; >> >>> > - long usec = recvtime->tv_usec - sendtime->tv_usec; >> >>> > - if (usec >= 0) { >> >>> > - recvtime->tv_sec = sec; >> >>> > - recvtime->tv_usec = usec; >> >>> > - } else { >> >>> > - recvtime->tv_sec = sec - 1; >> >>> > - recvtime->tv_usec = -usec; >> >>> > - } >> >>> > -} >> >>> > -- >> >>> > 1.9.1 >> >>> > >> >> >> >> > >
O, yes, I see, it is the same entity to process both ICMP request/reply and can piggyback opaque payload, sorry for the misunderstanding, this still applied to the master branch and: Reviewed-and-tested-by: Yi He <yi.he@linaro.org> On 24 May 2017 at 16:12, Bogdan Pricope <bogdan.pricope@linaro.org> wrote: > Hi Yi, > > As I said in my reply, the sender is also the interpreter of the data. > The peer must only send the payload unmodified in echo reply - it is > not relevant if it understands or not the payload (it is opaque data > for peer): you can ping la Ubuntu or FreeBSD machine... don't expect > them to understand the payload but only to send it back unmodified. > > Best regards, > Bogdan > > On 24 May 2017 at 11:03, Yi He <yi.he@linaro.org> wrote: > > O, sorry Bogdan missed your reply, > > > > I saw the time value in ICMP message was filled(sending) by > "memcpy(tval_d, > > ..., sizeof(uint64_t));" and processed(receiving) as "memcpy(&tsend, ..., > > sizeof(uint64_t));" > > > > If the sending peer is little endian and the receiving peer is big > endian I > > think it will cause wrong interpretation, right? > > > > Best Regards, Yi > > > > On 24 May 2017 at 15:48, Bogdan Pricope <bogdan.pricope@linaro.org> > wrote: > >> > >> Ping. > >> > >> On 3 May 2017 at 11:45, Bogdan Pricope <bogdan.pricope@linaro.org> > wrote: > >> > Hi Yi, > >> > > >> > I disagree: bytes sent in Data field of Echo request are opaque and > >> > have no meaning for any other entity then ‘me’ (the sender of echo > >> > request / receiver of echo reply). > >> > > >> > The only obligation of the receiver of echo request, as stated by RFC > >> > 792 is: > >> > “The data received in the echo message must be returned in the echo > >> > reply message.” > >> > > >> > There is no standardization on what should be there (some even using > >> > it to convey commands/data – see „Loki ICMP Tunneling”) so it can be > >> > whatever sender wants –endianness transformations are not useful in > >> > this case. > >> > > >> > BR, > >> > Bogdan > >> > > >> > On 19 April 2017 at 06:08, Yi He <yi.he@linaro.org> wrote: > >> >> Hi, Bogdan > >> >> > >> >> Consider endianness for this uint64_t time value carried in ICMP > >> >> packets, > >> >> need htonl and ntohl here. > >> >> > >> >> Best Regards, Yi > >> >> > >> >> > >> >> > >> >> On 11 April 2017 at 19:31, Bogdan Pricope <bogdan.pricope@linaro.org > > > >> >> wrote: > >> >>> > >> >>> Ping! > >> >>> > >> >>> > >> >>> On 3 April 2017 at 13:00, Bogdan Pricope <bogdan.pricope@linaro.org > > > >> >>> wrote: > >> >>> > Signed-off-by: Bogdan Pricope <bogdan.pricope@linaro.org> > >> >>> > --- > >> >>> > example/generator/odp_generator.c | 89 > >> >>> > ++++++++++++++++++--------------------- > >> >>> > 1 file changed, 41 insertions(+), 48 deletions(-) > >> >>> > > >> >>> > diff --git a/example/generator/odp_generator.c > >> >>> > b/example/generator/odp_generator.c > >> >>> > index 95fb543..9c49d94 100644 > >> >>> > --- a/example/generator/odp_generator.c > >> >>> > +++ b/example/generator/odp_generator.c > >> >>> > @@ -119,7 +119,6 @@ static void parse_args(int argc, char *argv[], > >> >>> > appl_args_t *appl_args); > >> >>> > static void print_info(char *progname, appl_args_t *appl_args); > >> >>> > static void usage(char *progname); > >> >>> > static int scan_ip(char *buf, unsigned int *paddr); > >> >>> > -static void tv_sub(struct timeval *recvtime, struct timeval > >> >>> > *sendtime); > >> >>> > static void print_global_stats(int num_workers); > >> >>> > > >> >>> > /** > >> >>> > @@ -348,7 +347,7 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t > >> >>> > pool, > >> >>> > odp_packet_t pkt_ref) > >> >>> > char *buf; > >> >>> > odph_ipv4hdr_t *ip; > >> >>> > odph_icmphdr_t *icmp; > >> >>> > - struct timeval tval; > >> >>> > + uint64_t tval; > >> >>> > uint8_t *tval_d; > >> >>> > unsigned short seq; > >> >>> > > >> >>> > @@ -372,12 +371,12 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t > >> >>> > pool, > >> >>> > odp_packet_t pkt_ref) > >> >>> > /* icmp */ > >> >>> > icmp = (odph_icmphdr_t *)(buf + ODPH_ETHHDR_LEN + > >> >>> > ODPH_IPV4HDR_LEN); > >> >>> > icmp->un.echo.sequence = ip->id; > >> >>> > + > >> >>> > tval_d = (uint8_t *)(buf + ODPH_ETHHDR_LEN + > >> >>> > ODPH_IPV4HDR_LEN + > >> >>> > ODPH_ICMPHDR_LEN); > >> >>> > - /* TODO This should be changed to use an > >> >>> > - * ODP timer API once one exists. */ > >> >>> > - gettimeofday(&tval, NULL); > >> >>> > - memcpy(tval_d, &tval, sizeof(struct timeval)); > >> >>> > + tval = odp_time_to_ns(odp_time_local()); > >> >>> > + memcpy(tval_d, &tval, sizeof(uint64_t)); > >> >>> > + > >> >>> > icmp->chksum = 0; > >> >>> > icmp->chksum = odph_chksum(icmp, args->appl.payload + > >> >>> > ODPH_ICMPHDR_LEN); > >> >>> > > >> >>> > @@ -594,6 +593,40 @@ static int gen_send_thread(void *arg) > >> >>> > } > >> >>> > > >> >>> > /** > >> >>> > + * Process icmp packets > >> >>> > + * > >> >>> > + * @param icmp icmp header address > >> >>> > + * @param msg output buffer > >> >>> > + */ > >> >>> > + > >> >>> > +static void process_icmp_pkt(odph_icmphdr_t *icmp, char *msg) > >> >>> > +{ > >> >>> > + uint64_t trecv; > >> >>> > + uint64_t tsend; > >> >>> > + uint64_t rtt_ms, rtt_us; > >> >>> > + > >> >>> > + msg[0] = 0; > >> >>> > + > >> >>> > + if (icmp->type == ICMP_ECHOREPLY) { > >> >>> > + odp_atomic_inc_u64(&counters.icmp); > >> >>> > + > >> >>> > + memcpy(&tsend, (uint8_t *)icmp + ODPH_ICMPHDR_LEN, > >> >>> > + sizeof(uint64_t)); > >> >>> > + trecv = odp_time_to_ns(odp_time_local()); > >> >>> > + rtt_ms = (trecv - tsend) / ODP_TIME_MSEC_IN_NS; > >> >>> > + rtt_us = (trecv - tsend) / ODP_TIME_USEC_IN_NS - > >> >>> > + 1000 * rtt_ms; > >> >>> > + sprintf(msg, > >> >>> > + "ICMP Echo Reply seq %d time %" > >> >>> > + PRIu64 ".%.03" PRIu64" ms", > >> >>> > + odp_be_to_cpu_16(icmp->un.echo.sequence), > >> >>> > + rtt_ms, rtt_us); > >> >>> > + } else if (icmp->type == ICMP_ECHO) { > >> >>> > + sprintf(msg, "Icmp Echo Request"); > >> >>> > + } > >> >>> > +} > >> >>> > + > >> >>> > +/** > >> >>> > * Print odp packets > >> >>> > * > >> >>> > * @param thr worker id > >> >>> > @@ -606,16 +639,12 @@ static void print_pkts(int thr, odp_packet_t > >> >>> > pkt_tbl[], unsigned len) > >> >>> > char *buf; > >> >>> > odph_ipv4hdr_t *ip; > >> >>> > odph_icmphdr_t *icmp; > >> >>> > - struct timeval tvrecv; > >> >>> > - struct timeval tvsend; > >> >>> > - double rtt; > >> >>> > unsigned i; > >> >>> > size_t offset; > >> >>> > char msg[1024]; > >> >>> > - int rlen; > >> >>> > + > >> >>> > for (i = 0; i < len; ++i) { > >> >>> > pkt = pkt_tbl[i]; > >> >>> > - rlen = 0; > >> >>> > > >> >>> > /* only ip pkts */ > >> >>> > if (!odp_packet_has_ipv4(pkt)) > >> >>> > @@ -634,26 +663,8 @@ static void print_pkts(int thr, odp_packet_t > >> >>> > pkt_tbl[], unsigned len) > >> >>> > /* icmp */ > >> >>> > if (ip->proto == ODPH_IPPROTO_ICMP) { > >> >>> > icmp = (odph_icmphdr_t *)(buf + offset); > >> >>> > - /* echo reply */ > >> >>> > - if (icmp->type == ICMP_ECHOREPLY) { > >> >>> > - odp_atomic_inc_u64(&counters. > icmp); > >> >>> > - memcpy(&tvsend, buf + offset + > >> >>> > ODPH_ICMPHDR_LEN, > >> >>> > - sizeof(struct timeval)); > >> >>> > - /* TODO This should be changed to > >> >>> > use an > >> >>> > - * ODP timer API once one exists. > */ > >> >>> > - gettimeofday(&tvrecv, NULL); > >> >>> > - tv_sub(&tvrecv, &tvsend); > >> >>> > - rtt = tvrecv.tv_sec*1000 + > >> >>> > tvrecv.tv_usec/1000; > >> >>> > - rlen += sprintf(msg + rlen, > >> >>> > - "ICMP Echo Reply seq %d > time > >> >>> > %.1f ", > >> >>> > - > >> >>> > odp_be_to_cpu_16(icmp->un.echo.sequence) > >> >>> > - , rtt); > >> >>> > - } else if (icmp->type == ICMP_ECHO) { > >> >>> > - rlen += sprintf(msg + rlen, > >> >>> > - "Icmp Echo > >> >>> > Request"); > >> >>> > - } > >> >>> > > >> >>> > - msg[rlen] = '\0'; > >> >>> > + process_icmp_pkt(icmp, msg); > >> >>> > printf(" [%02i] %s\n", thr, msg); > >> >>> > } > >> >>> > } > >> >>> > @@ -1392,21 +1403,3 @@ static void usage(char *progname) > >> >>> > "\n", NO_PATH(progname), NO_PATH(progname) > >> >>> > ); > >> >>> > } > >> >>> > -/** > >> >>> > - * calc time period > >> >>> > - * > >> >>> > - *@param recvtime start time > >> >>> > - *@param sendtime end time > >> >>> > -*/ > >> >>> > -static void tv_sub(struct timeval *recvtime, struct timeval > >> >>> > *sendtime) > >> >>> > -{ > >> >>> > - long sec = recvtime->tv_sec - sendtime->tv_sec; > >> >>> > - long usec = recvtime->tv_usec - sendtime->tv_usec; > >> >>> > - if (usec >= 0) { > >> >>> > - recvtime->tv_sec = sec; > >> >>> > - recvtime->tv_usec = usec; > >> >>> > - } else { > >> >>> > - recvtime->tv_sec = sec - 1; > >> >>> > - recvtime->tv_usec = -usec; > >> >>> > - } > >> >>> > -} > >> >>> > -- > >> >>> > 1.9.1 > >> >>> > > >> >> > >> >> > > > > >
diff --git a/example/generator/odp_generator.c b/example/generator/odp_generator.c index 95fb543..9c49d94 100644 --- a/example/generator/odp_generator.c +++ b/example/generator/odp_generator.c @@ -119,7 +119,6 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args); static void print_info(char *progname, appl_args_t *appl_args); static void usage(char *progname); static int scan_ip(char *buf, unsigned int *paddr); -static void tv_sub(struct timeval *recvtime, struct timeval *sendtime); static void print_global_stats(int num_workers); /** @@ -348,7 +347,7 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t pool, odp_packet_t pkt_ref) char *buf; odph_ipv4hdr_t *ip; odph_icmphdr_t *icmp; - struct timeval tval; + uint64_t tval; uint8_t *tval_d; unsigned short seq; @@ -372,12 +371,12 @@ static odp_packet_t pack_icmp_pkt(odp_pool_t pool, odp_packet_t pkt_ref) /* icmp */ icmp = (odph_icmphdr_t *)(buf + ODPH_ETHHDR_LEN + ODPH_IPV4HDR_LEN); icmp->un.echo.sequence = ip->id; + tval_d = (uint8_t *)(buf + ODPH_ETHHDR_LEN + ODPH_IPV4HDR_LEN + ODPH_ICMPHDR_LEN); - /* TODO This should be changed to use an - * ODP timer API once one exists. */ - gettimeofday(&tval, NULL); - memcpy(tval_d, &tval, sizeof(struct timeval)); + tval = odp_time_to_ns(odp_time_local()); + memcpy(tval_d, &tval, sizeof(uint64_t)); + icmp->chksum = 0; icmp->chksum = odph_chksum(icmp, args->appl.payload + ODPH_ICMPHDR_LEN); @@ -594,6 +593,40 @@ static int gen_send_thread(void *arg) } /** + * Process icmp packets + * + * @param icmp icmp header address + * @param msg output buffer + */ + +static void process_icmp_pkt(odph_icmphdr_t *icmp, char *msg) +{ + uint64_t trecv; + uint64_t tsend; + uint64_t rtt_ms, rtt_us; + + msg[0] = 0; + + if (icmp->type == ICMP_ECHOREPLY) { + odp_atomic_inc_u64(&counters.icmp); + + memcpy(&tsend, (uint8_t *)icmp + ODPH_ICMPHDR_LEN, + sizeof(uint64_t)); + trecv = odp_time_to_ns(odp_time_local()); + rtt_ms = (trecv - tsend) / ODP_TIME_MSEC_IN_NS; + rtt_us = (trecv - tsend) / ODP_TIME_USEC_IN_NS - + 1000 * rtt_ms; + sprintf(msg, + "ICMP Echo Reply seq %d time %" + PRIu64 ".%.03" PRIu64" ms", + odp_be_to_cpu_16(icmp->un.echo.sequence), + rtt_ms, rtt_us); + } else if (icmp->type == ICMP_ECHO) { + sprintf(msg, "Icmp Echo Request"); + } +} + +/** * Print odp packets * * @param thr worker id @@ -606,16 +639,12 @@ static void print_pkts(int thr, odp_packet_t pkt_tbl[], unsigned len) char *buf; odph_ipv4hdr_t *ip; odph_icmphdr_t *icmp; - struct timeval tvrecv; - struct timeval tvsend; - double rtt; unsigned i; size_t offset; char msg[1024]; - int rlen; + for (i = 0; i < len; ++i) { pkt = pkt_tbl[i]; - rlen = 0; /* only ip pkts */ if (!odp_packet_has_ipv4(pkt)) @@ -634,26 +663,8 @@ static void print_pkts(int thr, odp_packet_t pkt_tbl[], unsigned len) /* icmp */ if (ip->proto == ODPH_IPPROTO_ICMP) { icmp = (odph_icmphdr_t *)(buf + offset); - /* echo reply */ - if (icmp->type == ICMP_ECHOREPLY) { - odp_atomic_inc_u64(&counters.icmp); - memcpy(&tvsend, buf + offset + ODPH_ICMPHDR_LEN, - sizeof(struct timeval)); - /* TODO This should be changed to use an - * ODP timer API once one exists. */ - gettimeofday(&tvrecv, NULL); - tv_sub(&tvrecv, &tvsend); - rtt = tvrecv.tv_sec*1000 + tvrecv.tv_usec/1000; - rlen += sprintf(msg + rlen, - "ICMP Echo Reply seq %d time %.1f ", - odp_be_to_cpu_16(icmp->un.echo.sequence) - , rtt); - } else if (icmp->type == ICMP_ECHO) { - rlen += sprintf(msg + rlen, - "Icmp Echo Request"); - } - msg[rlen] = '\0'; + process_icmp_pkt(icmp, msg); printf(" [%02i] %s\n", thr, msg); } } @@ -1392,21 +1403,3 @@ static void usage(char *progname) "\n", NO_PATH(progname), NO_PATH(progname) ); } -/** - * calc time period - * - *@param recvtime start time - *@param sendtime end time -*/ -static void tv_sub(struct timeval *recvtime, struct timeval *sendtime) -{ - long sec = recvtime->tv_sec - sendtime->tv_sec; - long usec = recvtime->tv_usec - sendtime->tv_usec; - if (usec >= 0) { - recvtime->tv_sec = sec; - recvtime->tv_usec = usec; - } else { - recvtime->tv_sec = sec - 1; - recvtime->tv_usec = -usec; - } -}
Signed-off-by: Bogdan Pricope <bogdan.pricope@linaro.org> --- example/generator/odp_generator.c | 89 ++++++++++++++++++--------------------- 1 file changed, 41 insertions(+), 48 deletions(-) -- 1.9.1