On 10/9/21 1:05 AM, Ihar Hrachyshka wrote:
> This is correct, thank you for fixing it.
>
> One comment and one question:
>
> 1) While the referred patch that suppressed LOCAL_ONLY traffic exposed
> the issue, it was always a bug, though affecting a tiny minority of
> scenarios.
>
> For example for PD, one can probably imagine a scenario where PD server
> is not located in a provider network but is hosted on a VIF that is
> attached to a "public" logical switch (no localnet ports!). This VIF may
> be located on a different chassis, in which case, even without the
> suppressing patch, PD requests wouldn't reach PD server. On the other
> hand, they would reach the server if its VIF would happen to land on the
> same chassis. A similar scenario may be envisioned for BFD, where router
> ports are distributed across multiple chassis.
I agree, the "Fixes" tag looks wrong. I think it should be:
Fixes: e3a398e9146e ("controller: Add ipv6 prefix delegation state machine")
Fixes: 365a8a696afb ("bfd: support demand mode on rx side.")
>
> 2) I see you covered all pinctrl handlers that were setting the
> LOCAL_ONLY bit via MLF_LOCAL_ONLY_BIT macro. I've noticed that there are
> several other handlers that use MLF_LOCAL_ONLY macro instead, with (as
> far as I can see) exactly the same effect (any idea why we have both?).
> An example of that would be e.g. ip_mcast_querier_send_igmp. Do we
> believe that other packet injections are meant to stay inside the same
> chassis, or is it just something you missed / left out of scope for this
> patch?
The other use cases are, AFAICT:
- IGMP/MLD querier
- service monitor
The multicast queriers use per logical switch source mac address, and
are generated by ovn-controller on each hypervisor where the logical
switch has bound ports. As far as I see it's similar for service
monitor. So using MLF_LOCAL_ONLY seems correct to me.
>
> Thanks again,
>
> Ihar
Regards,
Dumitru
>
> On 10/8/21 10:54 AM, Lorenzo Bianconi wrote:
>> Do not mark BFD and IPv6 Prefix Delegation messages as local only since
>> they will be sent through a localnet port.
>>
>> Fixes: 14c6dac51dc8 ("Suppress LOCAL_ONLY traffic for localnet ports")
>> Signed-off-by: Lorenzo Bianconi <[email protected]>
>> ---
>> controller/pinctrl.c | 2 --
>> 1 file changed, 2 deletions(-)
>>
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index cc3edaaf4..7d01b18a3 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -1108,7 +1108,6 @@ ipv6_prefixd_send(struct rconn *swconn, struct
>> ipv6_prefixd_state *pfd)
>> uint32_t port_key = pfd->port_key;
>> put_load(dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
>> put_load(port_key, MFF_LOG_INPORT, 0, 32, &ofpacts);
>> - put_load(1, MFF_LOG_FLAGS, MLF_LOCAL_ONLY_BIT, 1, &ofpacts);
>> struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
>> resubmit->in_port = OFPP_CONTROLLER;
>> resubmit->table_id = OFTABLE_LOG_INGRESS_PIPELINE;
>> @@ -6815,7 +6814,6 @@ pinctrl_send_bfd_tx_msg(struct rconn *swconn,
>> struct bfd_entry *entry,
>> uint32_t port_key = entry->port_key;
>> put_load(dp_key, MFF_LOG_DATAPATH, 0, 64, &ofpacts);
>> put_load(port_key, MFF_LOG_INPORT, 0, 32, &ofpacts);
>> - put_load(1, MFF_LOG_FLAGS, MLF_LOCAL_ONLY_BIT, 1, &ofpacts);
>> struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
>> resubmit->in_port = OFPP_CONTROLLER;
>> resubmit->table_id = OFTABLE_LOG_INGRESS_PIPELINE;
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
