Main idea for this work is to minimize the work needed to be done in packet_get(), which is the hottest path in the program.
Cheers, Vita
Vitezslav Samel (7): refactor struct pkt_hdr initialization refactor pkt_cleanup() packet_get(): optimize - move struct iovec packet_get(): optimize - move struct sockaddr_ll packet_get(): optimize - move struct msghdr packet_get(): optimize - remove cache variables pkt_hdr: make pkt_buf allocated from heap
src/capture-pkt.c | 12 ++++++---- src/detstats.c | 10 ++++---- src/hostmon.c | 18 ++++++++------ src/ifstats.c | 8 ++++--- src/itrafmon.c | 12 ++++++---- src/othptab.c | 6 ++--- src/packet.c | 70 ++++++++++++++++++++++++++++++++++++++----------------- src/packet.h | 30 +++++++----------------- src/pktsize.c | 6 ++++- src/serv.c | 6 +++-- 10 files changed, 105 insertions(+), 73 deletions(-)
1) dissect declaration and initialization of struct pkt_hdr 2) rename PACKET_INIT_STRUCT() to packet_init() 3) un-inline packet_init(): it's not used on any fast path so there's no need to be inlined 4) capture-pkt.c: rename p to pkt to have it consistent with other uses
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/capture-pkt.c | 10 ++++++---- src/detstats.c | 4 +++- src/hostmon.c | 4 +++- src/ifstats.c | 4 +++- src/itrafmon.c | 4 +++- src/packet.c | 13 +++++++++++++ src/packet.h | 16 +--------------- src/pktsize.c | 4 +++- src/serv.c | 4 +++- 9 files changed, 38 insertions(+), 25 deletions(-)
diff --git a/src/capture-pkt.c b/src/capture-pkt.c index 6df4611..25e9f2b 100644 --- a/src/capture-pkt.c +++ b/src/capture-pkt.c @@ -47,20 +47,22 @@ int cmd_capture(int argc, char **argv) die_errno("fopen"); }
- PACKET_INIT(p); + struct pkt_hdr pkt; + packet_init(&pkt); + int captured = 0; for (;;) { - if (packet_get(fd, &p, NULL, NULL) == -1) + if (packet_get(fd, &pkt, NULL, NULL) == -1) die_errno("fail to get packet");
- if (!p.pkt_len) + if (!pkt.pkt_len) continue;
printf("."); fflush(stdout);
if (fp) - fwrite(&p, sizeof(p), 1, fp); + fwrite(&pkt, sizeof(pkt), 1, fp);
if (++captured == cap_nr_pkt) break; diff --git a/src/detstats.c b/src/detstats.c index 646893f..0f96e9b 100644 --- a/src/detstats.c +++ b/src/detstats.c @@ -290,6 +290,8 @@ void detstats(char *iface, time_t facilitytime)
int fd;
+ struct pkt_hdr pkt; + if (!dev_up(iface)) { err_iface_down(); return; @@ -378,7 +380,7 @@ void detstats(char *iface, time_t facilitytime)
exitloop = 0;
- PACKET_INIT(pkt); + packet_init(&pkt);
/* * Data-gathering loop diff --git a/src/hostmon.c b/src/hostmon.c index 44dcf40..51267ef 100644 --- a/src/hostmon.c +++ b/src/hostmon.c @@ -758,6 +758,8 @@ void hostmon(time_t facilitytime, char *ifptr)
int fd;
+ struct pkt_hdr pkt; + if (ifptr && !dev_up(ifptr)) { err_iface_down(); return; @@ -821,7 +823,7 @@ void hostmon(time_t facilitytime, char *ifptr) updtime = tv; statbegin = startlog = tv.tv_sec;
- PACKET_INIT(pkt); + packet_init(&pkt);
do { gettimeofday(&tv, NULL); diff --git a/src/ifstats.c b/src/ifstats.c index 2a5bba4..b7eeb7a 100644 --- a/src/ifstats.c +++ b/src/ifstats.c @@ -427,6 +427,8 @@ void ifstats(time_t facilitytime)
int fd;
+ struct pkt_hdr pkt; + struct timeval tv; time_t starttime = 0; time_t statbegin = 0; @@ -491,7 +493,7 @@ void ifstats(time_t facilitytime) updtime = tv; starttime = startlog = statbegin = tv.tv_sec;
- PACKET_INIT(pkt); + packet_init(&pkt);
while (!exitloop) { gettimeofday(&tv, NULL); diff --git a/src/itrafmon.c b/src/itrafmon.c index 20c6f6c..8bf6c3a 100644 --- a/src/itrafmon.c +++ b/src/itrafmon.c @@ -588,6 +588,8 @@ void ipmon(time_t facilitytime, char *ifptr)
int fd;
+ struct pkt_hdr pkt; + int ch; int keymode = 0; char msgstring[80]; @@ -692,7 +694,7 @@ void ipmon(time_t facilitytime, char *ifptr) updtime = tv; starttime = timeint = closedint = tv.tv_sec;
- PACKET_INIT(pkt); + packet_init(&pkt);
while (!exitloop) { char ifnamebuf[IFNAMSIZ]; diff --git a/src/packet.c b/src/packet.c index 4556516..afa767f 100644 --- a/src/packet.c +++ b/src/packet.c @@ -329,6 +329,19 @@ again: return PACKET_OK; }
+int packet_init(struct pkt_hdr *pkt) +{ + pkt->pkt_bufsize = MAX_PACKET_SIZE; + pkt->pkt_payload = NULL; + pkt->ethhdr = NULL; + pkt->fddihdr = NULL; + pkt->iphdr = NULL; + pkt->ip6_hdr = NULL; + pkt->pkt_len = 0; /* signalize we have no packet prepared */ + + return 0; /* all O.K. */ +} + void pkt_cleanup(void) { destroyfraglist(); diff --git a/src/packet.h b/src/packet.h index 895d63b..e8811e6 100644 --- a/src/packet.h +++ b/src/packet.h @@ -38,21 +38,6 @@ struct pkt_hdr { char pkt_buf[MAX_PACKET_SIZE]; };
-static inline void PACKET_INIT_STRUCT(struct pkt_hdr *p) -{ - p->pkt_bufsize = MAX_PACKET_SIZE; - p->pkt_payload = NULL; - p->ethhdr = NULL; - p->fddihdr = NULL; - p->iphdr = NULL; - p->ip6_hdr = NULL; - p->pkt_len = 0; /* signalize we have no packet prepared */ -} - -#define PACKET_INIT(packet) \ - struct pkt_hdr packet; \ - PACKET_INIT_STRUCT(&packet) - static inline __u8 pkt_iph_len(const struct pkt_hdr *pkt) { switch (pkt->pkt_protocol) { @@ -80,6 +65,7 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win); int packet_process(struct pkt_hdr *pkt, unsigned int *total_br, in_port_t *sport, in_port_t *dport, int match_opposite, int v6inv4asv6); +int packet_init(struct pkt_hdr *pkt); void pkt_cleanup(void);
#endif /* IPTRAF_NG_PACKET_H */ diff --git a/src/pktsize.c b/src/pktsize.c index 54fb680..8206919 100644 --- a/src/pktsize.c +++ b/src/pktsize.c @@ -164,6 +164,8 @@ void packet_size_breakdown(char *ifname, time_t facilitytime)
int fd;
+ struct pkt_hdr pkt; + if (!dev_up(ifname)) { err_iface_down(); goto err_unmark; @@ -249,7 +251,7 @@ void packet_size_breakdown(char *ifname, time_t facilitytime) goto err_close; }
- PACKET_INIT(pkt); + packet_init(&pkt);
do { gettimeofday(&tv, NULL); diff --git a/src/serv.c b/src/serv.c index c82bd3a..eecb34a 100644 --- a/src/serv.c +++ b/src/serv.c @@ -769,6 +769,8 @@ void servmon(char *ifname, time_t facilitytime)
struct porttab *ports;
+ struct pkt_hdr pkt; + if (!dev_up(ifname)) { err_iface_down(); return; @@ -842,7 +844,7 @@ void servmon(char *ifname, time_t facilitytime) goto err_close; }
- PACKET_INIT(pkt); + packet_init(&pkt);
while (!exitloop) { gettimeofday(&tv, NULL);
Vitezslav Samel vitezslav@samel.cz writes:
- dissect declaration and initialization of struct pkt_hdr
- rename PACKET_INIT_STRUCT() to packet_init()
- un-inline packet_init(): it's not used on any fast path so there's no need to be inlined
- capture-pkt.c: rename p to pkt to have it consistent with other uses
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/capture-pkt.c | 10 ++++++---- src/detstats.c | 4 +++- src/hostmon.c | 4 +++- src/ifstats.c | 4 +++- src/itrafmon.c | 4 +++- src/packet.c | 13 +++++++++++++ src/packet.h | 16 +--------------- src/pktsize.c | 4 +++- src/serv.c | 4 +++- 9 files changed, 38 insertions(+), 25 deletions(-)
+int packet_init(struct pkt_hdr *pkt) +{
- pkt->pkt_bufsize = MAX_PACKET_SIZE;
- pkt->pkt_payload = NULL;
- pkt->ethhdr = NULL;
- pkt->fddihdr = NULL;
- pkt->iphdr = NULL;
- pkt->ip6_hdr = NULL;
- pkt->pkt_len = 0; /* signalize we have no packet prepared */
- return 0; /* all O.K. */
+}
void pkt_cleanup(void) { destroyfraglist(); diff --git a/src/packet.h b/src/packet.h index 895d63b..e8811e6 100644 --- a/src/packet.h +++ b/src/packet.h @@ -38,21 +38,6 @@ struct pkt_hdr { char pkt_buf[MAX_PACKET_SIZE]; };
-static inline void PACKET_INIT_STRUCT(struct pkt_hdr *p) -{
- p->pkt_bufsize = MAX_PACKET_SIZE;
- p->pkt_payload = NULL;
- p->ethhdr = NULL;
- p->fddihdr = NULL;
- p->iphdr = NULL;
- p->ip6_hdr = NULL;
- p->pkt_len = 0; /* signalize we have no packet prepared */
-}
-#define PACKET_INIT(packet) \
- struct pkt_hdr packet; \
- PACKET_INIT_STRUCT(&packet)
yeah, I have messed up the initialization. What I wanted to have one initialization for compiler time and one for run time.
struct pkt_hdr pkt = PKT_INIT;
where PKT_INIT is macro
#define PKT_INIT { .pkt_bufsize = MAX_PACKET_SIZE, .pkt_payload = NULL ... }
and one you have written for run time. that would be ideal.
hmm?
On Wed, May 07, 2014 at 06:35:29PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
- dissect declaration and initialization of struct pkt_hdr
- rename PACKET_INIT_STRUCT() to packet_init()
- un-inline packet_init(): it's not used on any fast path so there's no need to be inlined
- capture-pkt.c: rename p to pkt to have it consistent with other uses
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/capture-pkt.c | 10 ++++++---- src/detstats.c | 4 +++- src/hostmon.c | 4 +++- src/ifstats.c | 4 +++- src/itrafmon.c | 4 +++- src/packet.c | 13 +++++++++++++ src/packet.h | 16 +--------------- src/pktsize.c | 4 +++- src/serv.c | 4 +++- 9 files changed, 38 insertions(+), 25 deletions(-)
+int packet_init(struct pkt_hdr *pkt) +{
- pkt->pkt_bufsize = MAX_PACKET_SIZE;
- pkt->pkt_payload = NULL;
- pkt->ethhdr = NULL;
- pkt->fddihdr = NULL;
- pkt->iphdr = NULL;
- pkt->ip6_hdr = NULL;
- pkt->pkt_len = 0; /* signalize we have no packet prepared */
- return 0; /* all O.K. */
+}
void pkt_cleanup(void) { destroyfraglist(); diff --git a/src/packet.h b/src/packet.h index 895d63b..e8811e6 100644 --- a/src/packet.h +++ b/src/packet.h @@ -38,21 +38,6 @@ struct pkt_hdr { char pkt_buf[MAX_PACKET_SIZE]; };
-static inline void PACKET_INIT_STRUCT(struct pkt_hdr *p) -{
- p->pkt_bufsize = MAX_PACKET_SIZE;
- p->pkt_payload = NULL;
- p->ethhdr = NULL;
- p->fddihdr = NULL;
- p->iphdr = NULL;
- p->ip6_hdr = NULL;
- p->pkt_len = 0; /* signalize we have no packet prepared */
-}
-#define PACKET_INIT(packet) \
- struct pkt_hdr packet; \
- PACKET_INIT_STRUCT(&packet)
yeah, I have messed up the initialization. What I wanted to have one initialization for compiler time and one for run time.
struct pkt_hdr pkt = PKT_INIT;where PKT_INIT is macro
#define PKT_INIT { .pkt_bufsize = MAX_PACKET_SIZE, .pkt_payload = NULL ... }and one you have written for run time. that would be ideal.
hmm?
Why should we have 2 init routines? In the case packet mmap() case even the compile time initialization will be moved to run time init. I think my patch does this the right way.
Vita
Vitezslav Samel vitezslav@samel.cz writes:
On Wed, May 07, 2014 at 06:35:29PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
- dissect declaration and initialization of struct pkt_hdr
- rename PACKET_INIT_STRUCT() to packet_init()
- un-inline packet_init(): it's not used on any fast path so there's no need to be inlined
- capture-pkt.c: rename p to pkt to have it consistent with other uses
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/capture-pkt.c | 10 ++++++---- src/detstats.c | 4 +++- src/hostmon.c | 4 +++- src/ifstats.c | 4 +++- src/itrafmon.c | 4 +++- src/packet.c | 13 +++++++++++++ src/packet.h | 16 +--------------- src/pktsize.c | 4 +++- src/serv.c | 4 +++- 9 files changed, 38 insertions(+), 25 deletions(-)
+int packet_init(struct pkt_hdr *pkt) +{
- pkt->pkt_bufsize = MAX_PACKET_SIZE;
- pkt->pkt_payload = NULL;
- pkt->ethhdr = NULL;
- pkt->fddihdr = NULL;
- pkt->iphdr = NULL;
- pkt->ip6_hdr = NULL;
- pkt->pkt_len = 0; /* signalize we have no packet prepared */
- return 0; /* all O.K. */
+}
void pkt_cleanup(void) { destroyfraglist(); diff --git a/src/packet.h b/src/packet.h index 895d63b..e8811e6 100644 --- a/src/packet.h +++ b/src/packet.h @@ -38,21 +38,6 @@ struct pkt_hdr { char pkt_buf[MAX_PACKET_SIZE]; };
-static inline void PACKET_INIT_STRUCT(struct pkt_hdr *p) -{
- p->pkt_bufsize = MAX_PACKET_SIZE;
- p->pkt_payload = NULL;
- p->ethhdr = NULL;
- p->fddihdr = NULL;
- p->iphdr = NULL;
- p->ip6_hdr = NULL;
- p->pkt_len = 0; /* signalize we have no packet prepared */
-}
-#define PACKET_INIT(packet) \
- struct pkt_hdr packet; \
- PACKET_INIT_STRUCT(&packet)
yeah, I have messed up the initialization. What I wanted to have one initialization for compiler time and one for run time.
struct pkt_hdr pkt = PKT_INIT;where PKT_INIT is macro
#define PKT_INIT { .pkt_bufsize = MAX_PACKET_SIZE, .pkt_payload = NULL ... }and one you have written for run time. that would be ideal.
hmm?
Why should we have 2 init routines? In the case packet mmap() case even the compile time initialization will be moved to run time init. I think my patch does this the right way.
yeah, I have realize that after I have finished the review.
1) rename pkt_cleanup() to packet_destroy() 2) add struct pkt_hdr parameter (in preparation for proper init/destroy) 3) add packet_destroy() to cmd_capture(), hostmon() and packet_size_breakdown() (again in preparation for proper init/destroy)
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/capture-pkt.c | 2 ++ src/detstats.c | 2 +- src/hostmon.c | 2 ++ src/ifstats.c | 2 +- src/itrafmon.c | 2 +- src/packet.c | 2 +- src/packet.h | 2 +- src/pktsize.c | 2 ++ src/serv.c | 2 +- 9 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/src/capture-pkt.c b/src/capture-pkt.c index 25e9f2b..93f0b58 100644 --- a/src/capture-pkt.c +++ b/src/capture-pkt.c @@ -69,6 +69,8 @@ int cmd_capture(int argc, char **argv) } printf("\n");
+ packet_destroy(&pkt); + close(fd);
if (fp) diff --git a/src/detstats.c b/src/detstats.c index 0f96e9b..7377161 100644 --- a/src/detstats.c +++ b/src/detstats.c @@ -616,7 +616,7 @@ err: del_panel(statpanel); delwin(statwin); strcpy(current_logfile, ""); - pkt_cleanup(); + packet_destroy(&pkt); update_panels(); doupdate(); } diff --git a/src/hostmon.c b/src/hostmon.c index 51267ef..78315fe 100644 --- a/src/hostmon.c +++ b/src/hostmon.c @@ -1017,6 +1017,8 @@ err: doupdate(); destroyethtab(&table);
+ packet_destroy(&pkt); + free_eth_desc(elist); free_eth_desc(flist);
diff --git a/src/ifstats.c b/src/ifstats.c index b7eeb7a..cc1e2ad 100644 --- a/src/ifstats.c +++ b/src/ifstats.c @@ -636,7 +636,7 @@ err: fclose(logfile); } destroyiflist(table.head); - pkt_cleanup(); + packet_destroy(&pkt); strcpy(current_logfile, ""); }
diff --git a/src/itrafmon.c b/src/itrafmon.c index 8bf6c3a..2400e60 100644 --- a/src/itrafmon.c +++ b/src/itrafmon.c @@ -1178,7 +1178,7 @@ err: delwin(statwin); destroytcptable(&table); destroyothptable(&othptbl); - pkt_cleanup(); + packet_destroy(&pkt);
if (logging) { signal(SIGUSR1, SIG_DFL); diff --git a/src/packet.c b/src/packet.c index afa767f..ad6fb61 100644 --- a/src/packet.c +++ b/src/packet.c @@ -342,7 +342,7 @@ int packet_init(struct pkt_hdr *pkt) return 0; /* all O.K. */ }
-void pkt_cleanup(void) +void packet_destroy(struct pkt_hdr *pkt) { destroyfraglist(); } diff --git a/src/packet.h b/src/packet.h index e8811e6..1cdbe76 100644 --- a/src/packet.h +++ b/src/packet.h @@ -66,6 +66,6 @@ int packet_process(struct pkt_hdr *pkt, unsigned int *total_br, in_port_t *sport, in_port_t *dport, int match_opposite, int v6inv4asv6); int packet_init(struct pkt_hdr *pkt); -void pkt_cleanup(void); +void packet_destroy(struct pkt_hdr *pkt);
#endif /* IPTRAF_NG_PACKET_H */ diff --git a/src/pktsize.c b/src/pktsize.c index 8206919..413d768 100644 --- a/src/pktsize.c +++ b/src/pktsize.c @@ -318,6 +318,8 @@ void packet_size_breakdown(char *ifname, time_t facilitytime) update_size_distrib(pkt.pkt_len, brackets, interval); } while (!exitloop);
+ packet_destroy(&pkt); + err_close: close(fd); err: diff --git a/src/serv.c b/src/serv.c index eecb34a..9e57220 100644 --- a/src/serv.c +++ b/src/serv.c @@ -1069,7 +1069,7 @@ err: doupdate(); destroyportlist(&list); destroyporttab(ports); - pkt_cleanup(); + packet_destroy(&pkt); strcpy(current_logfile, ""); }
There's no need to initialize struct iovec before every recvmsg(); move it to struct pkt_hdr and initialize only once in packet_init().
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/packet.c | 9 ++++----- src/packet.h | 3 +++ 2 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/packet.c b/src/packet.c index ad6fb61..e6fcae5 100644 --- a/src/packet.c +++ b/src/packet.c @@ -148,15 +148,11 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win)
if ((ss > 0) && (pfds[0].revents & POLLIN) != 0) { struct sockaddr_ll from; - struct iovec iov; struct msghdr msg;
- iov.iov_len = pkt->pkt_bufsize; - iov.iov_base = pkt->pkt_buf; - msg.msg_name = &from; msg.msg_namelen = sizeof(from); - msg.msg_iov = &iov; + msg.msg_iov = &pkt->iov; msg.msg_iovlen = 1; msg.msg_control = NULL; msg.msg_controllen = 0; @@ -339,6 +335,9 @@ int packet_init(struct pkt_hdr *pkt) pkt->ip6_hdr = NULL; pkt->pkt_len = 0; /* signalize we have no packet prepared */
+ pkt->iov.iov_len = pkt->pkt_bufsize; + pkt->iov.iov_base = pkt->pkt_buf; + return 0; /* all O.K. */ }
diff --git a/src/packet.h b/src/packet.h index 1cdbe76..d47d658 100644 --- a/src/packet.h +++ b/src/packet.h @@ -31,6 +31,9 @@ struct pkt_hdr { unsigned char pkt_pkttype; /* Packet type: PACKET_OUTGOING, PACKET_BROADCAST, ... */ unsigned char pkt_halen; /* Length of address */ unsigned char pkt_addr[8]; /* Physical layer address */ + + struct iovec iov; + struct ethhdr *ethhdr; struct fddihdr *fddihdr; struct iphdr *iphdr;
Move this struct to struct pkt_hdr; don't blow up struct pkt_hdr and make it allocated from heap.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/packet.c | 18 +++++++++++------- src/packet.h | 1 + 2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/packet.c b/src/packet.c index e6fcae5..5ab128a 100644 --- a/src/packet.c +++ b/src/packet.c @@ -147,11 +147,10 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win) pkt->pkt_len = 0;
if ((ss > 0) && (pfds[0].revents & POLLIN) != 0) { - struct sockaddr_ll from; struct msghdr msg;
- msg.msg_name = &from; - msg.msg_namelen = sizeof(from); + msg.msg_name = pkt->from; + msg.msg_namelen = sizeof(*pkt->from); msg.msg_iov = &pkt->iov; msg.msg_iovlen = 1; msg.msg_control = NULL; @@ -165,10 +164,10 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win) if (pkt->pkt_caplen > pkt->pkt_bufsize) pkt->pkt_caplen = pkt->pkt_bufsize; pkt->pkt_payload = NULL; - pkt->pkt_protocol = ntohs(from.sll_protocol); - pkt->pkt_ifindex = from.sll_ifindex; - pkt->pkt_hatype = from.sll_hatype; - pkt->pkt_pkttype = from.sll_pkttype; + pkt->pkt_protocol = ntohs(pkt->from->sll_protocol); + pkt->pkt_ifindex = pkt->from->sll_ifindex; + pkt->pkt_hatype = pkt->from->sll_hatype; + pkt->pkt_pkttype = pkt->from->sll_pkttype; } else ss = len; } @@ -338,10 +337,15 @@ int packet_init(struct pkt_hdr *pkt) pkt->iov.iov_len = pkt->pkt_bufsize; pkt->iov.iov_base = pkt->pkt_buf;
+ pkt->from = xmallocz(sizeof(*pkt->from)); + return 0; /* all O.K. */ }
void packet_destroy(struct pkt_hdr *pkt) { + if (pkt->from) + free(pkt->from); + pkt->from = NULL; destroyfraglist(); } diff --git a/src/packet.h b/src/packet.h index d47d658..68d0633 100644 --- a/src/packet.h +++ b/src/packet.h @@ -33,6 +33,7 @@ struct pkt_hdr { unsigned char pkt_addr[8]; /* Physical layer address */
struct iovec iov; + struct sockaddr_ll *from;
struct ethhdr *ethhdr; struct fddihdr *fddihdr;
Vitezslav Samel vitezslav@samel.cz writes:
Move this struct to struct pkt_hdr; don't blow up struct pkt_hdr and make it allocated from heap.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/packet.c | 18 +++++++++++------- src/packet.h | 1 + 2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/packet.c b/src/packet.c index e6fcae5..5ab128a 100644 --- a/src/packet.c +++ b/src/packet.c @@ -147,11 +147,10 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win) pkt->pkt_len = 0;
if ((ss > 0) && (pfds[0].revents & POLLIN) != 0) {
struct sockaddr_ll from;struct msghdr msg;
msg.msg_name = &from;msg.msg_namelen = sizeof(from);
msg.msg_name = pkt->from;msg.msg_namelen = sizeof(*pkt->from);@@ -165,10 +164,10 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win) if (pkt->pkt_caplen > pkt->pkt_bufsize) pkt->pkt_caplen = pkt->pkt_bufsize; pkt->pkt_payload = NULL;
pkt->pkt_protocol = ntohs(from.sll_protocol);pkt->pkt_ifindex = from.sll_ifindex;pkt->pkt_hatype = from.sll_hatype;pkt->pkt_pkttype = from.sll_pkttype;
pkt->pkt_protocol = ntohs(pkt->from->sll_protocol);pkt->pkt_ifindex = pkt->from->sll_ifindex;pkt->pkt_hatype = pkt->from->sll_hatype;pkt->pkt_pkttype = pkt->from->sll_pkttype;@@ -338,10 +337,15 @@ int packet_init(struct pkt_hdr *pkt) pkt->iov.iov_len = pkt->pkt_bufsize; pkt->iov.iov_base = pkt->pkt_buf;
- pkt->from = xmallocz(sizeof(*pkt->from));
- return 0; /* all O.K. */
}
void packet_destroy(struct pkt_hdr *pkt) {
- if (pkt->from)
free(pkt->from);
you don't have to check pkt->from, because xmallocz always returns pointer, otherwise it dies.
On Wed, May 07, 2014 at 06:52:58PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
Move this struct to struct pkt_hdr; don't blow up struct pkt_hdr and make it allocated from heap.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/packet.c | 18 +++++++++++------- src/packet.h | 1 + 2 files changed, 12 insertions(+), 7 deletions(-)
diff --git a/src/packet.c b/src/packet.c index e6fcae5..5ab128a 100644 --- a/src/packet.c +++ b/src/packet.c @@ -147,11 +147,10 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win) pkt->pkt_len = 0;
if ((ss > 0) && (pfds[0].revents & POLLIN) != 0) {
struct sockaddr_ll from;struct msghdr msg;
msg.msg_name = &from;msg.msg_namelen = sizeof(from);
msg.msg_name = pkt->from;msg.msg_namelen = sizeof(*pkt->from);@@ -165,10 +164,10 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win) if (pkt->pkt_caplen > pkt->pkt_bufsize) pkt->pkt_caplen = pkt->pkt_bufsize; pkt->pkt_payload = NULL;
pkt->pkt_protocol = ntohs(from.sll_protocol);pkt->pkt_ifindex = from.sll_ifindex;pkt->pkt_hatype = from.sll_hatype;pkt->pkt_pkttype = from.sll_pkttype;
pkt->pkt_protocol = ntohs(pkt->from->sll_protocol);pkt->pkt_ifindex = pkt->from->sll_ifindex;pkt->pkt_hatype = pkt->from->sll_hatype;pkt->pkt_pkttype = pkt->from->sll_pkttype;@@ -338,10 +337,15 @@ int packet_init(struct pkt_hdr *pkt) pkt->iov.iov_len = pkt->pkt_bufsize; pkt->iov.iov_base = pkt->pkt_buf;
- pkt->from = xmallocz(sizeof(*pkt->from));
- return 0; /* all O.K. */
}
void packet_destroy(struct pkt_hdr *pkt) {
- if (pkt->from)
free(pkt->from);you don't have to check pkt->from, because xmallocz always returns pointer, otherwise it dies.
OK, will redo.
Vita
There's no need to initialize every struct msghdr member before every recvmsg(); move this struct to struct pkt_hdr, make it allocated from heap and move unnecessary initializations to packet_init().
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/packet.c | 26 +++++++++++++++++--------- src/packet.h | 1 + 2 files changed, 18 insertions(+), 9 deletions(-)
diff --git a/src/packet.c b/src/packet.c index 5ab128a..21c711d 100644 --- a/src/packet.c +++ b/src/packet.c @@ -147,17 +147,13 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win) pkt->pkt_len = 0;
if ((ss > 0) && (pfds[0].revents & POLLIN) != 0) { - struct msghdr msg;
- msg.msg_name = pkt->from; - msg.msg_namelen = sizeof(*pkt->from); - msg.msg_iov = &pkt->iov; - msg.msg_iovlen = 1; - msg.msg_control = NULL; - msg.msg_controllen = 0; - msg.msg_flags = 0; + /* these are set upon return from recvmsg() so clean */ + /* them beforehand */ + pkt->msg->msg_controllen = 0; + pkt->msg->msg_flags = 0;
- ssize_t len = recvmsg(fd, &msg, MSG_TRUNC | MSG_DONTWAIT); + ssize_t len = recvmsg(fd, pkt->msg, MSG_TRUNC | MSG_DONTWAIT); if (len > 0) { pkt->pkt_len = len; pkt->pkt_caplen = len; @@ -338,14 +334,26 @@ int packet_init(struct pkt_hdr *pkt) pkt->iov.iov_base = pkt->pkt_buf;
pkt->from = xmallocz(sizeof(*pkt->from)); + pkt->msg = xmallocz(sizeof(*pkt->msg)); + + pkt->msg->msg_name = pkt->from; + pkt->msg->msg_namelen = sizeof(*pkt->from); + pkt->msg->msg_iov = &pkt->iov; + pkt->msg->msg_iovlen = 1; + pkt->msg->msg_control = NULL;
return 0; /* all O.K. */ }
void packet_destroy(struct pkt_hdr *pkt) { + if (pkt->msg) + free(pkt->msg); + pkt->msg = NULL; + if (pkt->from) free(pkt->from); pkt->from = NULL; + destroyfraglist(); } diff --git a/src/packet.h b/src/packet.h index 68d0633..ddb570b 100644 --- a/src/packet.h +++ b/src/packet.h @@ -34,6 +34,7 @@ struct pkt_hdr {
struct iovec iov; struct sockaddr_ll *from; + struct msghdr *msg;
struct ethhdr *ethhdr; struct fddihdr *fddihdr;
Vitezslav Samel vitezslav@samel.cz writes:
There's no need to initialize every struct msghdr member before every recvmsg(); move this struct to struct pkt_hdr, make it allocated from heap and move unnecessary initializations to packet_init().
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/packet.c | 26 +++++++++++++++++--------- src/packet.h | 1 + 2 files changed, 18 insertions(+), 9 deletions(-)
pkt->from = xmallocz(sizeof(*pkt->from));
pkt->msg = xmallocz(sizeof(*pkt->msg));
pkt->msg->msg_name = pkt->from;
pkt->msg->msg_namelen = sizeof(*pkt->from);
pkt->msg->msg_iov = &pkt->iov;
pkt->msg->msg_iovlen = 1;
pkt->msg->msg_control = NULL;
return 0; /* all O.K. */
}
void packet_destroy(struct pkt_hdr *pkt) {
- if (pkt->msg)
free(pkt->msg);
ditto with check and xmallocz
Since we have struct sockaddr_ll in struct pkt_hdr now, remove pkt_ifindex, pkt_hatype and pkt_pkttype members from struct pkt_hdr and modify all their uses.
Leave pkt_protocol untouched, because it caches the protocol number in host order (no need for another htons()/ntohs() in other code).
Also remove pkt_halen and pkt_addr members, they are unused.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/detstats.c | 4 ++-- src/hostmon.c | 12 ++++++------ src/ifstats.c | 2 +- src/itrafmon.c | 6 +++--- src/othptab.c | 6 +++--- src/packet.c | 5 +---- src/packet.h | 5 ----- 7 files changed, 16 insertions(+), 24 deletions(-)
diff --git a/src/detstats.c b/src/detstats.c index 7377161..6d8ca07 100644 --- a/src/detstats.c +++ b/src/detstats.c @@ -531,9 +531,9 @@ void detstats(char *iface, time_t facilitytime) continue; }
- outgoing = (pkt.pkt_pkttype == PACKET_OUTGOING); + outgoing = (pkt.from->sll_pkttype == PACKET_OUTGOING); update_proto_counter(&ifcounts.total, outgoing, pkt.pkt_len); - if (pkt.pkt_pkttype == PACKET_BROADCAST) { + if (pkt.from->sll_pkttype == PACKET_BROADCAST) { update_proto_counter(&ifcounts.bcast, outgoing, pkt.pkt_len); update_pkt_counter(&span_bcast, pkt.pkt_len); } diff --git a/src/hostmon.c b/src/hostmon.c index 78315fe..71a1836 100644 --- a/src/hostmon.c +++ b/src/hostmon.c @@ -922,7 +922,7 @@ void hostmon(time_t facilitytime, char *ifptr) /* we're capturing on "All interfaces", */ /* so get the name of the interface */ /* of this packet */ - int r = dev_get_ifname(pkt.pkt_ifindex, ifnamebuf); + int r = dev_get_ifname(pkt.from->sll_ifindex, ifnamebuf); if (r != 0) { write_error("Unable to get interface name"); break; /* can't get interface name, get out! */ @@ -931,7 +931,7 @@ void hostmon(time_t facilitytime, char *ifptr) }
/* get HW addresses */ - switch (pkt.pkt_hatype) { + switch (pkt.from->sll_hatype) { case ARPHRD_ETHER: { memcpy(scratch_saddr, pkt.ethhdr->h_source, ETH_ALEN); memcpy(scratch_daddr, pkt.ethhdr->h_dest, ETH_ALEN); @@ -958,11 +958,11 @@ void hostmon(time_t facilitytime, char *ifptr) }
/* Check source address entry */ - entry = in_ethtable(&table, pkt.pkt_hatype, + entry = in_ethtable(&table, pkt.from->sll_hatype, scratch_saddr);
if (!entry) - entry = addethentry(&table, pkt.pkt_hatype, + entry = addethentry(&table, pkt.from->sll_hatype, ifname, scratch_saddr, list);
if (entry != NULL) { @@ -975,10 +975,10 @@ void hostmon(time_t facilitytime, char *ifptr) }
/* Check destination address entry */ - entry = in_ethtable(&table, pkt.pkt_hatype, + entry = in_ethtable(&table, pkt.from->sll_hatype, scratch_daddr); if (!entry) - entry = addethentry(&table, pkt.pkt_hatype, + entry = addethentry(&table, pkt.from->sll_hatype, ifname, scratch_daddr, list);
if (entry != NULL) { diff --git a/src/ifstats.c b/src/ifstats.c index cc1e2ad..097fde1 100644 --- a/src/ifstats.c +++ b/src/ifstats.c @@ -591,7 +591,7 @@ void ifstats(time_t facilitytime) continue; }
- ptmp = positionptr(table.head, pkt.pkt_ifindex); + ptmp = positionptr(table.head, pkt.from->sll_ifindex); if (!ptmp) continue;
diff --git a/src/itrafmon.c b/src/itrafmon.c index 2400e60..37c3229 100644 --- a/src/itrafmon.c +++ b/src/itrafmon.c @@ -964,7 +964,7 @@ void ipmon(time_t facilitytime, char *ifptr) /* we're capturing on "All interfaces", */ /* so get the name of the interface */ /* of this packet */ - int r = dev_get_ifname(pkt.pkt_ifindex, ifnamebuf); + int r = dev_get_ifname(pkt.from->sll_ifindex, ifnamebuf); if (r != 0) { write_error("Unable to get interface name"); break; /* error getting interface name, get out! */ @@ -1052,13 +1052,13 @@ void ipmon(time_t facilitytime, char *ifptr)
if (pkt.iphdr) updateentry(&table, tcpentry, transpacket, - pkt.pkt_buf, pkt.pkt_hatype, + pkt.pkt_buf, pkt.from->sll_hatype, pkt.pkt_len, br, pkt.iphdr->frag_off, logging, &revlook, rvnfd, logfile); else updateentry(&table, tcpentry, transpacket, - pkt.pkt_buf, pkt.pkt_hatype, + pkt.pkt_buf, pkt.from->sll_hatype, pkt.pkt_len, pkt.pkt_len, 0, logging, &revlook, rvnfd, logfile); diff --git a/src/othptab.c b/src/othptab.c index e23f39e..d4b21bd 100644 --- a/src/othptab.c +++ b/src/othptab.c @@ -182,10 +182,10 @@ struct othptabent *add_othp_entry(struct othptable *table, struct pkt_hdr *pkt, new_entry->fragment = fragment;
if (options.mac || !is_ip) { - if (pkt->pkt_hatype == ARPHRD_ETHER) { + if (pkt->from->sll_hatype == ARPHRD_ETHER) { convmacaddr((char *) pkt->ethhdr->h_source, new_entry->smacaddr); convmacaddr((char *) pkt->ethhdr->h_dest, new_entry->dmacaddr); - } else if (pkt->pkt_hatype == ARPHRD_FDDI) { + } else if (pkt->from->sll_hatype == ARPHRD_FDDI) { convmacaddr((char *) pkt->fddihdr->saddr, new_entry->smacaddr); convmacaddr((char *) pkt->fddihdr->daddr, new_entry->dmacaddr); } @@ -231,7 +231,7 @@ struct othptabent *add_othp_entry(struct othptable *table, struct pkt_hdr *pkt, } } } else { - new_entry->linkproto = pkt->pkt_hatype; + new_entry->linkproto = pkt->from->sll_hatype;
if (protocol == ETH_P_ARP) { new_entry->un.arp.opcode = diff --git a/src/packet.c b/src/packet.c index 21c711d..3e6a6d2 100644 --- a/src/packet.c +++ b/src/packet.c @@ -59,7 +59,7 @@ static int packet_adjust(struct pkt_hdr *pkt) { int retval = 0;
- switch (pkt->pkt_hatype) { + switch (pkt->from->sll_hatype) { case ARPHRD_ETHER: case ARPHRD_LOOPBACK: pkt_cast_hdrp_l2(ethhdr, pkt); @@ -161,9 +161,6 @@ int packet_get(int fd, struct pkt_hdr *pkt, int *ch, WINDOW *win) pkt->pkt_caplen = pkt->pkt_bufsize; pkt->pkt_payload = NULL; pkt->pkt_protocol = ntohs(pkt->from->sll_protocol); - pkt->pkt_ifindex = pkt->from->sll_ifindex; - pkt->pkt_hatype = pkt->from->sll_hatype; - pkt->pkt_pkttype = pkt->from->sll_pkttype; } else ss = len; } diff --git a/src/packet.h b/src/packet.h index ddb570b..05afa68 100644 --- a/src/packet.h +++ b/src/packet.h @@ -25,12 +25,7 @@ struct pkt_hdr { char *pkt_payload; size_t pkt_caplen; /* bytes captured */ size_t pkt_len; /* bytes on-the-wire */ - int pkt_ifindex; /* Interface number */ unsigned short pkt_protocol; /* Physical layer protocol: ETH_P_* */ - unsigned short pkt_hatype; /* Header type: ARPHRD_* */ - unsigned char pkt_pkttype; /* Packet type: PACKET_OUTGOING, PACKET_BROADCAST, ... */ - unsigned char pkt_halen; /* Length of address */ - unsigned char pkt_addr[8]; /* Physical layer address */
struct iovec iov; struct sockaddr_ll *from;
Vitezslav Samel vitezslav@samel.cz writes:
Since we have struct sockaddr_ll in struct pkt_hdr now, remove pkt_ifindex, pkt_hatype and pkt_pkttype members from struct pkt_hdr and modify all their uses.
Leave pkt_protocol untouched, because it caches the protocol number in host order (no need for another htons()/ntohs() in other code).
Also remove pkt_halen and pkt_addr members, they are unused.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/detstats.c | 4 ++-- src/hostmon.c | 12 ++++++------ src/ifstats.c | 2 +- src/itrafmon.c | 6 +++--- src/othptab.c | 6 +++--- src/packet.c | 5 +---- src/packet.h | 5 ----- 7 files changed, 16 insertions(+), 24 deletions(-) diff --git a/src/itrafmon.c b/src/itrafmon.c index 2400e60..37c3229 100644 --- a/src/itrafmon.c +++ b/src/itrafmon.c @@ -964,7 +964,7 @@ void ipmon(time_t facilitytime, char *ifptr) /* we're capturing on "All interfaces", */ /* so get the name of the interface */ /* of this packet */
int r = dev_get_ifname(pkt.pkt_ifindex, ifnamebuf);
int r = dev_get_ifname(pkt.from->sll_ifindex, ifnamebuf); if (r != 0) { write_error("Unable to get interface name"); break; /* error getting interface name, get out! */@@ -1052,13 +1052,13 @@ void ipmon(time_t facilitytime, char *ifptr)
if (pkt.iphdr) updateentry(&table, tcpentry, transpacket,
pkt.pkt_buf, pkt.pkt_hatype,
pkt.pkt_buf, pkt.from->sll_hatype, pkt.pkt_len, br, pkt.iphdr->frag_off, logging, &revlook, rvnfd, logfile); else updateentry(&table, tcpentry, transpacket,
pkt.pkt_buf, pkt.pkt_hatype,
pkt.pkt_buf, pkt.from->sll_hatype, pkt.pkt_len, pkt.pkt_len, 0, logging, &revlook, rvnfd, logfile);
updateentry() new has quite a lot of members of pkt. Marking that to my TODO list to just pass pkt pointer instead of opening struct.
On Wed, May 07, 2014 at 06:59:21PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
Since we have struct sockaddr_ll in struct pkt_hdr now, remove pkt_ifindex, pkt_hatype and pkt_pkttype members from struct pkt_hdr and modify all their uses.
Leave pkt_protocol untouched, because it caches the protocol number in host order (no need for another htons()/ntohs() in other code).
Also remove pkt_halen and pkt_addr members, they are unused.
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/detstats.c | 4 ++-- src/hostmon.c | 12 ++++++------ src/ifstats.c | 2 +- src/itrafmon.c | 6 +++--- src/othptab.c | 6 +++--- src/packet.c | 5 +---- src/packet.h | 5 ----- 7 files changed, 16 insertions(+), 24 deletions(-) diff --git a/src/itrafmon.c b/src/itrafmon.c index 2400e60..37c3229 100644 --- a/src/itrafmon.c +++ b/src/itrafmon.c @@ -964,7 +964,7 @@ void ipmon(time_t facilitytime, char *ifptr) /* we're capturing on "All interfaces", */ /* so get the name of the interface */ /* of this packet */
int r = dev_get_ifname(pkt.pkt_ifindex, ifnamebuf);
int r = dev_get_ifname(pkt.from->sll_ifindex, ifnamebuf); if (r != 0) { write_error("Unable to get interface name"); break; /* error getting interface name, get out! */@@ -1052,13 +1052,13 @@ void ipmon(time_t facilitytime, char *ifptr)
if (pkt.iphdr) updateentry(&table, tcpentry, transpacket,
pkt.pkt_buf, pkt.pkt_hatype,
pkt.pkt_buf, pkt.from->sll_hatype, pkt.pkt_len, br, pkt.iphdr->frag_off, logging, &revlook, rvnfd, logfile); else updateentry(&table, tcpentry, transpacket,
pkt.pkt_buf, pkt.pkt_hatype,
pkt.pkt_buf, pkt.from->sll_hatype, pkt.pkt_len, pkt.pkt_len, 0, logging, &revlook, rvnfd, logfile);updateentry() new has quite a lot of members of pkt. Marking that to my TODO list to just pass pkt pointer instead of opening struct.
I've done this already, but waiting to accept earlier patches.
Vita
Signed-off-by: Vitezslav Samel vitezslav@samel.cz --- src/packet.c | 5 +++++ src/packet.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/packet.c b/src/packet.c index 3e6a6d2..f3cc522 100644 --- a/src/packet.c +++ b/src/packet.c @@ -319,6 +319,7 @@ again:
int packet_init(struct pkt_hdr *pkt) { + pkt->pkt_buf = xmallocz(MAX_PACKET_SIZE); pkt->pkt_bufsize = MAX_PACKET_SIZE; pkt->pkt_payload = NULL; pkt->ethhdr = NULL; @@ -352,5 +353,9 @@ void packet_destroy(struct pkt_hdr *pkt) free(pkt->from); pkt->from = NULL;
+ if (pkt->pkt_buf) + free(pkt->pkt_buf); + pkt->pkt_buf = NULL; + destroyfraglist(); } diff --git a/src/packet.h b/src/packet.h index 05afa68..8027760 100644 --- a/src/packet.h +++ b/src/packet.h @@ -21,6 +21,7 @@ packet.h - external declarations for packet.c #define MORE_FRAGMENTS 4
struct pkt_hdr { + char *pkt_buf; size_t pkt_bufsize; char *pkt_payload; size_t pkt_caplen; /* bytes captured */ @@ -35,7 +36,6 @@ struct pkt_hdr { struct fddihdr *fddihdr; struct iphdr *iphdr; struct ip6_hdr *ip6_hdr; - char pkt_buf[MAX_PACKET_SIZE]; };
static inline __u8 pkt_iph_len(const struct pkt_hdr *pkt)
Vitezslav Samel vitezslav@samel.cz writes:
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/packet.c | 5 +++++ src/packet.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/packet.c b/src/packet.c index 3e6a6d2..f3cc522 100644 --- a/src/packet.c +++ b/src/packet.c @@ -319,6 +319,7 @@ again:
int packet_init(struct pkt_hdr *pkt) {
- pkt->pkt_buf = xmallocz(MAX_PACKET_SIZE); pkt->pkt_bufsize = MAX_PACKET_SIZE; pkt->pkt_payload = NULL; pkt->ethhdr = NULL;
@@ -352,5 +353,9 @@ void packet_destroy(struct pkt_hdr *pkt) free(pkt->from); pkt->from = NULL;
- if (pkt->pkt_buf)
free(pkt->pkt_buf);- pkt->pkt_buf = NULL;
- destroyfraglist();
} diff --git a/src/packet.h b/src/packet.h index 05afa68..8027760 100644 --- a/src/packet.h +++ b/src/packet.h @@ -21,6 +21,7 @@ packet.h - external declarations for packet.c #define MORE_FRAGMENTS 4
struct pkt_hdr {
- char *pkt_buf; size_t pkt_bufsize; char *pkt_payload; size_t pkt_caplen; /* bytes captured */
@@ -35,7 +36,6 @@ struct pkt_hdr { struct fddihdr *fddihdr; struct iphdr *iphdr; struct ip6_hdr *ip6_hdr;
- char pkt_buf[MAX_PACKET_SIZE];
};
static inline __u8 pkt_iph_len(const struct pkt_hdr *pkt)
I don't see a reason for that. Is there any?
On Wed, May 07, 2014 at 07:00:39PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/packet.c | 5 +++++ src/packet.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/packet.c b/src/packet.c index 3e6a6d2..f3cc522 100644 --- a/src/packet.c +++ b/src/packet.c @@ -319,6 +319,7 @@ again:
int packet_init(struct pkt_hdr *pkt) {
- pkt->pkt_buf = xmallocz(MAX_PACKET_SIZE); pkt->pkt_bufsize = MAX_PACKET_SIZE; pkt->pkt_payload = NULL; pkt->ethhdr = NULL;
@@ -352,5 +353,9 @@ void packet_destroy(struct pkt_hdr *pkt) free(pkt->from); pkt->from = NULL;
- if (pkt->pkt_buf)
free(pkt->pkt_buf);- pkt->pkt_buf = NULL;
- destroyfraglist();
} diff --git a/src/packet.h b/src/packet.h index 05afa68..8027760 100644 --- a/src/packet.h +++ b/src/packet.h @@ -21,6 +21,7 @@ packet.h - external declarations for packet.c #define MORE_FRAGMENTS 4
struct pkt_hdr {
- char *pkt_buf; size_t pkt_bufsize; char *pkt_payload; size_t pkt_caplen; /* bytes captured */
@@ -35,7 +36,6 @@ struct pkt_hdr { struct fddihdr *fddihdr; struct iphdr *iphdr; struct ip6_hdr *ip6_hdr;
- char pkt_buf[MAX_PACKET_SIZE];
};
static inline __u8 pkt_iph_len(const struct pkt_hdr *pkt)
I don't see a reason for that. Is there any?
Preparation for future ;-)) In the packet mmap() case the pkt_buf is pointer to some mmaped memory.
Vita
Vitezslav Samel vitezslav@samel.cz writes:
On Wed, May 07, 2014 at 07:00:39PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
Signed-off-by: Vitezslav Samel vitezslav@samel.cz
src/packet.c | 5 +++++ src/packet.h | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/src/packet.c b/src/packet.c index 3e6a6d2..f3cc522 100644 --- a/src/packet.c +++ b/src/packet.c @@ -319,6 +319,7 @@ again:
int packet_init(struct pkt_hdr *pkt) {
- pkt->pkt_buf = xmallocz(MAX_PACKET_SIZE); pkt->pkt_bufsize = MAX_PACKET_SIZE; pkt->pkt_payload = NULL; pkt->ethhdr = NULL;
@@ -352,5 +353,9 @@ void packet_destroy(struct pkt_hdr *pkt) free(pkt->from); pkt->from = NULL;
- if (pkt->pkt_buf)
free(pkt->pkt_buf);- pkt->pkt_buf = NULL;
- destroyfraglist();
} diff --git a/src/packet.h b/src/packet.h index 05afa68..8027760 100644 --- a/src/packet.h +++ b/src/packet.h @@ -21,6 +21,7 @@ packet.h - external declarations for packet.c #define MORE_FRAGMENTS 4
struct pkt_hdr {
- char *pkt_buf; size_t pkt_bufsize; char *pkt_payload; size_t pkt_caplen; /* bytes captured */
@@ -35,7 +36,6 @@ struct pkt_hdr { struct fddihdr *fddihdr; struct iphdr *iphdr; struct ip6_hdr *ip6_hdr;
- char pkt_buf[MAX_PACKET_SIZE];
};
static inline __u8 pkt_iph_len(const struct pkt_hdr *pkt)
I don't see a reason for that. Is there any?
Preparation for future ;-)) In the packet mmap() case the pkt_buf is pointer to some mmaped memory.
ah, hidden secret ...
ok, redo those two patches and I will applied them along with show drop pkts.
Vitezslav Samel vitezslav@samel.cz writes:
Main idea for this work is to minimize the work needed to be done in packet_get(), which is the hottest path in the program.
Cheers, Vita
Vitezslav Samel (7): refactor struct pkt_hdr initialization refactor pkt_cleanup() packet_get(): optimize - move struct iovec packet_get(): optimize - move struct sockaddr_ll packet_get(): optimize - move struct msghdr packet_get(): optimize - remove cache variables pkt_hdr: make pkt_buf allocated from heap
src/capture-pkt.c | 12 ++++++---- src/detstats.c | 10 ++++---- src/hostmon.c | 18 ++++++++------ src/ifstats.c | 8 ++++--- src/itrafmon.c | 12 ++++++---- src/othptab.c | 6 ++--- src/packet.c | 70 ++++++++++++++++++++++++++++++++++++++----------------- src/packet.h | 30 +++++++----------------- src/pktsize.c | 6 ++++- src/serv.c | 6 +++-- 10 files changed, 105 insertions(+), 73 deletions(-)
Hello,
do you have any numbers, which can proof your optimization?
On Sun, May 04, 2014 at 08:10:32PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
Main idea for this work is to minimize the work needed to be done in packet_get(), which is the hottest path in the program.
Cheers, Vita
Vitezslav Samel (7): refactor struct pkt_hdr initialization refactor pkt_cleanup() packet_get(): optimize - move struct iovec packet_get(): optimize - move struct sockaddr_ll packet_get(): optimize - move struct msghdr packet_get(): optimize - remove cache variables pkt_hdr: make pkt_buf allocated from heap
src/capture-pkt.c | 12 ++++++---- src/detstats.c | 10 ++++---- src/hostmon.c | 18 ++++++++------ src/ifstats.c | 8 ++++--- src/itrafmon.c | 12 ++++++---- src/othptab.c | 6 ++--- src/packet.c | 70 ++++++++++++++++++++++++++++++++++++++----------------- src/packet.h | 30 +++++++----------------- src/pktsize.c | 6 ++++- src/serv.c | 6 +++-- 10 files changed, 105 insertions(+), 73 deletions(-)
Hello,
do you have any numbers, which can proof your optimization?
No. But these are first steps to minimize dropped packets count (packets which are received by interface but not processed by recvmsg()) and to have recvmsg() replaced by packet mmap() (PACKET_RX_RING) interface and to have multithreading support.
Replacing recvmsg() by mmap() with multithreading in ifstats() decreases dropped packets counts from several thousands to tens packets per second on my workloads.
I have a patch which shows dropped packets count; do you think this info should be displayed on any screen? Or is this only debugging info which user should not bother about?
Cheers, Vita
Vitezslav Samel vitezslav@samel.cz writes:
On Sun, May 04, 2014 at 08:10:32PM +1200, Nikola Pajkovsky wrote:
Vitezslav Samel vitezslav@samel.cz writes:
Main idea for this work is to minimize the work needed to be done in packet_get(), which is the hottest path in the program.
Cheers, Vita
Vitezslav Samel (7): refactor struct pkt_hdr initialization refactor pkt_cleanup() packet_get(): optimize - move struct iovec packet_get(): optimize - move struct sockaddr_ll packet_get(): optimize - move struct msghdr packet_get(): optimize - remove cache variables pkt_hdr: make pkt_buf allocated from heap
src/capture-pkt.c | 12 ++++++---- src/detstats.c | 10 ++++---- src/hostmon.c | 18 ++++++++------ src/ifstats.c | 8 ++++--- src/itrafmon.c | 12 ++++++---- src/othptab.c | 6 ++--- src/packet.c | 70 ++++++++++++++++++++++++++++++++++++++----------------- src/packet.h | 30 +++++++----------------- src/pktsize.c | 6 ++++- src/serv.c | 6 +++-- 10 files changed, 105 insertions(+), 73 deletions(-)
Hello,
do you have any numbers, which can proof your optimization?
No. But these are first steps to minimize dropped packets count (packets which are received by interface but not processed by recvmsg()) and to have recvmsg() replaced by packet mmap() (PACKET_RX_RING) interface and to have multithreading support.
Replacing recvmsg() by mmap() with multithreading in ifstats() decreases dropped packets counts from several thousands to tens packets per second on my workloads.
That sounds promising. Looking forward to see the patch.
I have a patch which shows dropped packets count; do you think this info should be displayed on any screen? Or is this only debugging info which user should not bother about?
This is nice information for everybody, and if you can squeeze it somewhere, would be fantastic.
Could you please send me just patch, which shows dropped packet count?
iptraf-ng@lists.fedorahosted.org
-
Nikola Pajkovsky -
Vitezslav Samel