* [PATCH] net: rocker: Add basic netdev counters
@ 2015-01-14 22:39 David Ahern
2015-01-14 22:57 ` Florian Fainelli
0 siblings, 1 reply; 4+ messages in thread
From: David Ahern @ 2015-01-14 22:39 UTC (permalink / raw)
To: netdev; +Cc: David Ahern, Scott Feldman, Jiri Pirko
Add packet and byte counters for RX and TX paths.
$ ifconfig eth1
eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
inet6 fe80::5054:ff:fe12:3501 prefixlen 64 scopeid 0x20<link>
ether 52:54:00:12:35:01 txqueuelen 1000 (Ethernet)
RX packets 63 bytes 15813 (15.4 KiB)
RX errors 0 dropped 0 overruns 0 frame 0
TX packets 79 bytes 17991 (17.5 KiB)
TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
Signed-off-by: David Ahern <dsahern@gmail.com>
Cc: Scott Feldman <sfeldma@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>
---
drivers/net/ethernet/rocker/rocker.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
index 2f398fa4b9e6..9743279d9121 100644
--- a/drivers/net/ethernet/rocker/rocker.c
+++ b/drivers/net/ethernet/rocker/rocker.c
@@ -3557,6 +3557,9 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
if (!desc_info)
netif_stop_queue(dev);
+ dev->stats.tx_packets++;
+ dev->stats.tx_bytes += skb->len;
+
return NETDEV_TX_OK;
unmap_frags:
@@ -3565,6 +3568,8 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
rocker_tlv_nest_cancel(desc_info, frags);
out:
dev_kfree_skb(skb);
+ dev->stats.tx_dropped++;
+
return NETDEV_TX_OK;
}
@@ -3890,6 +3895,9 @@ static int rocker_port_rx_proc(struct rocker *rocker,
skb->protocol = eth_type_trans(skb, rocker_port->dev);
netif_receive_skb(skb);
+ rocker_port->dev->stats.rx_packets++;
+ rocker_port->dev->stats.rx_bytes += skb->len;
+
return rocker_dma_rx_ring_skb_alloc(rocker, rocker_port, desc_info);
}
--
1.9.3 (Apple Git-50)
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] net: rocker: Add basic netdev counters
2015-01-14 22:39 [PATCH] net: rocker: Add basic netdev counters David Ahern
@ 2015-01-14 22:57 ` Florian Fainelli
2015-01-14 23:54 ` David Ahern
0 siblings, 1 reply; 4+ messages in thread
From: Florian Fainelli @ 2015-01-14 22:57 UTC (permalink / raw)
To: David Ahern, netdev; +Cc: Scott Feldman, Jiri Pirko
On 14/01/15 14:39, David Ahern wrote:
> Add packet and byte counters for RX and TX paths.
>
> $ ifconfig eth1
> eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
> inet6 fe80::5054:ff:fe12:3501 prefixlen 64 scopeid 0x20<link>
> ether 52:54:00:12:35:01 txqueuelen 1000 (Ethernet)
> RX packets 63 bytes 15813 (15.4 KiB)
> RX errors 0 dropped 0 overruns 0 frame 0
> TX packets 79 bytes 17991 (17.5 KiB)
> TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
>
> Signed-off-by: David Ahern <dsahern@gmail.com>
> Cc: Scott Feldman <sfeldma@gmail.com>
> Cc: Jiri Pirko <jiri@resnulli.us>
> ---
> drivers/net/ethernet/rocker/rocker.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
> index 2f398fa4b9e6..9743279d9121 100644
> --- a/drivers/net/ethernet/rocker/rocker.c
> +++ b/drivers/net/ethernet/rocker/rocker.c
> @@ -3557,6 +3557,9 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
> if (!desc_info)
> netif_stop_queue(dev);
>
> + dev->stats.tx_packets++;
> + dev->stats.tx_bytes += skb->len;
Potential use after free, the skb pointer is certainly not valid anymore
here.
BTW, increasing statistics here is valid because this is a driver for a
virtual piece of HW, which does not have TX reclaim/completion logic,
but if it did, statistics update should occur there, not in the
ndo_start_xmit() function.
> +
> return NETDEV_TX_OK;
>
> unmap_frags:
> @@ -3565,6 +3568,8 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
> rocker_tlv_nest_cancel(desc_info, frags);
> out:
> dev_kfree_skb(skb);
> + dev->stats.tx_dropped++;
> +
> return NETDEV_TX_OK;
> }
>
> @@ -3890,6 +3895,9 @@ static int rocker_port_rx_proc(struct rocker *rocker,
> skb->protocol = eth_type_trans(skb, rocker_port->dev);
> netif_receive_skb(skb);
>
> + rocker_port->dev->stats.rx_packets++;
> + rocker_port->dev->stats.rx_bytes += skb->len;
Same here, past netif_receive_skb() you should not assume that this skb
reference is valid.
> +
> return rocker_dma_rx_ring_skb_alloc(rocker, rocker_port, desc_info);
> }
>
>
--
Florian
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net: rocker: Add basic netdev counters
2015-01-14 22:57 ` Florian Fainelli
@ 2015-01-14 23:54 ` David Ahern
2015-01-16 19:53 ` Scott Feldman
0 siblings, 1 reply; 4+ messages in thread
From: David Ahern @ 2015-01-14 23:54 UTC (permalink / raw)
To: Florian Fainelli, netdev; +Cc: Scott Feldman, Jiri Pirko
On 1/14/15 3:57 PM, Florian Fainelli wrote:
> On 14/01/15 14:39, David Ahern wrote:
>> Add packet and byte counters for RX and TX paths.
>>
>> $ ifconfig eth1
>> eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
>> inet6 fe80::5054:ff:fe12:3501 prefixlen 64 scopeid 0x20<link>
>> ether 52:54:00:12:35:01 txqueuelen 1000 (Ethernet)
>> RX packets 63 bytes 15813 (15.4 KiB)
>> RX errors 0 dropped 0 overruns 0 frame 0
>> TX packets 79 bytes 17991 (17.5 KiB)
>> TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
>>
>> Signed-off-by: David Ahern <dsahern@gmail.com>
>> Cc: Scott Feldman <sfeldma@gmail.com>
>> Cc: Jiri Pirko <jiri@resnulli.us>
>> ---
>> drivers/net/ethernet/rocker/rocker.c | 8 ++++++++
>> 1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/rocker/rocker.c b/drivers/net/ethernet/rocker/rocker.c
>> index 2f398fa4b9e6..9743279d9121 100644
>> --- a/drivers/net/ethernet/rocker/rocker.c
>> +++ b/drivers/net/ethernet/rocker/rocker.c
>> @@ -3557,6 +3557,9 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
>> if (!desc_info)
>> netif_stop_queue(dev);
>>
>> + dev->stats.tx_packets++;
>> + dev->stats.tx_bytes += skb->len;
>
> Potential use after free, the skb pointer is certainly not valid anymore
> here.
>
> BTW, increasing statistics here is valid because this is a driver for a
> virtual piece of HW, which does not have TX reclaim/completion logic,
> but if it did, statistics update should occur there, not in the
> ndo_start_xmit() function.
sure. I had considered putting in the rocker_port_poll_tx function like
this:
- dev_kfree_skb_any(rocker_desc_cookie_ptr_get(desc_info));
+
+ skb = rocker_desc_cookie_ptr_get(desc_info);
+ rocker_port->dev->stats.tx_packets++;
+ rocker_port->dev->stats.tx_bytes += skb->len;
+
+ dev_kfree_skb_any(skb);
I think this the reclaim point.
>
>> +
>> return NETDEV_TX_OK;
>>
>> unmap_frags:
>> @@ -3565,6 +3568,8 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff *skb, struct net_device *dev)
>> rocker_tlv_nest_cancel(desc_info, frags);
>> out:
>> dev_kfree_skb(skb);
>> + dev->stats.tx_dropped++;
>> +
>> return NETDEV_TX_OK;
>> }
>>
>> @@ -3890,6 +3895,9 @@ static int rocker_port_rx_proc(struct rocker *rocker,
>> skb->protocol = eth_type_trans(skb, rocker_port->dev);
>> netif_receive_skb(skb);
>>
>> + rocker_port->dev->stats.rx_packets++;
>> + rocker_port->dev->stats.rx_bytes += skb->len;
>
> Same here, past netif_receive_skb() you should not assume that this skb
> reference is valid.
right. I'll move the stats above the netif_receive_skb.
Thanks,
David
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] net: rocker: Add basic netdev counters
2015-01-14 23:54 ` David Ahern
@ 2015-01-16 19:53 ` Scott Feldman
0 siblings, 0 replies; 4+ messages in thread
From: Scott Feldman @ 2015-01-16 19:53 UTC (permalink / raw)
To: David Ahern; +Cc: Florian Fainelli, Netdev, Jiri Pirko
On Wed, Jan 14, 2015 at 3:54 PM, David Ahern <dsahern@gmail.com> wrote:
> On 1/14/15 3:57 PM, Florian Fainelli wrote:
>>
>> On 14/01/15 14:39, David Ahern wrote:
>>>
>>> Add packet and byte counters for RX and TX paths.
>>>
>>> $ ifconfig eth1
>>> eth1: flags=4163<UP,BROADCAST,RUNNING,MULTICAST> mtu 1500
>>> inet6 fe80::5054:ff:fe12:3501 prefixlen 64 scopeid 0x20<link>
>>> ether 52:54:00:12:35:01 txqueuelen 1000 (Ethernet)
>>> RX packets 63 bytes 15813 (15.4 KiB)
>>> RX errors 0 dropped 0 overruns 0 frame 0
>>> TX packets 79 bytes 17991 (17.5 KiB)
>>> TX errors 0 dropped 0 overruns 0 carrier 0 collisions 0
>>>
>>> Signed-off-by: David Ahern <dsahern@gmail.com>
>>> Cc: Scott Feldman <sfeldma@gmail.com>
>>> Cc: Jiri Pirko <jiri@resnulli.us>
>>> ---
>>> drivers/net/ethernet/rocker/rocker.c | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/rocker/rocker.c
>>> b/drivers/net/ethernet/rocker/rocker.c
>>> index 2f398fa4b9e6..9743279d9121 100644
>>> --- a/drivers/net/ethernet/rocker/rocker.c
>>> +++ b/drivers/net/ethernet/rocker/rocker.c
>>> @@ -3557,6 +3557,9 @@ static netdev_tx_t rocker_port_xmit(struct sk_buff
>>> *skb, struct net_device *dev)
>>> if (!desc_info)
>>> netif_stop_queue(dev);
>>>
>>> + dev->stats.tx_packets++;
>>> + dev->stats.tx_bytes += skb->len;
>>
>>
>> Potential use after free, the skb pointer is certainly not valid anymore
>> here.
>>
>> BTW, increasing statistics here is valid because this is a driver for a
>> virtual piece of HW, which does not have TX reclaim/completion logic,
>> but if it did, statistics update should occur there, not in the
>> ndo_start_xmit() function.
>
>
> sure. I had considered putting in the rocker_port_poll_tx function like
> this:
>
> - dev_kfree_skb_any(rocker_desc_cookie_ptr_get(desc_info));
> +
> + skb = rocker_desc_cookie_ptr_get(desc_info);
> + rocker_port->dev->stats.tx_packets++;
> + rocker_port->dev->stats.tx_bytes += skb->len;
> +
> + dev_kfree_skb_any(skb);
>
> I think this the reclaim point.
This is better. But also need to consider err code returned from
device in same routine. If the desc was marked with err, then I think
we can assume pkt wasn't sent so tx_packets/bytes stats should not be
incremented, but some tx err stat should, based on desc err code.
-scott
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-01-16 19:53 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-14 22:39 [PATCH] net: rocker: Add basic netdev counters David Ahern
2015-01-14 22:57 ` Florian Fainelli
2015-01-14 23:54 ` David Ahern
2015-01-16 19:53 ` Scott Feldman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).