> -----Original Message-----
> From: David Marchand <[email protected]>
> Sent: Thursday, December 2, 2021 4:05 PM
> To: Van Haaren, Harry <[email protected]>
> Cc: [email protected]; [email protected]; Stokes, Ian
> <[email protected]>; Amber, Kumar <[email protected]>
> Subject: Re: [PATCH v3] dpif-netdev: Call cpuid for x86 isa availability.
>
> On Tue, Nov 30, 2021 at 5:53 PM Van Haaren, Harry
> <[email protected]> wrote:
> > > Resolve isa availability in constructors by using a simplified query
> > > based on cpuid API that comes from the compiler.
> >
> > Using constructors instead of an init() time call is interesting, but may
> > not be
> what we
> > always want. For "vswitchd" it is a useful startup feature, however other
> binaries/tools
> > such as "ovs-vsctl" or "ovs-appctl" do not require CPUID-based ISA
> > detection at
> all.
> > As per this patch, every launch of "ovs-vsctl" (or other tooling/binaries)
> > will
> cause the
> > constructors to run.
> >
> > I would like to add some VLOG_* info/logging to the CPU ISA detection, it
> > may
> be useful
> > to understand the system if in future debug of CPU ISA implementations is
> required.
> > (This is how the constructor-running was identified, lots of printf() at
> > tooling
> startup!)
>
> I can look at adding logs in dpif as a preparation patch.
> The current situation where we have logs at init is not user friendly:
> for a user that wants to use feature X and enters the right ovs-*ctl
> commands, it is hard to make the relation with "cpu has feature Y" in
> OVS logs that could be days old.
Good point. The previously proposed solution to only run CPUID instruction and
decode
available ISA handles that very nicely. Suggestion in pseudo-code below:
static int cpuid_check_done;
void do_cpuid_check() {
/* execute cpuid, run through all OVS-known ISA, VLOG() and store results.
*/
cpuid_check_done = 1;
}
int check_isa( isa_type ) {
if (OVS_UNLIKELY( ! cpuid_check_done )) {
do_cpuid_check()
}
return isa_available[isa_type];
}
Benefits;
1) When user executes command to use ISA, ISA checks are executed, VLOG
executes too
2) OVS-appctl (etc) which do not call "check_isa()" will not run CPUID, and not
VLOG either.
3) Lack of constructor functions eases portability (DPDK & Windows has issues
with constructors IIUC)
> Moving this code in some init function will add an init order dependency.
No order dependency required, see the above. First to call will do the required
work.
> This detection code is really small, stateless and can be done as
> early as possible.
> Otoh, with constructors, components in OVS can get them without caring
> if init was run (let's say some day we need to know about those
> features in utils).
>
>
> > > +
> > > +enum x86_reg {
> > > + EAX,
> > > + EBX,
> > > + ECX,
> > > + EDX,
> > > +};
> > > +#define X86_LEAF_MASK 0x80000000
> > > +#define X86_EXT_FEATURES_LEAF 0x00000007
> > > +static bool x86_has_isa(uint32_t leaf, enum x86_reg reg, uint32_t bit)
> > > +{
> > > + uint32_t maxleaf = __get_cpuid_max(leaf & X86_LEAF_MASK, NULL);
> > > + uint32_t regs[4];
> > > +
> > > + if (maxleaf < leaf) {
> > > + return false;
> >
> > This is a programming error, not a runtime error correct? We're asking for a
> > leaf that has not been supported in OVS. Presumably the programmer
> intended
> > to ask for a feature that OVS has support for. So a unique/identifiable
> > error
> return
>
> A ovs_assert() is better in this case.
Yes agree.
> --
> David Marchand
Thanks, -Harry
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
