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

[Daniel Turull <daniel.turull@gmail.com>]

Eric Dumazet wrote:
> 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 ?

Yes, it allows to receive a feedback of pktgen of the traffic received which comes from pktgen. It is an addition for pktgen. By default is disabled.

> 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 ?
Yes, I used the same type as the number of packets send of the transmission size
struct pktgen_dev
	__u64 sofar;


>> +	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
It is not IPV6 compatible yet. 
> 2) Is it resistant to malicious packets ? (very small ones)
I only tried with pktgen traffic
> 3) No checksum ?
The PKTGEN_MAGIC is checked in order to validate that is a pktgen packet

> 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

Ok, thanks for the advice. I will redo this part in order to make it compatabile with IPV6 and make it more optimal.
> 
>> +	/* 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?

This was added, in order to add the ethernet header in the bytes in order to have the same number in tx an rx, but yes I should use ETH_HLEN

>> +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 ;)

This handler as a new packet type, but the packets are also delivered to the IP stack.  It is possible to control the session with ssh.

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