* [PATCH net-next v3] packet: uses kfree_skb() for errors.
@ 2016-04-08 16:25 Weongyo Jeong
2016-04-14 2:56 ` David Miller
0 siblings, 1 reply; 3+ messages in thread
From: Weongyo Jeong @ 2016-04-08 16:25 UTC (permalink / raw)
To: netdev; +Cc: Weongyo Jeong, David S. Miller, Willem de Bruijn
consume_skb() isn't for error cases that kfree_skb() is more proper
one. At this patch, it fixed tpacket_rcv() and packet_rcv() to be
consistent for error or non-error cases letting perf trace its event
properly.
Signed-off-by: Weongyo Jeong <weongyo.linux@gmail.com>
---
net/packet/af_packet.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
index 1ecfa71..4e054bb 100644
--- a/net/packet/af_packet.c
+++ b/net/packet/af_packet.c
@@ -2042,6 +2042,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
u8 *skb_head = skb->data;
int skb_len = skb->len;
unsigned int snaplen, res;
+ bool err = false;
if (skb->pkt_type == PACKET_LOOPBACK)
goto drop;
@@ -2130,6 +2131,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
return 0;
drop_n_acct:
+ err = true;
spin_lock(&sk->sk_receive_queue.lock);
po->stats.stats1.tp_drops++;
atomic_inc(&sk->sk_drops);
@@ -2141,7 +2143,10 @@ drop_n_restore:
skb->len = skb_len;
}
drop:
- consume_skb(skb);
+ if (!err)
+ consume_skb(skb);
+ else
+ kfree_skb(skb);
return 0;
}
@@ -2160,6 +2165,7 @@ static int tpacket_rcv(struct sk_buff *skb, struct net_device *dev,
struct sk_buff *copy_skb = NULL;
struct timespec ts;
__u32 ts_status;
+ bool err = false;
/* struct tpacket{2,3}_hdr is aligned to a multiple of TPACKET_ALIGNMENT.
* We may add members to them until current aligned size without forcing
@@ -2367,10 +2373,14 @@ drop_n_restore:
skb->len = skb_len;
}
drop:
- kfree_skb(skb);
+ if (!err)
+ consume_skb(skb);
+ else
+ kfree_skb(skb);
return 0;
drop_n_account:
+ err = true;
po->stats.stats1.tp_drops++;
spin_unlock(&sk->sk_receive_queue.lock);
--
2.1.3
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH net-next v3] packet: uses kfree_skb() for errors.
2016-04-08 16:25 [PATCH net-next v3] packet: uses kfree_skb() for errors Weongyo Jeong
@ 2016-04-14 2:56 ` David Miller
2016-04-14 21:11 ` Weongyo Jeong
0 siblings, 1 reply; 3+ messages in thread
From: David Miller @ 2016-04-14 2:56 UTC (permalink / raw)
To: weongyo.linux; +Cc: netdev, willemb
From: Weongyo Jeong <weongyo.linux@gmail.com>
Date: Fri, 8 Apr 2016 09:25:48 -0700
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 1ecfa71..4e054bb 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2042,6 +2042,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
> u8 *skb_head = skb->data;
> int skb_len = skb->len;
> unsigned int snaplen, res;
> + bool err = false;
>
> if (skb->pkt_type == PACKET_LOOPBACK)
> goto drop;
It is non-canonical to use a variable named 'err' as a boolean.
Instead, all programmers expect a variable named 'err' to be an
integer type and to hold an error code.
Therefore please use a more appropriate name for this variable.
Name it in a way which describes the exact important condition
which is true or false.
Thank you.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH net-next v3] packet: uses kfree_skb() for errors.
2016-04-14 2:56 ` David Miller
@ 2016-04-14 21:11 ` Weongyo Jeong
0 siblings, 0 replies; 3+ messages in thread
From: Weongyo Jeong @ 2016-04-14 21:11 UTC (permalink / raw)
To: David Miller; +Cc: netdev, willemb
On Wed, Apr 13, 2016 at 10:56:01PM -0400, David Miller wrote:
> From: Weongyo Jeong <weongyo.linux@gmail.com>
> Date: Fri, 8 Apr 2016 09:25:48 -0700
>
> > diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> > index 1ecfa71..4e054bb 100644
> > --- a/net/packet/af_packet.c
> > +++ b/net/packet/af_packet.c
> > @@ -2042,6 +2042,7 @@ static int packet_rcv(struct sk_buff *skb, struct net_device *dev,
> > u8 *skb_head = skb->data;
> > int skb_len = skb->len;
> > unsigned int snaplen, res;
> > + bool err = false;
> >
> > if (skb->pkt_type == PACKET_LOOPBACK)
> > goto drop;
>
> It is non-canonical to use a variable named 'err' as a boolean.
> Instead, all programmers expect a variable named 'err' to be an
> integer type and to hold an error code.
>
> Therefore please use a more appropriate name for this variable.
>
> Name it in a way which describes the exact important condition
> which is true or false.
>
> Thank you.
Thank you for your opinion. I'd fixed it to proper name and
submitted v4 patch.
Regards,
Weongyo Jeong
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-04-14 21:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-08 16:25 [PATCH net-next v3] packet: uses kfree_skb() for errors Weongyo Jeong
2016-04-14 2:56 ` David Miller
2016-04-14 21:11 ` Weongyo Jeong
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).