On 11/23/22 15:32, Lorenzo Bianconi wrote:
> On Nov 23, Dumitru Ceara wrote:
>> On 11/23/22 15:26, Lorenzo Bianconi wrote:
>>>>>>> /* Called with in the pinctrl_handler thread context. */
>>>>>>> static int
>>>>>>> pinctrl_handle_buffered_packets(struct dp_packet *pkt_in,
>>>>>>> const struct match *md, bool is_arp)
>>>>>>> OVS_REQUIRES(pinctrl_mutex)
>>>>>>> {
>>>>>>> - struct buffered_packets *bp;
>>>>>>> - struct dp_packet *clone;
>>>>>>> - struct in6_addr addr;
>>>>>>> -
>>>>>>> - if (is_arp) {
>>>>>>> - addr = in6_addr_mapped_ipv4(htonl(md->flow.regs[0]));
>>>>>>> - } else {
>>>>>>> - ovs_be128 ip6 = hton128(flow_get_xxreg(&md->flow, 0));
>>>>>>> - memcpy(&addr, &ip6, sizeof addr);
>>>>>>> - }
>>>>>>> -
>>>>>>> - uint32_t hash = hash_bytes(&addr, sizeof addr, 0);
>>>>>>> - bp = pinctrl_find_buffered_packets(&addr, hash);
>>>>>>> + uint64_t dp_key = ntohll(md->flow.metadata);
>>>>>>> + uint64_t oport_key = md->flow.regs[MFF_LOG_OUTPORT - MFF_REG0];
>>>>>>> + uint32_t hash = pinctrl_buffer_apcket_hash(dp_key, oport_key);
>>>>>>> + struct buffered_packets *bp
>>>>>>> + = pinctrl_find_buffered_packets(dp_key, oport_key, hash);
>>>>>>> if (!bp) {
>>>>>>> if (hmap_count(&buffered_packets_map) >= 1000) {
>>>> Before your patch we would hit this hard coded limit if there were 1000
>>>> next hops for which we were bufferring packets.
>>>>
>>>> Now, with your change, we will hit his limit only if there are 1000
>>>> ports for which we are bufferring packets. This seems very unlikely and
>>>> I wonder if this will ever happen in practice. Do we need to change its
>>>> value? Also, let's add a define for it somewhere to make it a bit less
>>>> "magic".
>>> ack. I would prefer to be a bit more conservative and keep the condition.
>>> What is the right value to use up to you? 😄
>>>
>>
>> Well, if you want to keep the exact same condition you'd have to count
>> how many individual next hops we're buffering packets for. Is that an
>> option?
>
> I am fine to just keep a condition that limits the max size of
> buffered_packets_map, but if you think that is really not important we can
> drop
> it.
>
I think I wasn't clear, sorry. I think we still need that condition to
protect ovn-controller. We just need to adapt the code a bit so that we
stop buffering packets if we're already buffering for a max (1000)
number of next hops. This is what we had before your patch.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
