Re: [PATCH 2/2] pktgen: receive packets and process incoming rate

[Eric Dumazet <eric.dumazet@gmail.com>]

Le mercredi 02 juin 2010 à 13:49 +0200, Daniel Turull a écrit :
> This patch adds receiver part to pktgen taking advantages of SMP systems
> with multiple rx queues:
> - Creation of new proc file  /proc/net/pktgen/pgrx to control and display the receiver.
> - It uses PER-CPU variable to store the results per each CPU.
> - Results displayed per CPU and aggregated.
> - The packet handler is add in the protocols handlers (dev_Add_pack())
> - Available statistics: packets and bytes received, work time and rate
> - Only process pktgen packets
> - It is possible to select the incoming interface 
> - Documentation updated with the new commands to control the receiver part.
> 

Interesting, but does it belongs to pktgen ?

other comments included :)

> Signed-off-by: Daniel Turull <daniel.turull@gmail.com>
> 


>  
> +/*Recevier parameters per cpu*/
> +struct pktgen_rx {
> +	u64 rx_packets;		/*packets arrived*/

unsigned long rx_packets ?

> +	u64 rx_bytes;		/*bytes arrived*/
> +
> +	ktime_t start_time;	/*first time stamp of a packet*/
> +	ktime_t last_time;	/*last packet arrival */
> +};


> +int pktgen_rcv_basic(struct sk_buff *skb, struct net_device *dev,
> +			 struct packet_type *pt, struct net_device *orig_dev)
> +{
> +	/* Check magic*/
> +	struct iphdr *iph = ip_hdr(skb);
> +	struct pktgen_hdr *pgh;
> +	void *vaddr;

Following code is ... well ... interesting... But ...

1) Is it IPV6 compatable ? pktgen can be ipv6 or ipv4
2) Is it resistant to malicious packets ? (very small ones)
3) No checksum ?

I think you should use standard mechanisms... (pskb_may_pull(), ...)
Take a look at __netpoll_rx() for an example.

> +	if (skb_is_nonlinear(skb)) {
> +		vaddr = kmap_skb_frag(&skb_shinfo(skb)->frags[0]);
> +		pgh = (struct pktgen_hdr *)
> +			(vaddr+skb_shinfo(skb)->frags[0].page_offset);
> +	} else {
> +		pgh = (struct pktgen_hdr *)(((char *)(iph)) + 28);
> +	}
> +
> +	if (unlikely(pgh->pgh_magic != PKTGEN_MAGIC_NET))
> +		goto end;
> +
> +	if (unlikely(!__get_cpu_var(pktgen_rx_data).rx_packets))
> +		__get_cpu_var(pktgen_rx_data).start_time = ktime_now();
> +
> +	__get_cpu_var(pktgen_rx_data).last_time = ktime_now();
> +

Its a bit suboptimal to use __get_cpu_var several time. Take a look at
disassembly code :)

Use a pointer instead


> +	/* Update counter of packets*/
> +	__get_cpu_var(pktgen_rx_data).rx_packets++;
> +	__get_cpu_var(pktgen_rx_data).rx_bytes += skb->len+14;

This +14 seems suspect (what about vlan tags ?)
Use ETH_HLEN instead at a very minimum?

> +end:
> +	if (skb_is_nonlinear(skb))
> +		kunmap_skb_frag(vaddr);

Should not recognised packets be allowed to flight in other parts of
kernel stack ? This way, we could use ssh to remotely control this
pktgen session ;)

> +	kfree_skb(skb);
> +	return 0;
> +}
> +