Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On 28.11.2019 14:42, Kevin Traynor wrote: > On 27/11/2019 20:33, David Marchand wrote: >> On Thu, Nov 21, 2019 at 3:35 PM Ilya Maximets wrote: >>> >>> On 21.11.2019 14:58, Ilya Maximets wrote: 'ol_flags' of DPDK mbuf could contain bits responsible for external or indirect buffers which are not actually offload flags in a common sense. Clearing/copying of these flags could lead to memory leaks of external memory chunks and crashes due to access to wrong memory. OVS should not clear these flags while resetting offloads and also should not copy them to the newly allocated packets. This change is required to support DPDK 19.11, as some drivers may return mbufs with these flags set. However, it might be good to do the same for DPDK 18.11, because these flags are present and should be taken into account. Signed-off-by: Ilya Maximets >> >> As discussed during the OVS-DPDK public meeting earlier, I am ok with >> your suggestion of going with this patch as a first step. >> We will have to keep an eye on new flags on the dpdk side. >> >> Then once the dpdk ring ports have been dropped, we can revisit this. >> >> Reviewed-by: David Marchand >> > > +1 > > Acked-by: Kevin Traynor Thanks! Applied to master and branch-2.12. OVS 2.11 uses DPDK 18.11 too, but it seems that we're not clearing flags on that branch. Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On 27/11/2019 20:33, David Marchand wrote:
> On Thu, Nov 21, 2019 at 3:35 PM Ilya Maximets wrote:
>>
>> On 21.11.2019 14:58, Ilya Maximets wrote:
>>> 'ol_flags' of DPDK mbuf could contain bits responsible for external
>>> or indirect buffers which are not actually offload flags in a common
>>> sense. Clearing/copying of these flags could lead to memory leaks of
>>> external memory chunks and crashes due to access to wrong memory.
>>>
>>> OVS should not clear these flags while resetting offloads and also
>>> should not copy them to the newly allocated packets.
>>>
>>> This change is required to support DPDK 19.11, as some drivers may
>>> return mbufs with these flags set. However, it might be good to do
>>> the same for DPDK 18.11, because these flags are present and should
>>> be taken into account.
>>>
>>> Signed-off-by: Ilya Maximets
>
> As discussed during the OVS-DPDK public meeting earlier, I am ok with
> your suggestion of going with this patch as a first step.
> We will have to keep an eye on new flags on the dpdk side.
>
> Then once the dpdk ring ports have been dropped, we can revisit this.
>
> Reviewed-by: David Marchand
>
+1
Acked-by: Kevin Traynor
>
>> We could also add the following tag:
>> Fixes: 03f3f9c0faf8 ("dpdk: Update to use DPDK 18.11.")
>
> Yes.
>
> Thanks.
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On Thu, Nov 21, 2019 at 3:35 PM Ilya Maximets wrote:
>
> On 21.11.2019 14:58, Ilya Maximets wrote:
> > 'ol_flags' of DPDK mbuf could contain bits responsible for external
> > or indirect buffers which are not actually offload flags in a common
> > sense. Clearing/copying of these flags could lead to memory leaks of
> > external memory chunks and crashes due to access to wrong memory.
> >
> > OVS should not clear these flags while resetting offloads and also
> > should not copy them to the newly allocated packets.
> >
> > This change is required to support DPDK 19.11, as some drivers may
> > return mbufs with these flags set. However, it might be good to do
> > the same for DPDK 18.11, because these flags are present and should
> > be taken into account.
> >
> > Signed-off-by: Ilya Maximets
As discussed during the OVS-DPDK public meeting earlier, I am ok with
your suggestion of going with this patch as a first step.
We will have to keep an eye on new flags on the dpdk side.
Then once the dpdk ring ports have been dropped, we can revisit this.
Reviewed-by: David Marchand
> We could also add the following tag:
> Fixes: 03f3f9c0faf8 ("dpdk: Update to use DPDK 18.11.")
Yes.
Thanks.
--
David Marchand
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On Mon, Nov 25, 2019 at 12:23 PM Ilya Maximets wrote: > > On 24.11.2019 21:38, David Marchand wrote: > > On Fri, Nov 22, 2019 at 3:04 PM Ilya Maximets wrote: > > Rather than exclude a set of flags, I would touch/copy only the flags > > that OVS uses/understands. > > > > When OVS uses a DPDK flag, it has an associated API and meaning wrt > > the rte_mbuf and the data. > > - when the flags are reset in OVS, that's because something has been > > done to the data (and the checksums and the rss hash must be > > reevaluated), > > - when a buffer is copied, OVS copies the data and the context that > > matters to OVS, > > > > Anything else should be discarded when copying to a new dp-packet. > > Yes, that was the first version I wanted to implement, i.e. to collect > all the flags that OVS really uses and clear/copy only these flags. > > But then I started to think about 'ring' ports and that we might send > mbufs with incorrect flags set after the packet modification to the > external application. This doesn't sound good. Because if OVS doesn't > use them doesn't mean that other applications doesn't. So, I've end up > with the current logic with clearing everything except the memory layout > flags. > > Does it make sense? What do you think? > >>> > >>> Before sending a packet through a dpdk ring, OVS will touch the data > >>> and update the relevant flags as far as it is concerned. > >>> > >>> The application can read/touch those mbuf flags as long as it complies > >>> with the DPDK APIs, this concerns both the flags that OVS is aware of, > >>> and the rest. > >>> So when getting this mbuf back, the flags should be valid. > >>> > >>> After this, OVS can do what it wants on the packet, it will only touch > >>> the flags it understands. > >>> > >>> What am I missing? > >> > >> I agree that OVS itself will work OK by only considering flags it > >> really uses. I'm concerned about the second application that receives > >> mbuf from the OVS via ring port. For example, I see that ixgbe driver > >> almost unconditionally parses vlans and sets PKT_RX_VLAN along with > >> mbuf->vlan_tci. If OVS will not clear this flag while sending to > > > > - Well, as I said, if OVS touches the data (pops a vlan), it must > > update the flags (PKT_RX_VLAN). > > I don't think that DPDK API requires that anyhow. Otherwise, DPDK > must ensure that RX flags are valid after receiving mbuf from the > DPDK port, but that is not true for ring pmd. Not sure I understand your point. If an offload flag is set, then the metadata and the data must be consistent with it. The ring driver is just a "passe-plat". If packets received from a ring driver have wrong flags, it means they were wrong when pushed to it. And hopefully, DPDK drivers provide valid offload flags in mbufs. > > This is overkill if the mbuf does not leave OVS, so it could be > > cleared unconditionally before jumping in the ring. > > That is unclear why we should act differently for ring ports if we > could just clear everything always and there is already generic > API for that (dp_packet_reset_offload). You are right, it should be generic. > > Asking, since we know that the multiprocess stuff is not really that > > robust and/or usable with OVS. > > I had an intention to deprecate ring ports in OVS, firstly because of > the poor level of support and testing, but I'm not sure if anyone uses > them or not. I could prepare the deprecation patch to collect some > opinions. > > BTW, I'm not sure if I ever used ring ports in OVS, maybe once for ivshmem. > And I'm not sure if anyone tests them any regularly (this is highly > unlikely since ring tests are not included even in VSPERF). +1 -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On 25.11.2019 12:23, Ilya Maximets wrote: > On 24.11.2019 21:38, David Marchand wrote: >> On Fri, Nov 22, 2019 at 3:04 PM Ilya Maximets wrote: >> Rather than exclude a set of flags, I would touch/copy only the flags >> that OVS uses/understands. >> >> When OVS uses a DPDK flag, it has an associated API and meaning wrt >> the rte_mbuf and the data. >> - when the flags are reset in OVS, that's because something has been >> done to the data (and the checksums and the rss hash must be >> reevaluated), >> - when a buffer is copied, OVS copies the data and the context that >> matters to OVS, >> >> Anything else should be discarded when copying to a new dp-packet. > > Yes, that was the first version I wanted to implement, i.e. to collect > all the flags that OVS really uses and clear/copy only these flags. > > But then I started to think about 'ring' ports and that we might send > mbufs with incorrect flags set after the packet modification to the > external application. This doesn't sound good. Because if OVS doesn't > use them doesn't mean that other applications doesn't. So, I've end up > with the current logic with clearing everything except the memory layout > flags. > > Does it make sense? What do you think? Before sending a packet through a dpdk ring, OVS will touch the data and update the relevant flags as far as it is concerned. The application can read/touch those mbuf flags as long as it complies with the DPDK APIs, this concerns both the flags that OVS is aware of, and the rest. So when getting this mbuf back, the flags should be valid. After this, OVS can do what it wants on the packet, it will only touch the flags it understands. What am I missing? >>> >>> I agree that OVS itself will work OK by only considering flags it >>> really uses. I'm concerned about the second application that receives >>> mbuf from the OVS via ring port. For example, I see that ixgbe driver >>> almost unconditionally parses vlans and sets PKT_RX_VLAN along with >>> mbuf->vlan_tci. If OVS will not clear this flag while sending to >> >> - Well, as I said, if OVS touches the data (pops a vlan), it must >> update the flags (PKT_RX_VLAN). > > I don't think that DPDK API requires that anyhow. Otherwise, DPDK s/Otherwise/Contrariwise/ > must ensure that RX flags are valid after receiving mbuf from the > DPDK port, but that is not true for ring pmd. > >> This is overkill if the mbuf does not leave OVS, so it could be >> cleared unconditionally before jumping in the ring. > > That is unclear why we should act differently for ring ports if we > could just clear everything always and there is already generic > API for that (dp_packet_reset_offload). > >> - What kind of applications can read those rings? >> Are those dpdk secondary applications? > > I think so. > >> Asking, since we know that the multiprocess stuff is not really that >> robust and/or usable with OVS. > > I had an intention to deprecate ring ports in OVS, firstly because of > the poor level of support and testing, but I'm not sure if anyone uses > them or not. I could prepare the deprecation patch to collect some > opinions. > > BTW, I'm not sure if I ever used ring ports in OVS, maybe once for ivshmem. > And I'm not sure if anyone tests them any regularly (this is highly > unlikely since ring tests are not included even in VSPERF). > > Best regards, Ilya Maximets. > ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On 24.11.2019 21:38, David Marchand wrote: > On Fri, Nov 22, 2019 at 3:04 PM Ilya Maximets wrote: > Rather than exclude a set of flags, I would touch/copy only the flags > that OVS uses/understands. > > When OVS uses a DPDK flag, it has an associated API and meaning wrt > the rte_mbuf and the data. > - when the flags are reset in OVS, that's because something has been > done to the data (and the checksums and the rss hash must be > reevaluated), > - when a buffer is copied, OVS copies the data and the context that > matters to OVS, > > Anything else should be discarded when copying to a new dp-packet. Yes, that was the first version I wanted to implement, i.e. to collect all the flags that OVS really uses and clear/copy only these flags. But then I started to think about 'ring' ports and that we might send mbufs with incorrect flags set after the packet modification to the external application. This doesn't sound good. Because if OVS doesn't use them doesn't mean that other applications doesn't. So, I've end up with the current logic with clearing everything except the memory layout flags. Does it make sense? What do you think? >>> >>> Before sending a packet through a dpdk ring, OVS will touch the data >>> and update the relevant flags as far as it is concerned. >>> >>> The application can read/touch those mbuf flags as long as it complies >>> with the DPDK APIs, this concerns both the flags that OVS is aware of, >>> and the rest. >>> So when getting this mbuf back, the flags should be valid. >>> >>> After this, OVS can do what it wants on the packet, it will only touch >>> the flags it understands. >>> >>> What am I missing? >> >> I agree that OVS itself will work OK by only considering flags it >> really uses. I'm concerned about the second application that receives >> mbuf from the OVS via ring port. For example, I see that ixgbe driver >> almost unconditionally parses vlans and sets PKT_RX_VLAN along with >> mbuf->vlan_tci. If OVS will not clear this flag while sending to > > - Well, as I said, if OVS touches the data (pops a vlan), it must > update the flags (PKT_RX_VLAN). I don't think that DPDK API requires that anyhow. Otherwise, DPDK must ensure that RX flags are valid after receiving mbuf from the DPDK port, but that is not true for ring pmd. > This is overkill if the mbuf does not leave OVS, so it could be > cleared unconditionally before jumping in the ring. That is unclear why we should act differently for ring ports if we could just clear everything always and there is already generic API for that (dp_packet_reset_offload). > - What kind of applications can read those rings? > Are those dpdk secondary applications? I think so. > Asking, since we know that the multiprocess stuff is not really that > robust and/or usable with OVS. I had an intention to deprecate ring ports in OVS, firstly because of the poor level of support and testing, but I'm not sure if anyone uses them or not. I could prepare the deprecation patch to collect some opinions. BTW, I'm not sure if I ever used ring ports in OVS, maybe once for ivshmem. And I'm not sure if anyone tests them any regularly (this is highly unlikely since ring tests are not included even in VSPERF). Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On Fri, Nov 22, 2019 at 3:04 PM Ilya Maximets wrote: > >>> Rather than exclude a set of flags, I would touch/copy only the flags > >>> that OVS uses/understands. > >>> > >>> When OVS uses a DPDK flag, it has an associated API and meaning wrt > >>> the rte_mbuf and the data. > >>> - when the flags are reset in OVS, that's because something has been > >>> done to the data (and the checksums and the rss hash must be > >>> reevaluated), > >>> - when a buffer is copied, OVS copies the data and the context that > >>> matters to OVS, > >>> > >>> Anything else should be discarded when copying to a new dp-packet. > >> > >> Yes, that was the first version I wanted to implement, i.e. to collect > >> all the flags that OVS really uses and clear/copy only these flags. > >> > >> But then I started to think about 'ring' ports and that we might send > >> mbufs with incorrect flags set after the packet modification to the > >> external application. This doesn't sound good. Because if OVS doesn't > >> use them doesn't mean that other applications doesn't. So, I've end up > >> with the current logic with clearing everything except the memory layout > >> flags. > >> > >> Does it make sense? What do you think? > > > > Before sending a packet through a dpdk ring, OVS will touch the data > > and update the relevant flags as far as it is concerned. > > > > The application can read/touch those mbuf flags as long as it complies > > with the DPDK APIs, this concerns both the flags that OVS is aware of, > > and the rest. > > So when getting this mbuf back, the flags should be valid. > > > > After this, OVS can do what it wants on the packet, it will only touch > > the flags it understands. > > > > What am I missing? > > I agree that OVS itself will work OK by only considering flags it > really uses. I'm concerned about the second application that receives > mbuf from the OVS via ring port. For example, I see that ixgbe driver > almost unconditionally parses vlans and sets PKT_RX_VLAN along with > mbuf->vlan_tci. If OVS will not clear this flag while sending to - Well, as I said, if OVS touches the data (pops a vlan), it must update the flags (PKT_RX_VLAN). This is overkill if the mbuf does not leave OVS, so it could be cleared unconditionally before jumping in the ring. - What kind of applications can read those rings? Are those dpdk secondary applications? Asking, since we know that the multiprocess stuff is not really that robust and/or usable with OVS. -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On 22.11.2019 13:27, David Marchand wrote: > On Fri, Nov 22, 2019 at 1:15 PM Ilya Maximets wrote: >> >> On 22.11.2019 12:42, David Marchand wrote: >>> On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets wrote: 'ol_flags' of DPDK mbuf could contain bits responsible for external or indirect buffers which are not actually offload flags in a common sense. Clearing/copying of these flags could lead to memory leaks of external memory chunks and crashes due to access to wrong memory. OVS should not clear these flags while resetting offloads and also should not copy them to the newly allocated packets. This change is required to support DPDK 19.11, as some drivers may return mbufs with these flags set. However, it might be good to do the same for DPDK 18.11, because these flags are present and should be taken into account. >>> >>> Did you consider inverting the logic? >>> >>> Rather than exclude a set of flags, I would touch/copy only the flags >>> that OVS uses/understands. >>> >>> When OVS uses a DPDK flag, it has an associated API and meaning wrt >>> the rte_mbuf and the data. >>> - when the flags are reset in OVS, that's because something has been >>> done to the data (and the checksums and the rss hash must be >>> reevaluated), >>> - when a buffer is copied, OVS copies the data and the context that >>> matters to OVS, >>> >>> Anything else should be discarded when copying to a new dp-packet. >> >> Yes, that was the first version I wanted to implement, i.e. to collect >> all the flags that OVS really uses and clear/copy only these flags. >> >> But then I started to think about 'ring' ports and that we might send >> mbufs with incorrect flags set after the packet modification to the >> external application. This doesn't sound good. Because if OVS doesn't >> use them doesn't mean that other applications doesn't. So, I've end up >> with the current logic with clearing everything except the memory layout >> flags. >> >> Does it make sense? What do you think? > > Before sending a packet through a dpdk ring, OVS will touch the data > and update the relevant flags as far as it is concerned. > > The application can read/touch those mbuf flags as long as it complies > with the DPDK APIs, this concerns both the flags that OVS is aware of, > and the rest. > So when getting this mbuf back, the flags should be valid. > > After this, OVS can do what it wants on the packet, it will only touch > the flags it understands. > > What am I missing? I agree that OVS itself will work OK by only considering flags it really uses. I'm concerned about the second application that receives mbuf from the OVS via ring port. For example, I see that ixgbe driver almost unconditionally parses vlans and sets PKT_RX_VLAN along with mbuf->vlan_tci. If OVS will not clear this flag while sending to the ring port, other application that receives that mbuf will think that mbuf contains valid 'vlan_tci', which is not correct, because OVS might modify or strip that vlan from the packet before sending. > > >> BTW, it might be good to have a DPDK API for offload flags clear/get/copy. > > There is a rte_pktmbuf_copy that has been introduced recently, think a > helper copies metadata too... the best is to bother Olivier :-) > > >> >> BTW2, I don't really understand why memory layout flags are in ol_flags >> in the first place. I guess, there was just no room in mbuf structure? >> Is it possible that these flags will become regular or dynamic fields? > > I think it is the reason. > Again, Olivier ? ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On Fri, Nov 22, 2019 at 01:27:57PM +0100, David Marchand wrote:
> On Fri, Nov 22, 2019 at 1:15 PM Ilya Maximets wrote:
> >
> > On 22.11.2019 12:42, David Marchand wrote:
> > > On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets wrote:
> > >>
> > >> 'ol_flags' of DPDK mbuf could contain bits responsible for external
> > >> or indirect buffers which are not actually offload flags in a common
> > >> sense. Clearing/copying of these flags could lead to memory leaks of
> > >> external memory chunks and crashes due to access to wrong memory.
> > >>
> > >> OVS should not clear these flags while resetting offloads and also
> > >> should not copy them to the newly allocated packets.
> > >>
> > >> This change is required to support DPDK 19.11, as some drivers may
> > >> return mbufs with these flags set. However, it might be good to do
> > >> the same for DPDK 18.11, because these flags are present and should
> > >> be taken into account.
> > >
> > > Did you consider inverting the logic?
> > >
> > > Rather than exclude a set of flags, I would touch/copy only the flags
> > > that OVS uses/understands.
> > >
> > > When OVS uses a DPDK flag, it has an associated API and meaning wrt
> > > the rte_mbuf and the data.
> > > - when the flags are reset in OVS, that's because something has been
> > > done to the data (and the checksums and the rss hash must be
> > > reevaluated),
> > > - when a buffer is copied, OVS copies the data and the context that
> > > matters to OVS,
> > >
> > > Anything else should be discarded when copying to a new dp-packet.
> >
> > Yes, that was the first version I wanted to implement, i.e. to collect
> > all the flags that OVS really uses and clear/copy only these flags.
> >
> > But then I started to think about 'ring' ports and that we might send
> > mbufs with incorrect flags set after the packet modification to the
> > external application. This doesn't sound good. Because if OVS doesn't
> > use them doesn't mean that other applications doesn't. So, I've end up
> > with the current logic with clearing everything except the memory layout
> > flags.
> >
> > Does it make sense? What do you think?
>
> Before sending a packet through a dpdk ring, OVS will touch the data
> and update the relevant flags as far as it is concerned.
>
> The application can read/touch those mbuf flags as long as it complies
> with the DPDK APIs, this concerns both the flags that OVS is aware of,
> and the rest.
> So when getting this mbuf back, the flags should be valid.
>
> After this, OVS can do what it wants on the packet, it will only touch
> the flags it understands.
>
> What am I missing?
>
>
> > BTW, it might be good to have a DPDK API for offload flags clear/get/copy.
>
> There is a rte_pktmbuf_copy that has been introduced recently, think a
> helper copies metadata too... the best is to bother Olivier :-)
There is no API for that. Currently, rte_pktmbuf_copy() does:
mc->ol_flags = m->ol_flags & ~(IND_ATTACHED_MBUF|EXT_ATTACHED_MBUF);
> > BTW2, I don't really understand why memory layout flags are in ol_flags
> > in the first place. I guess, there was just no room in mbuf structure?
> > Is it possible that these flags will become regular or dynamic fields?
>
> I think it is the reason.
> Again, Olivier ?
Historically, the ol_flags field was only used for offloads. But since
the introduction of such features, "flags" would have been a better
name. This is something we could do by adding a union { flags; ol_flags; }
and remove ol_flags at next API breakage.
Olivier
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On Fri, Nov 22, 2019 at 1:15 PM Ilya Maximets wrote: > > On 22.11.2019 12:42, David Marchand wrote: > > On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets wrote: > >> > >> 'ol_flags' of DPDK mbuf could contain bits responsible for external > >> or indirect buffers which are not actually offload flags in a common > >> sense. Clearing/copying of these flags could lead to memory leaks of > >> external memory chunks and crashes due to access to wrong memory. > >> > >> OVS should not clear these flags while resetting offloads and also > >> should not copy them to the newly allocated packets. > >> > >> This change is required to support DPDK 19.11, as some drivers may > >> return mbufs with these flags set. However, it might be good to do > >> the same for DPDK 18.11, because these flags are present and should > >> be taken into account. > > > > Did you consider inverting the logic? > > > > Rather than exclude a set of flags, I would touch/copy only the flags > > that OVS uses/understands. > > > > When OVS uses a DPDK flag, it has an associated API and meaning wrt > > the rte_mbuf and the data. > > - when the flags are reset in OVS, that's because something has been > > done to the data (and the checksums and the rss hash must be > > reevaluated), > > - when a buffer is copied, OVS copies the data and the context that > > matters to OVS, > > > > Anything else should be discarded when copying to a new dp-packet. > > Yes, that was the first version I wanted to implement, i.e. to collect > all the flags that OVS really uses and clear/copy only these flags. > > But then I started to think about 'ring' ports and that we might send > mbufs with incorrect flags set after the packet modification to the > external application. This doesn't sound good. Because if OVS doesn't > use them doesn't mean that other applications doesn't. So, I've end up > with the current logic with clearing everything except the memory layout > flags. > > Does it make sense? What do you think? Before sending a packet through a dpdk ring, OVS will touch the data and update the relevant flags as far as it is concerned. The application can read/touch those mbuf flags as long as it complies with the DPDK APIs, this concerns both the flags that OVS is aware of, and the rest. So when getting this mbuf back, the flags should be valid. After this, OVS can do what it wants on the packet, it will only touch the flags it understands. What am I missing? > BTW, it might be good to have a DPDK API for offload flags clear/get/copy. There is a rte_pktmbuf_copy that has been introduced recently, think a helper copies metadata too... the best is to bother Olivier :-) > > BTW2, I don't really understand why memory layout flags are in ol_flags > in the first place. I guess, there was just no room in mbuf structure? > Is it possible that these flags will become regular or dynamic fields? I think it is the reason. Again, Olivier ? -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On 22.11.2019 12:42, David Marchand wrote: > On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets wrote: >> >> 'ol_flags' of DPDK mbuf could contain bits responsible for external >> or indirect buffers which are not actually offload flags in a common >> sense. Clearing/copying of these flags could lead to memory leaks of >> external memory chunks and crashes due to access to wrong memory. >> >> OVS should not clear these flags while resetting offloads and also >> should not copy them to the newly allocated packets. >> >> This change is required to support DPDK 19.11, as some drivers may >> return mbufs with these flags set. However, it might be good to do >> the same for DPDK 18.11, because these flags are present and should >> be taken into account. > > Did you consider inverting the logic? > > Rather than exclude a set of flags, I would touch/copy only the flags > that OVS uses/understands. > > When OVS uses a DPDK flag, it has an associated API and meaning wrt > the rte_mbuf and the data. > - when the flags are reset in OVS, that's because something has been > done to the data (and the checksums and the rss hash must be > reevaluated), > - when a buffer is copied, OVS copies the data and the context that > matters to OVS, > > Anything else should be discarded when copying to a new dp-packet. Yes, that was the first version I wanted to implement, i.e. to collect all the flags that OVS really uses and clear/copy only these flags. But then I started to think about 'ring' ports and that we might send mbufs with incorrect flags set after the packet modification to the external application. This doesn't sound good. Because if OVS doesn't use them doesn't mean that other applications doesn't. So, I've end up with the current logic with clearing everything except the memory layout flags. Does it make sense? What do you think? BTW, it might be good to have a DPDK API for offload flags clear/get/copy. BTW2, I don't really understand why memory layout flags are in ol_flags in the first place. I guess, there was just no room in mbuf structure? Is it possible that these flags will become regular or dynamic fields? Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On Thu, Nov 21, 2019 at 2:59 PM Ilya Maximets wrote: > > 'ol_flags' of DPDK mbuf could contain bits responsible for external > or indirect buffers which are not actually offload flags in a common > sense. Clearing/copying of these flags could lead to memory leaks of > external memory chunks and crashes due to access to wrong memory. > > OVS should not clear these flags while resetting offloads and also > should not copy them to the newly allocated packets. > > This change is required to support DPDK 19.11, as some drivers may > return mbufs with these flags set. However, it might be good to do > the same for DPDK 18.11, because these flags are present and should > be taken into account. Did you consider inverting the logic? Rather than exclude a set of flags, I would touch/copy only the flags that OVS uses/understands. When OVS uses a DPDK flag, it has an associated API and meaning wrt the rte_mbuf and the data. - when the flags are reset in OVS, that's because something has been done to the data (and the checksums and the rss hash must be reevaluated), - when a buffer is copied, OVS copies the data and the context that matters to OVS, Anything else should be discarded when copying to a new dp-packet. -- David Marchand ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On Thu, Nov 21, 2019 at 02:58:31PM +0100, Ilya Maximets wrote: > 'ol_flags' of DPDK mbuf could contain bits responsible for external > or indirect buffers which are not actually offload flags in a common > sense. Clearing/copying of these flags could lead to memory leaks of > external memory chunks and crashes due to access to wrong memory. > > OVS should not clear these flags while resetting offloads and also > should not copy them to the newly allocated packets. > > This change is required to support DPDK 19.11, as some drivers may > return mbufs with these flags set. However, it might be good to do > the same for DPDK 18.11, because these flags are present and should > be taken into account. > > Signed-off-by: Ilya Maximets This seems reasonable to me, but I don't have much context. With that in mind: Acked-by: Ben Pfaff I don't usually like "negative" names for things, like the "non-" infix, but it seems appropriate in this case. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dp-packet: Fix clearing/copying of memory layout flags.
On 21.11.2019 14:58, Ilya Maximets wrote:
> 'ol_flags' of DPDK mbuf could contain bits responsible for external
> or indirect buffers which are not actually offload flags in a common
> sense. Clearing/copying of these flags could lead to memory leaks of
> external memory chunks and crashes due to access to wrong memory.
>
> OVS should not clear these flags while resetting offloads and also
> should not copy them to the newly allocated packets.
>
> This change is required to support DPDK 19.11, as some drivers may
> return mbufs with these flags set. However, it might be good to do
> the same for DPDK 18.11, because these flags are present and should
> be taken into account.
>
> Signed-off-by: Ilya Maximets
We could also add the following tag:
Fixes: 03f3f9c0faf8 ("dpdk: Update to use DPDK 18.11.")
Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
