On 13/10/2023 15:17, Arnaldo Carvalho de Melo wrote: > Em Thu, Oct 12, 2023 at 01:35:41PM +0100, Alan Maguire escreveu: >> On 11/10/2023 23:14, Eduard Zingerman wrote: >>> On Wed, 2023-10-11 at 23:05 +0100, Alan Maguire wrote: >>> [...] >>>>>>> I'm not sure I understand the logic behind "skip" features. >>>>>>> Take `decl_tag` for example: >>>>>>> - by default conf_load->skip_encoding_btf_decl_tag is 0; >>>>>>> - if `--btf_features=decl_tag` is passed it is still 0 because of the >>>>>>> `skip ? false : true` logic. >>>>>>> >>>>>>> If there is no way to change "skip" features why listing these at all? >>>>>>> >>>>>> You're right; in the case of a skip feature, I think we need the >>>>>> following behaviour >>>>>> >>>>>> 1. we skip the encoding by default (so the equivalent of >>>>>> --skip_encoding_btf_decl_tag, setting skip_encoding_btf_decl_tag >>>>>> to true >>>>>> >>>>>> 2. if the user however specifies the logical inversion of the skip >>>>>> feature in --btf_features (in this case "decl_tag" - or "all") >>>>>> skip_encoding_btf_decl_tag is set to false. >>>>>> >>>>>> So in my code we had 2 above but not 1. If both were in place I think >>>>>> we'd have the right set of behaviours. Does that sound right? >>>>> >>>>> You mean when --features=? is specified we default to >>>>> conf_load->skip_encoding_btf_decl_tag = true, and set it to false only >>>>> if "all" or "decl_tag" is listed in features, right? >>>>> >>>> >>>> Yep. Here's the comment I was thinking of adding for the next version, >>>> hopefully it clarifies this all a bit more than the original. >>>> >>>> +/* --btf_features=feature1[,feature2,..] allows us to specify >>>> + * a list of requested BTF features or "all" to enable all features. >>>> + * These are translated into the appropriate conf_load values via >>>> + * struct btf_feature which specifies the associated conf_load >>>> + * boolean field and whether its default (representing the feature being >>>> + * off) is false or true. >>>> + * >>>> + * btf_features is for opting _into_ features so for a case like >>>> + * conf_load->btf_gen_floats, the translation is simple; the presence >>>> + * of the "float" feature in --btf_features sets conf_load->btf_gen_floats >>>> + * to true. >>>> + * >>>> + * The more confusing case is for features that are enabled unless >>>> + * skipping them is specified; for example >>>> + * conf_load->skip_encoding_btf_type_tag. By default - to support >>>> + * the opt-in model of only enabling features the user asks for - >>>> + * conf_load->skip_encoding_btf_type_tag is set to true (meaning no >>>> + * type_tags) and it is only set to false if --btf_features contains >>>> + * the "type_tag" feature. >>>> + * >>>> + * So from the user perspective, all features specified via >>>> + * --btf_features are enabled, and if a feature is not specified, >>>> + * it is disabled. >>>> */ >>> >>> Sounds reasonable. Maybe also add a line saying that >>> skip_encoding_btf_decl_tag defaults to false if --btf_features is not >>> specified to remain backwards compatible? >>> >> >> good idea, will do! Thanks! > > I have to catch up with all the comments on this thread, but does this > mean you're respinning the series now? > Yep, I'm respinning now. There were a few small things in Andrii's suggestions that we might need to figure out - mostly around names - but there's enough change since the RFC that it would probably be best to discuss those with the v2 series. Thanks! Alan > - Arnaldo >
