Hi Ying, I'll respond to this one, since I initiated it. See below. > -----Original Message----- > From: Ying Xue [mailto:[email protected]] > Sent: Tuesday, April 24, 2018 08:22 > To: Tung Quang Nguyen <[email protected]>; tipc- > [email protected] > Subject: Re: [tipc-discussion] tipc: remove skb_clone() in function > tipc_msg_extract() > > First of all, please submit your patch with git send email command. > > On 04/23/2018 11:16 AM, tung quang nguyen wrote: > > Function tipc_msg_extract() is using skb_clone() to clone > > > > inner messages from bundle message. Although it is safe to > > > > use this method, it has an undesired effect that each clone > > > > inherits the true-size of the original buffer. As a result, > > > > the clone almost always has to be copied by the message > > > > validation function. > > Secondly, I cannot understand who is the message validation function. > Can you please give us a bit detailed description about this?
msg.c::tipc_msg_validate(). This one copies the buffer if the ratio skb->truesize / msg_size() >= 4, since fulfilling that condition is a prerequisite for the flow control mechanism to be safe. > > > > > [...] > > > > + > > > > + skb_copy_to_linear_data(*iskb, ihdr, imsz); > > In skb_clone(), only skb header is copied, by contrast, in your proposal, we > have to copy the entire inner message to a new skb buffer. > From performance perspective, our performance will be worse. > > Can you please help me understand how your proposal is better than the > current method? This came as result of my attempt to fix a flow control problem with extracted buffer clones: They inherit the truesize value of the base buffer, meaning that if you extract an e.g., 40 byte message from a 1500 byte buffer (which normally has a truesize of 2304 bytes) even the clone has truesize of 2304 bytes. 2304 /40 = 576, which grossly violates the < 4 condition for flow control. I tried to fix this by doing a "smart" adjustment of the truesize value of the clone, but my patch was flatly rejected by David Miller, and the whole approach of 'abusing' the skc_clone() feature the way we do was criticized also by Eric Dumazet. So, if we end up with having to do skb_expand() anyway on almost all extracted buffers, we can just as well simplify this function and do the copying up front. It might cost some performance, but I am not even sure about that. ///jon > > Thanks, > Ying > > > > > if (unlikely(!tipc_msg_validate(iskb))) > > > > goto none; > > > > + > > > > *pos += align(imsz); > > > > return true; > > > > none: > > > > @@ -531,12 +536,6 @@ bool tipc_msg_reverse(u32 own_node, struct > > sk_buff **skb, int err) > > > > msg_set_hdr_sz(hdr, BASIC_H_SIZE); > > > > } > > > > > > > > - if (skb_cloned(_skb) && > > > > - pskb_expand_head(_skb, BUF_HEADROOM, BUF_TAILROOM, > GFP_ATOMIC)) > > > > - goto exit; > > > > - > > > > - /* reassign after skb header modifications */ > > > > - hdr = buf_msg(_skb); > > > > /* Now reverse the concerned fields */ > > > > msg_set_errcode(hdr, err); > > > > msg_set_non_seq(hdr, 0); > > > > @@ -595,10 +594,6 @@ bool tipc_msg_lookup_dest(struct net *net, struct > > sk_buff *skb, int *err) > > > > if (!skb_cloned(skb)) > > > > return true; > > > > > > > > - /* Unclone buffer in case it was bundled */ > > > > - if (pskb_expand_head(skb, BUF_HEADROOM, BUF_TAILROOM, > GFP_ATOMIC)) > > > > - return false; > > > > - > > > > return true; > > > > } > > > > > > > > -- > > > > 2.1.4 > > > > > > > > -- > > > > Best regards, > > > > Tung Nguyen > > > > > > > > ---------------------------------------------------------------------- > > -------- Check out the vibrant tech community on one of the world's > > most engaging tech sites, Slashdot.org! http://sdm.link/slashdot > > _______________________________________________ > > tipc-discussion mailing list > > [email protected] > > https://lists.sourceforge.net/lists/listinfo/tipc-discussion > > > > ------------------------------------------------------------------------------ > Check out the vibrant tech community on one of the world's most engaging > tech sites, Slashdot.org! http://sdm.link/slashdot > _______________________________________________ > tipc-discussion mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/tipc-discussion ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot _______________________________________________ tipc-discussion mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/tipc-discussion
