On 6/11/24 16:02, Aaron Conole wrote:
> Ilya Maximets <i.maxim...@ovn.org> writes:
>
>> On 6/5/24 16:54, Aaron Conole wrote:
>>> Mike Pattrick <m...@redhat.com> writes:
>>>
>>>> When conntrack is reassembling packet fragments, the same reassembly
>>>> context can be shared across multiple threads handling different packets
>>>> simultaneously. Once a full packet is assembled, it is added to a packet
>>>> batch for processing, in the case where there are multiple different pmd
>>>> threads accessing conntrack simultaneously, there is a race condition
>>>> where the reassembled packet may be added to an arbitrary batch even if
>>>> the current batch is available.
>>>>
>>>> When this happens, the packet may be handled incorrectly as it is
>>>> inserted into a random openflow execution pipeline, instead of the
>>>> pipeline for that packets flow.
>>>>
>>>> This change makes a best effort attempt to try to add the defragmented
>>>> packet to the current batch. directly. This should succeed most of the
>>>> time.
>>>>
>>>> Fixes: 4ea96698f667 ("Userspace datapath: Add fragmentation handling.")
>>>> Reported-at: https://issues.redhat.com/browse/FDP-560
>>>> Signed-off-by: Mike Pattrick <m...@redhat.com>
>>>> ---
>>>
>>> The patch overall looks good to me. I'm considering applying with the
>>> following addition:
>>>
>>> diff --git a/utilities/checkpatch.py b/utilities/checkpatch.py
>>> index 6b293770dd..d9b9e0c23f 100755
>>> --- a/utilities/checkpatch.py
>>> +++ b/utilities/checkpatch.py
>>> @@ -63,7 +63,8 @@ def open_spell_check_dict():
>>> 'dhcpv6', 'opts', 'metadata', 'geneve',
>>> 'mutex',
>>> 'netdev', 'netdevs', 'subtable', 'virtio',
>>> 'qos',
>>> 'policer', 'datapath', 'tunctl', 'attr',
>>> 'ethernet',
>>> - 'ether', 'defrag', 'defragment', 'loopback',
>>> 'sflow',
>>> + 'ether', 'defrag', 'defragment',
>>> 'defragmented',
>>> + 'loopback', 'sflow',
>>> 'acl', 'initializer', 'recirc', 'xlated',
>>> 'unclosed',
>>> 'netlink', 'msec', 'usec', 'nsec', 'ms', 'us',
>>> 'ns',
>>> 'kilobits', 'kbps', 'kilobytes', 'megabytes',
>>> 'mbps',
>>>
>>>
>>> unless anyone objects. This is to squelch:
>>>
>>> == Checking 16f6885353c2 ("ipf: Handle common case of ipf
>>> defragmentation.") ==
>>> WARNING: Possible misspelled word: "defragmented"
>>> Did you mean: ['defragment ed', 'defragment-ed', 'defragment']
>>> Lines checked: 129, Warnings: 1, Errors: 0
>>
>> It doesn't affect CI today, so can be a separate patch, I think. We
>> have a few more
>> words like this in relatively recent commits, like 'poller' or
>> 'autovalidator', these
>> can be bundled in that separate commit as well.
>>
>> Though updating the dictionary along with the patch that is using the
>> word sounds OK
>> to me as well.
>
> That makes sense to me.
>
> I've been thinking of adding a spell-check test to the robot. Rather
> than the existing apply check doing the spell checking. The spell
> checker would only ever generate a warning. WDYT?
Sounds fine to me, but we really need to make the checking more robust.
Currently it feeds into the checker too many things that it shouldn't.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
