* Re: [PATCH] can: use sock_efree instead of own destructor
[not found] <1425959300-27132-1-git-send-email-fw@strlen.de>
@ 2015-03-10 6:14 ` Oliver Hartkopp
2015-03-10 12:22 ` Eric Dumazet
2015-03-10 7:50 ` Marc Kleine-Budde
1 sibling, 1 reply; 7+ messages in thread
From: Oliver Hartkopp @ 2015-03-10 6:14 UTC (permalink / raw)
To: Florian Westphal; +Cc: netdev, mkl, linux-can
On 10.03.2015 04:48, Florian Westphal wrote:
> It is identical to the can destructor.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
Acked-by: Oliver Hartkopp <socketcan@hartkopp.net>
Hello Florian,
the other callers use it in the same way so it's a good simplification.
Btw. the name of sock_efree() is a bit misleading - nothing is free'd here.
Won't it be better to rename sock_efree(skb) with sock_put_skb(skb) or
something like that? sock_efree() has no comment why it's named like this.
Regards,
Oliver
ps. changed from linux ML to netdev and linux-can ML in CC
> ---
> include/linux/can/skb.h | 7 +------
> 1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/include/linux/can/skb.h b/include/linux/can/skb.h
> index cc00d15..b6a52a4 100644
> --- a/include/linux/can/skb.h
> +++ b/include/linux/can/skb.h
> @@ -44,16 +44,11 @@ static inline void can_skb_reserve(struct sk_buff *skb)
> skb_reserve(skb, sizeof(struct can_skb_priv));
> }
>
> -static inline void can_skb_destructor(struct sk_buff *skb)
> -{
> - sock_put(skb->sk);
> -}
> -
> static inline void can_skb_set_owner(struct sk_buff *skb, struct sock *sk)
> {
> if (sk) {
> sock_hold(sk);
> - skb->destructor = can_skb_destructor;
> + skb->destructor = sock_efree;
> skb->sk = sk;
> }
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: use sock_efree instead of own destructor
[not found] <1425959300-27132-1-git-send-email-fw@strlen.de>
2015-03-10 6:14 ` [PATCH] can: use sock_efree instead of own destructor Oliver Hartkopp
@ 2015-03-10 7:50 ` Marc Kleine-Budde
1 sibling, 0 replies; 7+ messages in thread
From: Marc Kleine-Budde @ 2015-03-10 7:50 UTC (permalink / raw)
To: Florian Westphal, linux-kernel; +Cc: socketcan, linux-can
[-- Attachment #1: Type: text/plain, Size: 462 bytes --]
On 03/10/2015 04:48 AM, Florian Westphal wrote:
> It is identical to the can destructor.
>
> Signed-off-by: Florian Westphal <fw@strlen.de>
Thanks, applied to can-next.
Marc
--
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 |
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: use sock_efree instead of own destructor
2015-03-10 6:14 ` [PATCH] can: use sock_efree instead of own destructor Oliver Hartkopp
@ 2015-03-10 12:22 ` Eric Dumazet
2015-03-10 13:36 ` Oliver Hartkopp
0 siblings, 1 reply; 7+ messages in thread
From: Eric Dumazet @ 2015-03-10 12:22 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Florian Westphal, netdev, mkl, linux-can
On Tue, 2015-03-10 at 07:14 +0100, Oliver Hartkopp wrote:
> the other callers use it in the same way so it's a good simplification.
> Btw. the name of sock_efree() is a bit misleading - nothing is free'd here.
>
> Won't it be better to rename sock_efree(skb) with sock_put_skb(skb) or
> something like that? sock_efree() has no comment why it's named like this.
I would prefer name stays as is. It eases searches in changelogs to not
change function names unless really needed, for backports and code
maintenance.
# git log | grep sock_efree
net: merge cases where sock_efree and sock_edemux are the same function
Since sock_efree and sock_demux are essentially the same code for non-TCP
In addition I have added a destructor named sock_efree which is meant to
sock_efree was added in commit 62bccb8cdb69051b95a55ab0c489e3cab261c8ef
I guess Alexander chose the name close to sock_edemux() and sock_rfree() ones.
This makes sense to me at least.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: use sock_efree instead of own destructor
2015-03-10 12:22 ` Eric Dumazet
@ 2015-03-10 13:36 ` Oliver Hartkopp
2015-03-10 14:36 ` Eric Dumazet
0 siblings, 1 reply; 7+ messages in thread
From: Oliver Hartkopp @ 2015-03-10 13:36 UTC (permalink / raw)
To: Eric Dumazet, Alexander Duyck; +Cc: Florian Westphal, netdev, mkl, linux-can
Yes - in connection with sock_rfree() for the read buffer destructur and
sock_wfree() for the write buffer it can make sense to name a function
sock_efree() as an unassigned destructor - which does not fiddle with rmem nor
wmen.
But both sock_efree() and sock_edemux() lack some comment - especially when it
makes sense to use them from non-INET contexts which Florian suggested.
Maybe Alexander can send a patch which adds a comment, as I don't know if I
would find the best words for it.
Regards,
Oliver
On 03/10/2015 01:22 PM, Eric Dumazet wrote:
> On Tue, 2015-03-10 at 07:14 +0100, Oliver Hartkopp wrote:
>
>> the other callers use it in the same way so it's a good simplification.
>> Btw. the name of sock_efree() is a bit misleading - nothing is free'd here.
>>
>> Won't it be better to rename sock_efree(skb) with sock_put_skb(skb) or
>> something like that? sock_efree() has no comment why it's named like this.
>
> I would prefer name stays as is. It eases searches in changelogs to not
> change function names unless really needed, for backports and code
> maintenance.
>
>
> # git log | grep sock_efree
> net: merge cases where sock_efree and sock_edemux are the same function
> Since sock_efree and sock_demux are essentially the same code for non-TCP
> In addition I have added a destructor named sock_efree which is meant to
>
> sock_efree was added in commit 62bccb8cdb69051b95a55ab0c489e3cab261c8ef
>
> I guess Alexander chose the name close to sock_edemux() and sock_rfree() ones.
>
> This makes sense to me at least.
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: use sock_efree instead of own destructor
2015-03-10 13:36 ` Oliver Hartkopp
@ 2015-03-10 14:36 ` Eric Dumazet
2015-03-10 14:55 ` Oliver Hartkopp
2015-03-10 15:19 ` Alexander Duyck
0 siblings, 2 replies; 7+ messages in thread
From: Eric Dumazet @ 2015-03-10 14:36 UTC (permalink / raw)
To: Oliver Hartkopp; +Cc: Alexander Duyck, Florian Westphal, netdev, mkl, linux-can
On Tue, 2015-03-10 at 14:36 +0100, Oliver Hartkopp wrote:
> Yes - in connection with sock_rfree() for the read buffer destructur and
> sock_wfree() for the write buffer it can make sense to name a function
> sock_efree() as an unassigned destructor - which does not fiddle with rmem nor
> wmen.
>
> But both sock_efree() and sock_edemux() lack some comment - especially when it
> makes sense to use them from non-INET contexts which Florian suggested.
>
> Maybe Alexander can send a patch which adds a comment, as I don't know if I
> would find the best words for it.
Please do not top post on netdev.
If you cannot find best words for it, maybe a comment would not be
useful : Very often, best comments are added by people that had problems
to understand the code ;)
I do not trust comments, I prefer using "git grep" or tools like that to
check call sites.
In this particular case this becomes clear, while being concise.
# git grep -n sock_efree
include/net/sock.h:1526:void sock_efree(struct sk_buff *skb);
include/net/sock.h:1530:#define sock_edemux(skb) sock_efree(skb)
net/core/skbuff.c:3625: clone->destructor = sock_efree;
net/core/sock.c:1658:void sock_efree(struct sk_buff *skb)
net/core/sock.c:1662:EXPORT_SYMBOL(sock_efree);
net/ipv4/udp.c:1992: skb->destructor = sock_efree;
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: use sock_efree instead of own destructor
2015-03-10 14:36 ` Eric Dumazet
@ 2015-03-10 14:55 ` Oliver Hartkopp
2015-03-10 15:19 ` Alexander Duyck
1 sibling, 0 replies; 7+ messages in thread
From: Oliver Hartkopp @ 2015-03-10 14:55 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Florian Westphal, netdev, mkl, linux-can, Alexander Duyck
On 03/10/2015 03:36 PM, Eric Dumazet wrote:
> On Tue, 2015-03-10 at 14:36 +0100, Oliver Hartkopp wrote:
>> Maybe Alexander can send a patch which adds a comment, as I don't know if I
>> would find the best words for it.
>
> Please do not top post on netdev.
? I did not.
> If you cannot find best words for it, maybe a comment would not be
> useful : Very often, best comments are added by people that had problems
> to understand the code ;)
Yes that's why I asked the original author to do so - he hopefully understood
his code :-)
>
> I do not trust comments, I prefer using "git grep" or tools like that to
> check call sites.
>
> In this particular case this becomes clear, while being concise.
>
> # git grep -n sock_efree
A good hint! Will use this first in the future.
Thanks,
Oliver
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] can: use sock_efree instead of own destructor
2015-03-10 14:36 ` Eric Dumazet
2015-03-10 14:55 ` Oliver Hartkopp
@ 2015-03-10 15:19 ` Alexander Duyck
1 sibling, 0 replies; 7+ messages in thread
From: Alexander Duyck @ 2015-03-10 15:19 UTC (permalink / raw)
To: Eric Dumazet, Oliver Hartkopp
Cc: Alexander Duyck, Florian Westphal, netdev, mkl, linux-can
On 03/10/2015 07:36 AM, Eric Dumazet wrote:
> On Tue, 2015-03-10 at 14:36 +0100, Oliver Hartkopp wrote:
>> Yes - in connection with sock_rfree() for the read buffer destructur and
>> sock_wfree() for the write buffer it can make sense to name a function
>> sock_efree() as an unassigned destructor - which does not fiddle with rmem nor
>> wmen.
>>
>> But both sock_efree() and sock_edemux() lack some comment - especially when it
>> makes sense to use them from non-INET contexts which Florian suggested.
>>
>> Maybe Alexander can send a patch which adds a comment, as I don't know if I
>> would find the best words for it.
> Please do not top post on netdev.
>
> If you cannot find best words for it, maybe a comment would not be
> useful : Very often, best comments are added by people that had problems
> to understand the code ;)
>
> I do not trust comments, I prefer using "git grep" or tools like that to
> check call sites.
>
> In this particular case this becomes clear, while being concise.
>
> # git grep -n sock_efree
> include/net/sock.h:1526:void sock_efree(struct sk_buff *skb);
> include/net/sock.h:1530:#define sock_edemux(skb) sock_efree(skb)
> net/core/skbuff.c:3625: clone->destructor = sock_efree;
> net/core/sock.c:1658:void sock_efree(struct sk_buff *skb)
> net/core/sock.c:1662:EXPORT_SYMBOL(sock_efree);
> net/ipv4/udp.c:1992: skb->destructor = sock_efree;
As I recall one of the reasons for leaving it as sock_efree was because
sock_put just calls sk_free if it has decremented the reference count to
0. Also as Eric pointed out the name is actually pretty close to the
existing destructors sock_edemux and sock_rfree. The sock_efree name
was based on the fact that the main consumer is usually either the UDP
early receive path or the error handler/Timestamp path.
As far as the comments about it not freeing anything take a look at
sock_rfree and tell me what it is actually freeing? Last I knew it is
simply dropping references to memory so that the socket can go ahead and
either make use of them or close itself once all references have been
freed. That is the same thing this does, but it is holding references
to a single byte of sk_wmem_alloc.
Patches are always welcome if you believe we need additional
documentation, I had assumed this was pretty straight forward since the
function was essentially just a wrapper for sock_put so I could use it
as a destructor.
- Alex
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2015-03-10 15:19 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <1425959300-27132-1-git-send-email-fw@strlen.de>
2015-03-10 6:14 ` [PATCH] can: use sock_efree instead of own destructor Oliver Hartkopp
2015-03-10 12:22 ` Eric Dumazet
2015-03-10 13:36 ` Oliver Hartkopp
2015-03-10 14:36 ` Eric Dumazet
2015-03-10 14:55 ` Oliver Hartkopp
2015-03-10 15:19 ` Alexander Duyck
2015-03-10 7:50 ` Marc Kleine-Budde
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).