From: "Hu, Jiayu" <jiayu.hu@intel.com> To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, yang_y_yi <yang_y_yi@163.com> Cc: "dev@dpdk.org" <dev@dpdk.org>, "olivier.matz@6wind.com" <olivier.matz@6wind.com>, "thomas@monjalon.net" <thomas@monjalon.net>, "yangyi01@inspur.com" <yangyi01@inspur.com> Subject: Re: [dpdk-dev] [PATCH] gso: fix free issue of mbuf gso segments attach to Date: Mon, 19 Oct 2020 03:17:48 +0000 Message-ID: <e3521bc642414b87b937cdd801d9597a@intel.com> (raw) In-Reply-To: <BYAPR11MB3301D13C95E1E43AFEB203979A030@BYAPR11MB3301.namprd11.prod.outlook.com> > -----Original Message----- > From: Ananyev, Konstantin <konstantin.ananyev@intel.com> > Sent: Friday, October 16, 2020 4:31 PM > To: Hu, Jiayu <jiayu.hu@intel.com>; yang_y_yi <yang_y_yi@163.com> > Cc: dev@dpdk.org; olivier.matz@6wind.com; thomas@monjalon.net; > yangyi01@inspur.com > Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach to > > > > > > > > > > > > > > > > > I think it isn't a good idea to free it in rte_gso_segment, maybe > > > application > > > > > will continue to use this pkt for other purpose, rte_gso_segment > > > > > > can't make decision for application without any notice, it is better to > > > return > > > > > this decision right backt to application. > > > > > > > > > > > > > > > > I think, if user wants to keep original packet, he can always call > > > > > rte_pktmbuf_refcnt_update(pkt, 1) > > > > > just before calling gso function. > > > > > > > > > > Also, as I remember in some cases it is not safe to do free() for input > > > packet > > > > > (as pkt_out[] can contain input pkt itself). Would it also be user > > > responsibility > > > > > to determine > > > > > such situations? > > > > > > > > In what case will pkt_out[] contain the input pkt? Can you give an > example? > > > > > > As I can read the code, whenever gso code decides that > > > no segmentation is not really needed, or it is not capable > > > of doing it properly. > > > Let say: > > > > > > gso_tcp4_segment(struct rte_mbuf *pkt, > > > uint16_t gso_size, > > > uint8_t ipid_delta, > > > struct rte_mempool *direct_pool, > > > struct rte_mempool *indirect_pool, > > > struct rte_mbuf **pkts_out, > > > uint16_t nb_pkts_out) > > > { > > > ... > > > /* Don't process the fragmented packet */ > > > ipv4_hdr = (struct rte_ipv4_hdr *)(rte_pktmbuf_mtod(pkt, char *) + > > > pkt->l2_len); > > > frag_off = rte_be_to_cpu_16(ipv4_hdr->fragment_offset); > > > if (unlikely(IS_FRAGMENTED(frag_off))) { > > > pkts_out[0] = pkt; > > > return 1; > > > } > > > > > > /* Don't process the packet without data */ > > > hdr_offset = pkt->l2_len + pkt->l3_len + pkt->l4_len; > > > if (unlikely(hdr_offset >= pkt->pkt_len)) { > > > pkts_out[0] = pkt; > > > return 1; > > > } > > > > > > That's why in rte_gso_segment() we update refcnt only when ret > 1. > > > > But in these cases, the value of ret is 1. So we can free input pkt only when > > ret > 1. Like: > > > > - if (ret > 1) { > > - pkt_seg = pkt; > > - while (pkt_seg) { > > - rte_mbuf_refcnt_update(pkt_seg, -1); > > - pkt_seg = pkt_seg->next; > > - } > > - } else if (ret < 0) { > > + if (ret > 1) > > + rte_pktmbuf_free(pkt); > > + else if (ret < 0) { > > /* Revert the ol_flags in the event of failure. */ > > pkt->ol_flags = ol_flags; > > } > > Yes, definitely. I am not arguing about that. > My question was to the author of the original patch: > He suggests not to free input packet inside gso function and leave it to the > user. > So, in his proposition, would it also become user responsibility to determine > when input packet can be freed (it is not present in pkt_out[]) or not? @Yi, I am OK with the both designs. If you think it's better to free the input pkt by users, you can keep the original design. But one thing to notice is that you need to update definition of rte_gso_segment() in rte_gso.h too. Thanks, Jiayu > > > > > Thanks, > > Jiayu > > > > > > > > > > > >
next prev parent reply other threads:[~2020-10-19 3:17 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-10-10 3:10 yang_y_yi 2020-10-13 7:28 ` Hu, Jiayu 2020-10-13 15:39 ` Ananyev, Konstantin 2020-10-14 1:00 ` Hu, Jiayu 2020-10-14 2:56 ` yang_y_yi 2020-10-14 12:05 ` Ananyev, Konstantin 2020-10-15 5:14 ` Hu, Jiayu 2020-10-15 16:16 ` Ananyev, Konstantin 2020-10-16 0:53 ` Hu, Jiayu 2020-10-16 8:31 ` Ananyev, Konstantin 2020-10-19 3:17 ` Hu, Jiayu [this message] 2020-10-19 6:44 ` yang_y_yi 2020-10-19 8:47 ` Ananyev, Konstantin 2020-10-20 1:16 ` yang_y_yi 2020-10-19 2:29 ` yang_y_yi 2020-10-19 2:20 ` yang_y_yi
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=e3521bc642414b87b937cdd801d9597a@intel.com \ --to=jiayu.hu@intel.com \ --cc=dev@dpdk.org \ --cc=konstantin.ananyev@intel.com \ --cc=olivier.matz@6wind.com \ --cc=thomas@monjalon.net \ --cc=yang_y_yi@163.com \ --cc=yangyi01@inspur.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
DPDK patches and discussions This inbox may be cloned and mirrored by anyone: git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \ dev@dpdk.org public-inbox-index dev Example config snippet for mirrors. Newsgroup available over NNTP: nntp://inbox.dpdk.org/inbox.dpdk.dev AGPL code for this site: git clone https://public-inbox.org/public-inbox.git