> -----Original Message----- > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ananyev, > Konstantin > Sent: Friday, October 24, 2014 11:58 PM > To: Richardson, Bruce > Cc: dev at dpdk.org > Subject: Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep > EXTERNAL_MBUF flag > > > > > -----Original Message----- > > From: Richardson, Bruce > > Sent: Friday, October 24, 2014 4:43 PM > > To: Ananyev, Konstantin > > Cc: dev at dpdk.org > > Subject: Re: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep > > EXTERNAL_MBUF flag > > > > On Fri, Oct 24, 2014 at 01:34:58PM +0100, Bruce Richardson wrote: > > > On Fri, Oct 24, 2014 at 10:46:06AM +0000, Ananyev, Konstantin wrote: > > > > Hi Changchun, > > > > > > > > > -----Original Message----- > > > > > From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Ouyang > > > > > Changchun > > > > > Sent: Friday, October 24, 2014 9:10 AM > > > > > To: dev at dpdk.org > > > > > Subject: [dpdk-dev] [PATCH 2/3] pmd: RX function need keep > > > > > EXTERNAL_MBUF flag > > > > > > > > > > Every pmd RX function need keep the EXTERNAL_MBUF flag in > > > > > mbuf.ol_flags, and can't overwrite it when filling ol_flags from > > > > > descriptor to mbuf, otherwise, it probably cause to crash when > > > > > freeing a mbuf and trying to freeing its attached external buffer, > > > > > say, > from guest space. > > > > > > > > > > > > > Don't really like the idea to put: > > > > mb->ol_flags = pkt_flags | (mb->ol_flags & EXTERNAL_MBUF); > > > > in each and every PMD from now on... > > > > > > > > From other side, it is probably not very good that RX functions update > whole ol_flags, not only RX related part. > > > > Wonder can we reserve low 32bits of ol_flags for RX, and high 32bits for > TX and generic stuff. > > > > So our ol_flags will look something like that: > > > > > > > > union { > > > > uint64_t ol_raw_flags; > > > > struct { > > > > uint32_t rx; > > > > uint32_t gen_tx; > > > > } ol_flags > > > > }; > > > > > > > > And make all PMD RX functions to operate on rx part of the flags only: > > > > mb->ol_flags.rx = pkt_flags; > > > > ? > > > > > > > > Konstantin > > > > > > > I would tend to agree with this. Changchun, did you get to assess > > > the performance impact of making this change to the PMDs? I suspect > > > that making the changes to each PMD would impact performance, while > > > Konstantin's suggestion should eliminate that impact. > > > The downside there is that we are limiting the flexibility we have > > > in expanding beyond 32 RX flags and 24 TX flags. :-( > > > > > > /Bruce > > > > > > > How about switching things about in terms of the flag. Instead of > > having to manage a flag across the baord to indicate if an mbuf is > > pointing to external memory, I think we should use the flag to > > indicate that an mbuf is attached to the memory space of another mbuf. > > > > My reasons for suggesting this are: > > 1. Mbufs pointing to externally managed memory are not really the > > problem to be dealt with on free, since they can be handled the same > > as mbufs with the data pointer pointing internally, it's mbufs > > attached to other mbufs which are - so that's what we need to track using a > flag. > > 2. Setting the flag to indicate an indirect mbuf should have no impact > > on the driver, as an mbuf that has just been allocated from mempool > > cannot be an indirect one. > > 3. The only place we would need to worry about such a flag is in the > > attach, detach and free mbuf functions - and on free we would simply > > need to replace the existing check for "md != m" with a new check for > > the new flag. It would be a contained change. > > > > Sounds good to me. > That's' definitely much better than my proposal. > Plus, if we'll stop to rely on: > > md = RTE_MBUF_FROM_BADDR(m->buf_addr); > if (unlikely (md != m)) { >
Currently seems good to me, too. But need more practice on it. > That will allow us to set buf_addr to some other valid offset inside mbuf and > that fix an old problem with mbufs extra metadata (userdata) stored in the > packet's headroom. > Not fully understand this. Konstantin, would you explain more? Thanks Changchun
