[ovs-dev] ipf: Cancel fragment pkt copy.

From: fang <fangjiannan@cmss.chinamobile.com>

Canceling packet copying can better improve the performance of capturing fragmented packets. In 640d4db, pkt copying was added to fix the crash, but this bug has been fixed in 7e6b41a, so there is no need to copy the pkt again.

Signed-off-by: fangjiannan <fangjiannan@cmss.chinamobile.com>
---
 lib/ipf.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

On Fri, Dec 13, 2024 at 4:07 AM fangjiannan <evanfjn@163.com> wrote:
>
> From: fang <fangjiannan@cmss.chinamobile.com>
>
> Canceling packet copying can better improve the performance of capturing fragmented packets. In 640d4db, pkt copying was added to fix the crash, but this bug has been fixed in 7e6b41a, so there is no need to copy the pkt again.
>
> Signed-off-by: fangjiannan <fangjiannan@cmss.chinamobile.com>

The CI is looking for an exact match with the from line, eg:
Signed-off-by: fang <fangjiannan@cmss.chinamobile.com>

But I think the sign off tag and commit message line wrap can just be
corrected as the patch is applied.

Code still looks good.

Acked-by: Mike Pattrick <mkp@redhat.com>


> ---
>  lib/ipf.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 2d715f5e9..59e232355 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -820,7 +820,7 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
>               * recommend not setting the mempool number of buffers too low
>               * and also clamp the number of fragments. */
>              struct ipf_frag *frag = &ipf_list->frag_list[last_inuse_idx + 1];
> -            frag->pkt = dp_packet_clone(pkt);
> +            frag->pkt = pkt;
>              frag->start_data_byte = start_data_byte;
>              frag->end_data_byte = end_data_byte;
>              ipf_list->last_inuse_idx++;
> @@ -959,7 +959,6 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct dp_packet_batch *pb,
>                      dp_packet_batch_refill(pb, rp->pkt, pb_idx);
>                      rp->list->reass_execute_ctx = rp->pkt;
>                  }
> -                dp_packet_delete(pkt);
>              }
>              ovs_mutex_unlock(&ipf->ipf_lock);
>          } else {
> --
> 2.42.0.windows.2
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

fangjiannan <evanfjn@163.com> writes:

> From: fang <fangjiannan@cmss.chinamobile.com>
>
> Canceling packet copying can better improve the performance of
> capturing fragmented packets. In 640d4db, pkt copying was added to fix
> the crash, but this bug has been fixed in 7e6b41a, so there is no need
> to copy the pkt again.
>
> Signed-off-by: fangjiannan <fangjiannan@cmss.chinamobile.com>
> ---

I'm not sure about this.  Currently, there are multiple ways for a
packet to be deleted during processing, and not all of them will
sit inline with the ipf handling, nor will IPF be aware of them.  IPF
was changed in 640d4db to ensure that it will always be the only owner
of the packet buffer in question.

It's been some time since I looked at the out-of-band release points
that IPF could still reference a packet on.  Actually, I think they
should mostly be eliminated so maybe I'm just carrying FUD from the
older IPF design.  Did you test this with DPDK ports?  And with long
running CT connections?  Or with frags that would unexpectedly expire?
Can you please give some testing details.

>  lib/ipf.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/lib/ipf.c b/lib/ipf.c
> index 2d715f5e9..59e232355 100644
> --- a/lib/ipf.c
> +++ b/lib/ipf.c
> @@ -820,7 +820,7 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
>               * recommend not setting the mempool number of buffers too low
>               * and also clamp the number of fragments. */
>              struct ipf_frag *frag = &ipf_list->frag_list[last_inuse_idx + 1];
> -            frag->pkt = dp_packet_clone(pkt);
> +            frag->pkt = pkt;
>              frag->start_data_byte = start_data_byte;
>              frag->end_data_byte = end_data_byte;
>              ipf_list->last_inuse_idx++;
> @@ -959,7 +959,6 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct dp_packet_batch *pb,
>                      dp_packet_batch_refill(pb, rp->pkt, pb_idx);
>                      rp->list->reass_execute_ctx = rp->pkt;
>                  }
> -                dp_packet_delete(pkt);
>              }
>              ovs_mutex_unlock(&ipf->ipf_lock);
>          } else {

Mike Pattrick <mkp@redhat.com> writes:

> On Fri, Dec 13, 2024 at 4:07 AM fangjiannan <evanfjn@163.com> wrote:
>>
>> From: fang <fangjiannan@cmss.chinamobile.com>
>>
>> Canceling packet copying can better improve the performance of
>> capturing fragmented packets. In 640d4db, pkt copying was added to
>> fix the crash, but this bug has been fixed in 7e6b41a, so there is
>> no need to copy the pkt again.
>>
>> Signed-off-by: fangjiannan <fangjiannan@cmss.chinamobile.com>
>
> The CI is looking for an exact match with the from line, eg:
> Signed-off-by: fang <fangjiannan@cmss.chinamobile.com>
>
> But I think the sign off tag and commit message line wrap can just be
> corrected as the patch is applied.

Yes, we can fix it up on apply.

> Code still looks good.
>
> Acked-by: Mike Pattrick <mkp@redhat.com>
>
>
>> ---
>>  lib/ipf.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/lib/ipf.c b/lib/ipf.c
>> index 2d715f5e9..59e232355 100644
>> --- a/lib/ipf.c
>> +++ b/lib/ipf.c
>> @@ -820,7 +820,7 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
>>               * recommend not setting the mempool number of buffers too low
>>               * and also clamp the number of fragments. */
>>              struct ipf_frag *frag = &ipf_list->frag_list[last_inuse_idx + 1];
>> -            frag->pkt = dp_packet_clone(pkt);
>> +            frag->pkt = pkt;
>>              frag->start_data_byte = start_data_byte;
>>              frag->end_data_byte = end_data_byte;
>>              ipf_list->last_inuse_idx++;
>> @@ -959,7 +959,6 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct dp_packet_batch *pb,
>>                      dp_packet_batch_refill(pb, rp->pkt, pb_idx);
>>                      rp->list->reass_execute_ctx = rp->pkt;
>>                  }
>> -                dp_packet_delete(pkt);
>>              }
>>              ovs_mutex_unlock(&ipf->ipf_lock);
>>          } else {
>> --
>> 2.42.0.windows.2
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Aaron Conole <aconole@redhat.com> writes:

> fangjiannan <evanfjn@163.com> writes:
>
>> From: fang <fangjiannan@cmss.chinamobile.com>
>>
>> Canceling packet copying can better improve the performance of
>> capturing fragmented packets. In 640d4db, pkt copying was added to fix
>> the crash, but this bug has been fixed in 7e6b41a, so there is no need
>> to copy the pkt again.
>>
>> Signed-off-by: fangjiannan <fangjiannan@cmss.chinamobile.com>
>> ---
>
> I'm not sure about this.  Currently, there are multiple ways for a
> packet to be deleted during processing, and not all of them will
> sit inline with the ipf handling, nor will IPF be aware of them.  IPF
> was changed in 640d4db to ensure that it will always be the only owner
> of the packet buffer in question.
>
> It's been some time since I looked at the out-of-band release points
> that IPF could still reference a packet on.  Actually, I think they
> should mostly be eliminated so maybe I'm just carrying FUD from the
> older IPF design.  Did you test this with DPDK ports?  And with long
> running CT connections?  Or with frags that would unexpectedly expire?
> Can you please give some testing details.

I'm going to contradict myself here.  I think it's okay.  We are assumed
to take ownership in all cases.  If we didn't, there would be a dangling
uaf as in the case of an unhandled frag we keep it on the batch, and the
case of reassembled, we refill the batch, and otherwise we will delete.
In all those cases, the ownership is quite clear.  So I think it should
be fine.

Acked-by: Aaron Conole <aconole@redhat.com>

>>  lib/ipf.c | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/lib/ipf.c b/lib/ipf.c
>> index 2d715f5e9..59e232355 100644
>> --- a/lib/ipf.c
>> +++ b/lib/ipf.c
>> @@ -820,7 +820,7 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
>>               * recommend not setting the mempool number of buffers too low
>>               * and also clamp the number of fragments. */
>>              struct ipf_frag *frag = &ipf_list->frag_list[last_inuse_idx + 1];
>> -            frag->pkt = dp_packet_clone(pkt);
>> +            frag->pkt = pkt;
>>              frag->start_data_byte = start_data_byte;
>>              frag->end_data_byte = end_data_byte;
>>              ipf_list->last_inuse_idx++;
>> @@ -959,7 +959,6 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct dp_packet_batch *pb,
>>                      dp_packet_batch_refill(pb, rp->pkt, pb_idx);
>>                      rp->list->reass_execute_ctx = rp->pkt;
>>                  }
>> -                dp_packet_delete(pkt);
>>              }
>>              ovs_mutex_unlock(&ipf->ipf_lock);
>>          } else {

Yes, all frag will be taken out of the batch into ipf. These packet will eventually have two situations: either they are reassembled successfully and then refilled the batch, or they are deleted in ipf.

We test it on OVS with DPDK, is okay.

This patch has been running in our environment since July and has been working fine.














At 2024-12-16 22:46:37, "Aaron Conole" <aconole@redhat.com> wrote:
>Aaron Conole <aconole@redhat.com> writes:
>
>> fangjiannan <evanfjn@163.com> writes:
>>
>>> From: fang <fangjiannan@cmss.chinamobile.com>
>>>
>>> Canceling packet copying can better improve the performance of
>>> capturing fragmented packets. In 640d4db, pkt copying was added to fix
>>> the crash, but this bug has been fixed in 7e6b41a, so there is no need
>>> to copy the pkt again.
>>>
>>> Signed-off-by: fangjiannan <fangjiannan@cmss.chinamobile.com>
>>> ---
>>
>> I'm not sure about this.  Currently, there are multiple ways for a
>> packet to be deleted during processing, and not all of them will
>> sit inline with the ipf handling, nor will IPF be aware of them.  IPF
>> was changed in 640d4db to ensure that it will always be the only owner
>> of the packet buffer in question.
>>
>> It's been some time since I looked at the out-of-band release points
>> that IPF could still reference a packet on.  Actually, I think they
>> should mostly be eliminated so maybe I'm just carrying FUD from the
>> older IPF design.  Did you test this with DPDK ports?  And with long
>> running CT connections?  Or with frags that would unexpectedly expire?
>> Can you please give some testing details.
>
>I'm going to contradict myself here.  I think it's okay.  We are assumed
>to take ownership in all cases.  If we didn't, there would be a dangling
>uaf as in the case of an unhandled frag we keep it on the batch, and the
>case of reassembled, we refill the batch, and otherwise we will delete.
>In all those cases, the ownership is quite clear.  So I think it should
>be fine.
>
>Acked-by: Aaron Conole <aconole@redhat.com>
>
>>>  lib/ipf.c | 3 +--
>>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/lib/ipf.c b/lib/ipf.c
>>> index 2d715f5e9..59e232355 100644
>>> --- a/lib/ipf.c
>>> +++ b/lib/ipf.c
>>> @@ -820,7 +820,7 @@ ipf_process_frag(struct ipf *ipf, struct ipf_list *ipf_list,
>>>               * recommend not setting the mempool number of buffers too low
>>>               * and also clamp the number of fragments. */
>>>              struct ipf_frag *frag = &ipf_list->frag_list[last_inuse_idx + 1];
>>> -            frag->pkt = dp_packet_clone(pkt);
>>> +            frag->pkt = pkt;
>>>              frag->start_data_byte = start_data_byte;
>>>              frag->end_data_byte = end_data_byte;
>>>              ipf_list->last_inuse_idx++;
>>> @@ -959,7 +959,6 @@ ipf_extract_frags_from_batch(struct ipf *ipf, struct dp_packet_batch *pb,
>>>                      dp_packet_batch_refill(pb, rp->pkt, pb_idx);
>>>                      rp->list->reass_execute_ctx = rp->pkt;
>>>                  }
>>> -                dp_packet_delete(pkt);
>>>              }
>>>              ovs_mutex_unlock(&ipf->ipf_lock);
>>>          } else {

fangjiannan <evanfjn@163.com> writes:

> From: fang <fangjiannan@cmss.chinamobile.com>
>
> Canceling packet copying can better improve the performance of
> capturing fragmented packets. In 640d4db, pkt copying was added to fix
> the crash, but this bug has been fixed in 7e6b41a, so there is no need
> to copy the pkt again.
>
> Signed-off-by: fangjiannan <fangjiannan@cmss.chinamobile.com>
> ---

Thanks - I cleaned up the signoffs and acks, and fixed up the language a
bit in the commit.

Applied!