Hi Kevin,
Responses inline.
> > > +static struct user_mempool_config *user_mempools = NULL; static int
> > > +n_user_mempools;
> >
> > Should we consider restricting n_user_mempools to some max value ?
> >
>
> We could do. There is no issue with having many values, it just might not
> make much sense for a user.
>
> Is your concern that someone might maliciously keep adding values?
Yes, this was my concern.
> I'm not sure if there is some protection already within the smap_get that
> would prevent this. i'd need to check that.
> If there is, then that would be covered already. Otherwise we could add
> some arbitrary max number like 100.
Sure, sounds good.
Otherwise, the patch looks good. I'll be happy to ack!
>
> > >
> > > /* There should be one 'struct dpdk_tx_queue' created for @@ -573,4
> > > +582,42 @@ dpdk_buf_size(int mtu) }
> > >
> > > +static int
> > > +dpdk_get_user_adjusted_mtu(int port_adj_mtu, int port_mtu, int
> > > +port_socket_id) {
> > > + int best_adj_user_mtu = INT_MAX;
> > > +
> > > + for (unsigned i = 0; i < n_user_mempools; i++) {
> > > + int user_adj_mtu, user_socket_id;
> > > +
> > > + user_adj_mtu = user_mempools[i].adj_mtu;
> > > + user_socket_id = user_mempools[i].socket_id;
> > > + if (port_adj_mtu > user_adj_mtu
> > > + || (user_socket_id != INT_MAX
> > > + && user_socket_id != port_socket_id)) {
> > > + continue;
> > > + }
> > > + if (user_adj_mtu < best_adj_user_mtu) {
> > > + /* This is the is the lowest valid user MTU. */
> > > + best_adj_user_mtu = user_adj_mtu;
> > > + if (best_adj_user_mtu == port_adj_mtu) {
> > > + /* Found an exact fit, no need to keep searching. */
> > > + break;
> > > + }
> > > + }
> >
> >
> > I guess this is more appropriate than sorting etc to find lowest valid
> user MTU since we assume n_user_mempools will be quite small ?
> >
>
> It is relying on MTU *and* socket id (which may be wildcard/any NUMA), so
> we couldn't sort it as a flat list.
> There would need to be multiple lists created for every numa etc
>
> There is no need to go to that trouble to speed up the look up when we
> expect a few entries and we only look this up on the control path when we
> are changing MTU. So I think it's fine the way it is.
>
> Kevin.
I agree with your assessment Kevin 😊
>
> > > + }
> > > + if (best_adj_user_mtu == INT_MAX) {
> > > + VLOG_DBG("No user configured shared mempool mbuf sizes found
> "
> > > + "suitable for port with MTU %d, NUMA %d.", port_mtu,
> > > + port_socket_id);
> > > + best_adj_user_mtu = port_adj_mtu;
> > > + } else {
> > > + VLOG_DBG("Found user configured shared mempool with mbufs "
> > > + "of size %d, suitable for port with MTU %d, NUMA
> %d.",
> > > + MTU_TO_FRAME_LEN(best_adj_user_mtu), port_mtu,
> > > + port_socket_id);
> > > + }
> > > + return best_adj_user_mtu;
> > > +}
> > > +
> >
> > <snipped>
> >
> > Thanks and regards,
> > Sunil
> > _______________________________________________
> > 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
