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

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

Hi,

I've made some modification to the patch. I'll send another email with the patch,
but I want to comment some things

Daniel Turull wrote:
> 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. 

In order to make it to ipv6 compatible it will be better to create a new packet 
handler for the ipv6, because a different struct is need.
>> 2) Is it resistant to malicious packets ? (very small ones)
> I only tried with pktgen traffic

Pktgen is used in experimental environments. In order to start the receiver is necessary 
to load the module and enable the reception with the proc file system.
>> 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.
> 

Now I'm using pskb_may_pull() but the performance is reduced. 
>>> +	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.

Changed.

>>> +	/* 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
> 

I changed the 14 to ETH_HLEN. The VLAN, mpls overheads are not computed in this statistics because in the transmission, only the packet size introduced is counted.
>>> +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;
>>> +}
>>> +
>