Home » Mailing list archives » linux-netdev » 2009 » March » 4

Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs

Previous thread: [Patch 2/5] Network Drop Monitor: Add trace declaration for skb frees by Neil Horman on Tuesday, March 3, 2009 - 10:01 am. (3 messages)

Next thread: [Patch 4/5] Network Drop Monitor: Adding drop monitor implementation & Netlink protocol by Neil Horman on Tuesday, March 3, 2009 - 10:04 am. (31 messages)
[view original message]
From: Neil Horman
Subject: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Tuesday, March 3, 2009 - 10:03 am

Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 include/linux/skbuff.h |    4 +++-
 net/core/datagram.c    |    2 +-
 net/core/skbuff.c      |   22 ++++++++++++++++++++++
 net/ipv4/arp.c         |    2 +-
 net/ipv4/udp.c         |    2 +-
 net/packet/af_packet.c |    2 +-
 6 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 1f659e8..d328267 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -421,6 +421,7 @@ extern void skb_dma_unmap(struct device *dev, struct sk_buff *skb,
 #endif
 
 extern void kfree_skb(struct sk_buff *skb);
+extern void kfree_skb_clean(struct sk_buff *skb);
 extern void	       __kfree_skb(struct sk_buff *skb);
 extern struct sk_buff *__alloc_skb(unsigned int size,
 				   gfp_t priority, int fclone, int node);
@@ -459,7 +460,8 @@ extern int	       skb_to_sgvec(struct sk_buff *skb,
 extern int	       skb_cow_data(struct sk_buff *skb, int tailbits,
 				    struct sk_buff **trailer);
 extern int	       skb_pad(struct sk_buff *skb, int pad);
-#define dev_kfree_skb(a)	kfree_skb(a)
+#define dev_kfree_skb(a)	kfree_skb_clean(a)
+#define dev_kfree_skb_clean(a)	kfree_skb_clean(a)
 extern void	      skb_over_panic(struct sk_buff *skb, int len,
 				     void *here);
 extern void	      skb_under_panic(struct sk_buff *skb, int len,
diff --git a/net/core/datagram.c b/net/core/datagram.c
index 5e2ac0c..6cb2723 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -208,7 +208,7 @@ struct sk_buff *skb_recv_datagram(struct sock *sk, unsigned flags,
 
 void skb_free_datagram(struct sock *sk, struct sk_buff *skb)
 {
-	kfree_skb(skb);
+	kfree_skb_clean(skb);
 	sk_mem_reclaim_partial(sk);
 }
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e5e2111..4458ec8 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -65,6 +65,7 @@
 
 #include ...
[ message continues ]
[view original message]
From: Arnaldo Carvalho de Melo
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 4, 2009 - 6:54 am

Why just not add a "drop_skb()" instead? kfree_skb_clean sounds as if we
will do some cleanup on the skb being freed. "drop_skb()" seems much
clearer.

- Arnaldo
--


[ message continues ]
[view original message]
From: Neil Horman
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 4, 2009 - 7:18 am

Check the RFC thread.  The cases in which we are dropping far outnumber the set
of cases in which we free the skb simply because its the end of the line.
Theres far less churn this way

Neil

--


[ message continues ]
[view original message]
From: Arnaldo Carvalho de Melo
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 4, 2009 - 9:13 am

I wouldn't mind the churn, as I really think drop_skb() would be
clearer, but then this is up to Dave.

- Arnaldo
--


[ message continues ]
[view original message]
From: David Miller
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 4, 2009 - 2:06 pm

From: Arnaldo Carvalho de Melo <acme@redhat.com>

I mind the churn, I have to apply this thing and deal with
the merge conflicts :-)

Why not come up with a clever alternative name for kfree_skb_clean()?
--


[ message continues ]
[view original message]
From: Arnaldo Carvalho de Melo
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 4, 2009 - 3:45 pm

Oh well, sometimes churn is inevitable, but lemme try to figure out some
more clearer names for kfree_skb_clean:

rest_in_peace_skb()
here_lies_a_productive_skb()
kfree_not_dropped_skb()
kfree_worthy_skb()
consumed_skb()
hasta_la_vista_skbaby()
kfree_used_skb()

Perhaps the last one, huh? :-)

- Arnaldo

P.S.: </joke'ishy moment>
--


[ message continues ]
[view original message]
From: David Miller
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 4, 2009 - 3:47 pm

From: Arnaldo Carvalho de Melo <acme@redhat.com>


But more seriously I like something like "consume_skb()" the best (ie.
drop the 'd').  It has the implication that an application or other
entity "consumed" and used the data before we freed the SKB.

What do you think?
--


[ message continues ]
[view original message]
From: Arnaldo Carvalho de Melo
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 4, 2009 - 3:51 pm

ROTFL, I have to get that book someday... For now I'm reading a brick

That would be better than kfree_skb_clean, yes.

- Arnaldo

P.S.: Neil, I didn't managed to get to the other aspects of your work,
sorry about being so picky :-\
--


[ message continues ]
[view original message]
From: Patrick McHardy
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 4, 2009 - 4:57 pm

My clear favourite as well. Maybe for some procedure that leaks
the skb :)
--


[ message continues ]
[view original message]
From: Neil Horman
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Thursday, March 5, 2009 - 2:01 pm

Ok, as requested, new version of this patch to add in bifurcation of kfree_skb
into two versions, one for legitimate kfree's, and one for drops

Modification Notes:
1) Renamed kfree_skb_clean to consume_skb, to better identify points where we
are finished with an skb

2) checkpatch cleanups

Thanks!

Neil

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 include/linux/skbuff.h |    4 +++-
 net/core/datagram.c    |    2 +-
 net/core/skbuff.c      |   22 ++++++++++++++++++++++
 net/ipv4/arp.c         |    2 +-
 net/ipv4/udp.c         |    2 +-
 net/packet/af_packet.c |    2 +-
 6 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 1f659e8..1fbab2a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -421,6 +421,7 @@ extern void skb_dma_unmap(struct device *dev, struct sk_buff *skb,
 #endif
 
 extern void kfree_skb(struct sk_buff *skb);
+extern void consume_skb(struct sk_buff *skb);
 extern void	       __kfree_skb(struct sk_buff *skb);
 extern struct sk_buff *__alloc_skb(unsigned int size,
 				   gfp_t priority, int fclone, int node);
@@ -459,7 +460,8 @@ extern int	       skb_to_sgvec(struct sk_buff *skb,
 extern int	       skb_cow_data(struct sk_buff *skb, int tailbits,
 				    struct sk_buff **trailer);
 extern int	       skb_pad(struct sk_buff *skb, int pad);
-#define dev_kfree_skb(a)	kfree_skb(a)
+#define dev_kfree_skb(a)	consume_skb(a)
+#define dev_consume_skb(a)	kfree_skb_clean(a)
 extern void	      skb_over_panic(struct sk_buff *skb, int len,
 				     void *here);
 extern void	      skb_under_panic(struct sk_buff *skb, int len,
diff --git a/net/core/datagram.c b/net/core/datagram.c
index 5e2ac0c..d0de644 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -208,7 +208,7 @@ struct sk_buff *skb_recv_datagram(struct sock *sk, unsigned flags,
 
 void skb_free_datagram(struct sock *sk, struct sk_buff *skb)
 {
-	kfree_skb(skb);
+	consume_skb(skb);
 ...
[ message continues ]
[view original message]
From: David Miller
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 11, 2009 - 9:18 am

From: Neil Horman <nhorman@tuxdriver.com>

Looks good.

Respin everything with the requested changes I asked for in #4
and I can likely toss this into net-next-2.6, thanks!
--


[ message continues ]
[view original message]
From: Eric Dumazet
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 11, 2009 - 9:34 am

I was wondering if packets droped at NIC level are handled by "Network Drop Monitor".
(See recent discussion about Multicast packet loss)

I believe not, maybe I missed something ...

--


[ message continues ]
[view original message]
From: David Miller
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 11, 2009 - 9:48 am

From: Eric Dumazet <dada1@cosmosbay.com>

No, it's just a "software network drop monitor"
--


[ message continues ]
[view original message]
From: Neil Horman
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 11, 2009 - 10:07 am

No, unfortunately, theres no really good way to detect drops at the driver level
without polling them periodically, which I think is somewhat in opposition to the
point of this utility.  I'd welcome suggestions however.
--


[ message continues ]
[view original message]
From: Evgeniy Polyakov
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 11, 2009 - 2:39 pm

It could be possible to check softnet stats difference when napi is
scheduled or drop monitor is about to send a packet to the userspace
about different drop.

-- 
	Evgeniy Polyakov
--


[ message continues ]
[view original message]
From: Neil Horman
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 11, 2009 - 5:30 pm

Those are both good ideas.  I think I like the former suggestion, since the
latter requires that a drop in the stack must occur to detect a stack closer to
the hardware.  Thank you, I'll keep that in mind for a subsequent enhancement.

Regards
--


[ message continues ]
[view original message]
From: Neil Horman
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Wednesday, March 11, 2009 - 12:49 pm

As requested, respinning against latest net-next tree.

Modification notes:

None, simple respin

Neil


Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>


 include/linux/skbuff.h |    4 +++-
 net/core/datagram.c    |    2 +-
 net/core/skbuff.c      |   22 ++++++++++++++++++++++
 net/ipv4/arp.c         |    2 +-
 net/ipv4/udp.c         |    2 +-
 net/packet/af_packet.c |    2 +-
 6 files changed, 29 insertions(+), 5 deletions(-)


diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 1f659e8..1fbab2a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -421,6 +421,7 @@ extern void skb_dma_unmap(struct device *dev, struct sk_buff *skb,
 #endif
 
 extern void kfree_skb(struct sk_buff *skb);
+extern void consume_skb(struct sk_buff *skb);
 extern void	       __kfree_skb(struct sk_buff *skb);
 extern struct sk_buff *__alloc_skb(unsigned int size,
 				   gfp_t priority, int fclone, int node);
@@ -459,7 +460,8 @@ extern int	       skb_to_sgvec(struct sk_buff *skb,
 extern int	       skb_cow_data(struct sk_buff *skb, int tailbits,
 				    struct sk_buff **trailer);
 extern int	       skb_pad(struct sk_buff *skb, int pad);
-#define dev_kfree_skb(a)	kfree_skb(a)
+#define dev_kfree_skb(a)	consume_skb(a)
+#define dev_consume_skb(a)	kfree_skb_clean(a)
 extern void	      skb_over_panic(struct sk_buff *skb, int len,
 				     void *here);
 extern void	      skb_under_panic(struct sk_buff *skb, int len,
diff --git a/net/core/datagram.c b/net/core/datagram.c
index 5e2ac0c..d0de644 100644
--- a/net/core/datagram.c
+++ b/net/core/datagram.c
@@ -208,7 +208,7 @@ struct sk_buff *skb_recv_datagram(struct sock *sk, unsigned flags,
 
 void skb_free_datagram(struct sock *sk, struct sk_buff *skb)
 {
-	kfree_skb(skb);
+	consume_skb(skb);
 	sk_mem_reclaim_partial(sk);
 }
 
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index e5e2111..6acbf9e ...
[ message continues ]
[view original message]
From: David Miller
Subject: Re: [Patch 3/5] Network Drop Monitor: Adding kfree_skb_clean for non-drops and modifying end-of-line points for skbs
Date: Friday, March 13, 2009 - 12:10 pm

From: Neil Horman <nhorman@tuxdriver.com>

Applied.
--


[ message continues ]
Previous thread: [Patch 2/5] Network Drop Monitor: Add trace declaration for skb frees by Neil Horman on Tuesday, March 3, 2009 - 10:01 am. (3 messages)

Next thread: [Patch 4/5] Network Drop Monitor: Adding drop monitor implementation & Netlink protocol by Neil Horman on Tuesday, March 3, 2009 - 10:04 am. (31 messages)