Re: [Xen-devel] [RFC][PATCH 01/13] tools: introduce some new parameters to set rdm policy

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, May 19, 2015 at 11:16:33AM +0800, Chen, Tiejun wrote:
> 
> On 2015/5/19 3:17, Wei Liu wrote:

[...]

> >>     rdm = [ 'host, reserve=<POLICY>' ]
> >>
> >
> >OK, so this is a specific example in vtd.txt. Last time I misread it as
> >part of the manpage.
> >
> >I think you meant in this specific example (with other suggestions
> >incorporated):
> >
> >      rdm = "type=host, reserve=force"
> >
> >Then you point user to xl.cfg manpage.
> >
> >>Or just for a specific device:
> >>
> >>     pci = [ '01:00.0,rdm_reserve=<POLICY>', '03:00.0' ]
> >>
> >
> >Same here.
> >
> 
> In order to be compatible with vtd.txt, could this work for you?
> 
> To enable this globally, add "rdm" in the config file:
> 
>     rdm = [ 'type=host, reserve=<policy>' ]     (default policy is
> "relaxed")
> 
> Or just for a specific device:
> 
>     pci = [ '01:00.0,rdm_reserve=<policy>' ]    (default policy is "strict")
> 

In my last reply I meant to ask you to have specific example in vtd.txt
and point user to xl.cfg manpage (see below). Previously I thought the
examples you proposed were for manpages, that's why I had asked you to
"list all values of that option". But now this is in vtd.txt, I would
just write something like:


    rdm = "type=host, reserve=relaxed"   (default policy is "relaxed")

    pci = [ '01:00.0,rdm_reserve=try', '03:00.0,reserve=strict' ]

    For all the options available to RDM, see xl.cfg(5).

And of course you would need to list all values of these options in
xl.cfg(5), as I suggested in my first reply to this patch.

> 
> >Just don't write "force/try" or "strcit/relax" because that's not the
> >exact syntax you would use in a real config file.
> 
> Yeah.
> 
> >

[...]

> >>>>>want you want?
> >>>>
> >>>>We have a little bit of complexity here,
> >>>>
> >>>>"Default per-device RDM policy is 'force', while default global RDM policy
> >>>>is 'try'. When both policies are specified on a given region, 'force' is
> >>>>always preferred."
> >>>>
> >>>
> >>>This is going to be done in actual code anyway.
> >>>
> >>>This type is used both in global and per-device setting, so I envisage
> >>
> >>Yes.
> >>
> >>>this to have an invalid value to start with. Appropriate default values
> >>
> >>Sounds I should set this,
> >>
> >>+libxl_rdm_reserve_flag = Enumeration("rdm_reserve_flag", [
> >>+    (-1, "invalid"),
> >>+    (0, "strict"),
> >>+    (1, "relaxed"),
> >>+    ], init_val = "LIBXL_RDM_RESERVE_FLAG_INVALID")
> >>+
> >>
> >

Yet another question about this feature. The current setup suggests that
we must choose a policy, either "strict" or "relaxed", i.e. there is no
way to disable this feature, as in there is no "none" policy to skip
checking rdm conflict.

AIUI this feature is more like a bug fix to existing problem, so we
always want to enable it. And the default relaxed policy makes sure we
don't break guest that was working before this feature. Do I understand
this correctly?

If we risk breaking existing guests, we might want to consider adding a
"none" (name subject to improvement) policy to just skip RDM all
together.

> >Yes, and then don't forget to set the value to appropriate value in the
> >_setdefault functions for different types.
> 
> Currently "type" is not associated to "policy" so we can do this if
> necessary in the future.
> 
> >
> >>
> >>>should be done in libxl_TYPE_setdefault functions. And the logic to
> >>>detect conflict and preferences done in your construct function.
> >>>
> >>>What do you think?
> >>>
> >>>>>
> >>>>>>+
> >
> >[...]
> >
> >>>>>>                      pcidev->permissive = atoi(tok);
> >>>>>>                  }else if ( !strcmp(optkey, "seize") ) {
> >>>>>>                      pcidev->seize = atoi(tok);
> >>>>>>+                }else if ( !strcmp(optkey, "rdm_reserve") ) {
> >>>>>>+                    if ( !strcmp(tok, "force") ) {
> >>>>>>+                        pcidev->rdm_reserve = 
> >>>>>>LIBXL_RDM_RESERVE_FLAG_FORCE;
> >>>>>>+                    } else if ( !strcmp(tok, "try") ) {
> >>>>>>+                        pcidev->rdm_reserve = 
> >>>>>>LIBXL_RDM_RESERVE_FLAG_TRY;
> >>>>>>+                    } else {
> >>>>>>+                        pcidev->rdm_reserve = 
> >>>>>>LIBXL_RDM_RESERVE_FLAG_FORCE;
> >>>>>>+                        XLU__PCI_ERR(cfg, "Unknown PCI RDM property 
> >>>>>>flag value:"
> >>>>>>+                                          " %s, so goes 'force' by 
> >>>>>>default.",
> >>>>>
> >>>>>If this is not an error, you don't need XLU__PCI_ERR.
> >>>>>
> >>>>>But I would say we should  just treat this as an error and
> >>>>>abort/exit/report (whatever the parser should do in this case).
> >>>>
> >>>>In our case we just want to post a message to set a appropriate flag to
> >>>>recover this behavior like we write here,
> >>>>
> >>>>                         XLU__PCI_ERR(cfg, "Unknown PCI RDM property flag
> >>>>value:"
> >>>>                                           " %s, so goes 'strict' by
> >>>>default.",
> >>>>                                      tok);
> >>>
> >>>I suggest we just abort in this case and not second guess what the admin
> >>>wants.
> >>
> >>Okay,
> >>                     } else {
> >>                         XLU__PCI_ERR(cfg, "%s is not an valid PCI RDM
> >>property"
> >>                                           " flag: 'strict' or 'relaxed'.",
> >>                                      tok);
> >>                         abort();
> >>
> >
> >No, not calling the "abort" function. I meant returning appropriate error
> >value and let the caller handles this situation.
> 
> Okay, just call "goto parse_error".
> 

Yes, that would work.

> >
> >>
> >>>
> >>>>
> >>>>This may just be a warning? But I don't we have this sort of definition,
> >>>>XLU__PCI_WARN, ...
> >>>>
> >>>>So what LOG format can be adopted here?
> >>>
> >>>Feel free to introduce XLU__PCI_WARN if it turns out to be necessary.
> >>
> >>If it goes to abort(), I think XLU__PCI_ERR() should be good.
> >>
> >>>
> >>>>
> >>>>>
> >>>>>>+                                     tok);
> >>>>>>+                    }
> >>>>>>                  }else{
> >>>>>>                      XLU__PCI_ERR(cfg, "Unknown PCI BDF option: %s", 
> >>>>>> optkey);
> >>>>>>                  }
> >>>>>>@@ -167,6 +180,71 @@ parse_error:
> >>>>>>      return ERROR_INVAL;
> >>>>>>  }
> >>>>>>
> >>>>>>+int xlu_rdm_parse(XLU_Config *cfg, libxl_rdm_reserve *rdm, const char 
> >>>>>>*str)
> >>>>>>+{
> >>>>>>+    unsigned state = STATE_TYPE;
> >>>>>>+    char *buf2, *tok, *ptr, *end;
> >>>>>>+
> >>>>>>+    if ( NULL == (buf2 = ptr = strdup(str)) )
> >>>>>>+        return ERROR_NOMEM;
> >>>>>>+
> >>>>>>+    for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
> >>>>>>+        switch(state) {
> >>>
> >>>Coding style. I haven't checked what actual style this file uses, but
> >>>there is inconsistency in this function by itself.
> >>
> >>I just refer to xlu_pci_parse_bdf() to generate xlu_rdm_parse(), and they
> >>are in the same file...
> >>
> >>Anyway, I should change this line,
> >>
> >>for ( tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++ ) {
> >>
> >
> >   for (tok = ptr, end...)
> >
> >   switch (state) {
> 
> But what is the difference to compare the initial code?
> 

Spaces.

> >>>>>> +    if ( NULL == (buf2 = ptr = strdup(str)) )
> >>>>>> +        return ERROR_NOMEM;
> >>>>>> +
> >>>>>> +    for(tok = ptr, end = ptr + strlen(ptr) + 1; ptr < end; ptr++) {
> >>>>>> +        switch(state) {
> 
> I thought initially you let me to follow that previous "if" :)
> 

Just be consistent with other part of the source code.

> >
> >
> >>>
> >>>>>>+        case STATE_TYPE:
> >>>>>>+            if ( *ptr == '\0' || *ptr == ',' ) {
> >>>>>>+                state = STATE_CHECK_FLAG;
> >>>>>>+                *ptr = '\0';
> >

[...]

> >                  [("hvm", Struct(None, [("firmware",         string),
> >                                         ("bios",             
> > libxl_bios_type),
> >@@ -521,6 +538,7 @@ libxl_device_pci = Struct("device_pci", [
> >      ("power_mgmt", bool),
> >      ("permissive", bool),
> >      ("seize", bool),
> >+    ("rdm_reserve",   libxl_rdm_reserve_flag),
> >      ])
> >
> >Do you actually need libxl_rdm_reserve type? I.e. do you envisage that
> >structure to change a lot? Can you not just use libxl_rdm_reserve_type
> >and libxl_rdm_reserve_flag in build_info.
> >
> 
> We'd like to introduce this type, libxl_rdm_reserve, to combine "type" and
> "flag". From my point of view, this sole structure can represent a holistic
> approach to rdm because,
> 
> #1. Obviously its easy to get all;
> #2. It will probably be extended since like this name, rdm, reserved device
> memory, this should not be restricted to RMRR currently. So I just feel its
> flexible to support others in the future, or much more properties :)
> 

Fair enough.

Wei.

> Thanks
> Tiejun

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel