On Apr 19, Simon Horman wrote:
> On Tue, Apr 18, 2023 at 12:16:28PM -0400, Ihar Hrachyshka wrote:
> > Acked-By: Ihar Hrachyshka <ihrac...@redhat.com>
> >
> > On Tue, Apr 18, 2023 at 10:14 AM Lorenzo Bianconi
> > <lorenzo.bianc...@redhat.com> wrote:
> > >
> > > Rework OVN QoS implementation in order to configure it through OVS QoS
> > > table instead of running tc command directly bypassing OVS.
> > >
> > > https://bugzilla.redhat.com/show_bug.cgi?id=2129742
> > > Tested-by: Rodolfo Alonso <ralon...@redhat.com>
> > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
>
> ...
>
> Hi Lorenzo,
>
> I am fine with this patch, modulo the issue below.
Hi Simon,
ack, will fix it.
Regards,
Lorenzo
>
> Reviewed-by: Simon Horman <simon.hor...@corigine.com>
>
>
> > > +/* 34359738360 == (2^32 - 1) * 8. netdev_set_qos() doesn't support
> > > + * 64-bit rate netlink attributes, so the maximum value is 2^32 - 1
> > > + * bytes. The 'max-rate' config option is in bits, so multiplying by 8.
> > > + * Without setting max-rate the reported link speed will be used, which
> > > + * can be unrecognized for certain NICs or reported too low for virtual
> > > + * interfaces. */
> > > +#define OVN_QOS_MAX_RATE 34359738360
>
> Maybe 34359738360ULL to make the type explicit, because...
>
> > > static void
> > > -setup_qos(const char *egress_iface, struct hmap *queue_map)
> > > +add_ovs_qos_table_entry(struct ovsdb_idl_txn *ovs_idl_txn,
> > > + struct shash *bridge_mappings,
> > > + struct qos_queue *q)
> > > {
> > > - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> > > - struct netdev *netdev_phy;
> > > + const struct ovsrec_interface *iface;
> > > + const struct ovsrec_port *port;
> > >
> > > - if (!egress_iface) {
> > > - /* Queues cannot be configured. */
> > > + iface = get_qos_egress_port_interface(bridge_mappings, &port,
> > > q->network);
> > > + if (!iface) {
> > > return;
> > > }
> > >
> > > - int error = netdev_open(egress_iface, NULL, &netdev_phy);
> > > - if (error) {
> > > - VLOG_WARN_RL(&rl, "%s: could not open netdev (%s)",
> > > - egress_iface, ovs_strerror(error));
> > > - return;
> > > + struct smap other_config = SMAP_INITIALIZER(&other_config);
> > > + const struct ovsrec_qos *qos = port->qos;
> > > + if (!qos) {
> > > + qos = ovsrec_qos_insert(ovs_idl_txn);
> > > + ovsrec_qos_set_type(qos, OVN_QOS_TYPE);
> > > + ovsrec_port_set_qos(port, qos);
> > > + smap_add_format(&other_config, "max-rate", "%ld",
> > > OVN_QOS_MAX_RATE);
>
> ... GCC complains about the use of %ld because OVN_QOS_MAX_RATE is too wide.
>
>
> controller/binding.c: In function ‘add_ovs_qos_table_entry’:
> controller/binding.c:233:55: error: format ‘%ld’ expects argument of type
> ‘long int’, but argument 4 has type ‘long long int’ [-Werror=format=]
> 233 | smap_add_format(&other_config, "max-rate", "%ld",
> OVN_QOS_MAX_RATE);
> | ~~^
> | |
> | long int
> | %lld
>
> Link:
> https://github.com/ovsrobot/ovn/actions/runs/4733678125/jobs/8401543448#step:14:3458
>
> Which I think means that the CI/CD also failed for all subsequent
> patches in the series :(
>
>
> > > + ovsrec_qos_set_other_config(qos, &other_config);
> > > + smap_clear(&other_config);
> > > }
>
> ...
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
