Skip to contentSkip to conversation

net: add comment for sock_efree() usage

PATCH In review Oliver Hartkopp 3 participants 5 messages Mar 10, 2015

Subsystems (3)
Networking CAN network drivers Sockets
lore
Share & tools
lore

Oliver Hartkopp

author
#0 lore raw

PATCH 1/1 net: add comment for sock_efree() usage

In opposite to sock_rfree() and sock_wfree() the function sock_efree() does not need to change the sk_[rw]_mem_alloc length. Add the comment to point out the idea for using sock_efree() in the _e_rror handler or e.g. timestamp path.

Signed-off-by Oliver Hartkopp <[email protected]>
---
 net/core/sock.c | 4 ++++
 1 file changed, 4 insertions(+)
net/core/sock.c +4 -0
index 93c8b20..7a1eac8 100644
@@ -1655,6 +1655,10 @@ void sock_rfree(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(sock_rfree);
 
+/*
+ * Buffer destructor for skbs that don't have sk_[rw]_mem_alloc accounting.
+ * E.g. error handler / timestamp path. Automatically called from kfree_skb.
+ */
 void sock_efree(struct sk_buff *skb)
 {
 	sock_put(skb->sk);
Signature · 1 line
2.1.4
#1 lore raw
Quoted 14 lines · Oliver Hartkopp

In opposite to sock_rfree() and sock_wfree() the function sock_efree() does not need to change the sk_[rw]_mem_alloc length. Add the comment to point out the idea for using sock_efree() in the _e_rror handler or e.g. timestamp path.

Signed-off-by Oliver Hartkopp <[email protected]>
---
  net/core/sock.c | 4 ++++
  1 file changed, 4 insertions(+)
net/core/sock.c +0 -0
index 93c8b20..7a1eac8 100644

net/core/sock.c void sock_rfree(struct sk_buff *skb) · line 1664

Quoted 10 lines
net/core/sock.c +4 -0
@@ -1655,6 +1655,10 @@ void sock_rfree(struct sk_buff *skb)
  }
  EXPORT_SYMBOL(sock_rfree);
 
+/*
+ * Buffer destructor for skbs that don't have sk_[rw]_mem_alloc accounting.
+ * E.g. error handler / timestamp path. Automatically called from kfree_skb.
+ */
  void sock_efree(struct sk_buff *skb)
  {
  	sock_put(skb->sk);

This should probably be pushed out to the netdev list since this effects all sockets.

Technically speaking there is a 1 byte sk_wmem_alloc that this is accounting for to keep the socket from being closed. It is shared between all instances that are using sock_efree as their destructor, and will drop that 1 byte when sk_refcnt has reached 0.

- Alex

author
#2 lore raw
Quoted 19 lines · Alexander Duyck
Quoted 18 lines · Oliver Hartkopp

In opposite to sock_rfree() and sock_wfree() the function sock_efree() does not need to change the sk_[rw]_mem_alloc length. Add the comment to point out the idea for using sock_efree() in the _e_rror handler or e.g. timestamp path.

Signed-off-by Oliver Hartkopp <[email protected]>
---
  net/core/sock.c | 4 ++++
  1 file changed, 4 insertions(+)
net/core/sock.c +1 -0
index 93c8b20..7a1eac8 100644
@@ -1655,6 +1655,10 @@ void sock_rfree(struct sk_buff *skb)
  }
  EXPORT_SYMBOL(sock_rfree);
  +/*
+ * Buffer destructor for skbs that don't have sk_[rw]_mem_alloc accounting.
Quoted 8 lines

net/core/sock.c void sock_rfree(struct sk_buff *skb) · line 1664

Quoted 5 lines
net/core/sock.c +2 -0
+ * E.g. error handler / timestamp path. Automatically called from kfree_skb.
+ */
  void sock_efree(struct sk_buff *skb)
  {
      sock_put(skb->sk);

This should probably be pushed out to the netdev list since this effects all sockets.

Yes. That was my default. Will put netdev in --to next time.

Quoted 4 lines

Technically speaking there is a 1 byte sk_wmem_alloc that this is accounting for to keep the socket from being closed. It is shared between all instances that are using sock_efree as their destructor, and will drop that 1 byte when sk_refcnt has reached 0.

Ok. What would be your suggestion then?

'destructor that doesn't fiddle with sk_[rw]_mem_alloc length' ??? :-)))

Best regards,
Oliver

#3 lore raw
Quoted 33 lines · Oliver Hartkopp
Quoted 10 lines · Alexander Duyck
Quoted 9 lines · Oliver Hartkopp

In opposite to sock_rfree() and sock_wfree() the function sock_efree() does not need to change the sk_[rw]_mem_alloc length. Add the comment to point out the idea for using sock_efree() in the _e_rror handler or e.g. timestamp path.

Signed-off-by Oliver Hartkopp <[email protected]>
---
   net/core/sock.c | 4 ++++
   1 file changed, 4 insertions(+)
Quoted 16 lines
Quoted 5 lines
net/core/sock.c +0 -0
index 93c8b20..7a1eac8 100644

net/core/sock.c void sock_rfree(struct sk_buff *skb) · line 1663

Quoted 9 lines
net/core/sock.c +3 -0
@@ -1655,6 +1655,10 @@ void sock_rfree(struct sk_buff *skb)
   }
   EXPORT_SYMBOL(sock_rfree);
   +/*
+ * Buffer destructor for skbs that don't have sk_[rw]_mem_alloc accounting.
+ * E.g. error handler / timestamp path. Automatically called from kfree_skb.
+ */
   void sock_efree(struct sk_buff *skb)
   {
       sock_put(skb->sk);

This should probably be pushed out to the netdev list since this effects all sockets.

Yes. That was my default. Will put netdev in --to next time.

Quoted 4 lines

Technically speaking there is a 1 byte sk_wmem_alloc that this is accounting for to keep the socket from being closed. It is shared between all instances that are using sock_efree as their destructor, and will drop that 1 byte when sk_refcnt has reached 0.

Quoted 6 lines

Ok. What would be your suggestion then?

'destructor that doesn't fiddle with sk_[rw]_mem_alloc length' ??? :-)))

Best regards,
Oliver

I'd suggest keeping it simple. You are trying to define it as something it isn't. It still does accounting, your description makes it sound like it doesn't.

For example sock_rfree doesn't tell you where it is supposed to be used and what it is accounting for. It just is described as a read buffer descructor called from kfree_skb. I'd suggest something similar, maybe just describe it as a desctructor used to free buffers that are not used directly in read or write.

- Alex

maintainer
#4 lore raw
In reply to Oliver Hartkopp
Quoted 14 lines · Oliver Hartkopp

In opposite to sock_rfree() and sock_wfree() the function sock_efree() does not need to change the sk_[rw]_mem_alloc length. Add the comment to point out the idea for using sock_efree() in the _e_rror handler or e.g. timestamp path.

Signed-off-by Oliver Hartkopp <[email protected]>
---
 net/core/sock.c | 4 ++++
 1 file changed, 4 insertions(+)
net/core/sock.c +0 -0
index 93c8b20..7a1eac8 100644

net/core/sock.c void sock_rfree(struct sk_buff *skb) · line 1661

Quoted 7 lines
net/core/sock.c +4 -0
@@ -1655,6 +1655,10 @@ void sock_rfree(struct sk_buff *skb)
 }
 EXPORT_SYMBOL(sock_rfree);
 
+/*
+ * Buffer destructor for skbs that don't have sk_[rw]_mem_alloc accounting.
+ * E.g. error handler / timestamp path. Automatically called from kfree_skb.
+ */

/* multiline comments in net
* work this
* way
*/

net/core/sock.c void sock_rfree(struct sk_buff *skb) · line 1664

Quoted 4 lines
net/core/sock.c +0 -0
 void sock_efree(struct sk_buff *skb)
 {
 	sock_put(skb->sk);

Marc

Signature · 4 lines
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |