At 2020-10-16 08:53:00, "Hu, Jiayu" <[email protected]> wrote: > > >> -----Original Message----- >> From: Ananyev, Konstantin <[email protected]> >> Sent: Friday, October 16, 2020 12:16 AM >> To: Hu, Jiayu <[email protected]>; yang_y_yi <[email protected]> >> Cc: [email protected]; [email protected]; [email protected]; >> [email protected] >> Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments attach >> to >> >> >> >> > >> > > -----Original Message----- >> > > From: Ananyev, Konstantin <[email protected]> >> > > Sent: Wednesday, October 14, 2020 8:06 PM >> > > To: yang_y_yi <[email protected]>; Hu, Jiayu <[email protected]> >> > > Cc: [email protected]; [email protected]; [email protected]; >> > > [email protected] >> > > Subject: RE: Re:RE: [PATCH] gso: fix free issue of mbuf gso segments >> attach to >> > > >> > > >> > > > From: yang_y_yi <[email protected]> >> > > > Sent: Wednesday, October 14, 2020 3:56 AM >> > > > To: Hu, Jiayu <[email protected]> >> > > > Cc: Ananyev, Konstantin <[email protected]>; >> [email protected]; >> > > [email protected]; [email protected]; >> > > > [email protected] >> > > > Subject: 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; > } > >Thanks, >Jiayu >> >> >>
Jiayu, please help commit the patch you pasted if you think it is ok. I need to update my GSO patch based on this fix, thanks a lot.
