riscv-non-isa/riscv-cbqri

Public Review Feedback for CBQRI and Ssqosid

Closed this issue · 2 comments

martinmaas commented

(Posting on behalf of different contributors at Google.)

Please find aggregate feedback on the proposed CBQRI and Ssqosid standards below:

  1. The equation on page 6 is unclear. We think the idea is to create an aggregate, but it is not clear exactly what the resulting bit pattern should be.
  2. Leaving the method in Section 2.1.2. as unspecified seems unsound. How is this unsoundness expected to be addressed?
  3. In Table 1 under Section 2.2, Prefetch is another common AT that could be worth considering. It may also be worth having a dedicated AT for demand misses.
  4. For both capacity and bandwidth monitoring, it would be useful to have a mechanism to snapshot/freeze multiple counters. Consider a large number of tasks in a server and wanting to read the memory bandwidth for all of them. It would be useful for a mechanism to do this without introducing shear by snapshot+read-ing one at a time.
  5. Bandwidth monitoring (and allocation): It would be good to have a bit more clarity on hierarchy. e.g. BW to/from all DDR controllers, BW to/from DDR controllers on another die, BW to/from memory over CXL - how are all these aggregated? Each have its own? There is some undefined way to aggregate? If there is a way to get the data from all of them without shear that would be preferred, and preferred if that is part of the standard.
  6. Separation to MCID and RCID is clean; one item that could be useful on the MCID type is again an aggregate. In particular it could be useful to have an aggregate of multiple MCID without shear - e.g., for all tasks in a group, you want to know the BW for each one, but you also want to know the BW for the entire group. If you are trying to get at that data often enough, shear becomes an issue.
  7. The current standard only specifies the maximum RCID and MCID fields but does not specify how many RCIDs and MCIDs can be effective at the same time. It will be helpful to clarify this.
  8. There needs to be more clarification on program reserved bandwidth blocks (Rbwb). Is the block specified in absolute memory bandwidth value, or has a mapping between a level and an absolute value? If we increase Rbwb, do we have a linear relationship on allocated memory bandwidth?
ved-rivos commented

Thank you for the feedback @martinmaas .

  1. Thanks for pointing out a PDF rendering issue. The equation should have been displayed as
    (RCID << P) | (MCID & ((1 << P) - 1)) see( equation code) however the "&" symbol is somehow not rendered in the PDF. We will fix the rendering issue.

  2. In systems with an IOMMU, the architecturally defined method for associating a QoS ID with a device initiated memory request is specified in chapter 5. The other two methods are for when there is not an IOMMU but there is some implementation specific mechanism to associate a QoS ID with the device requests. This is implementation specific it is unspecified by this specification. We will add this clarification to the section 2.1.2 i.e., the first two methods are applicable when the system does not support an IOMMU.

  3. We will include this enhancement for additional AT encoding for consideration in a follow on extension for adding encoding to differentiate prefetch vs. demand. The base specification defined two commonly useful AT as the prefetch vs. demand differentiation may not be commonly available outside of a hart. The specification has encoding reserved for additional AT encoding.

  4. We will include this request for this enhancement for consideration in a follow on extension. The specification has reserved encoding for additional commands and can be extended by adding a snapshot and a snapshot-read command.

  5. This bandwidth controller may be a shared bandwidth controller for a group of memory controllers or may be a bandwidth controller per memory controller. The enumeration of the hierarchy of the bandwidth controllers is being worked upon as part of the companion ACPI extensions for feature enumeration. This work is in progress in the PRS TG and feedback to enhance the ACPI enumeration is requested. The snapshot enhancement requested in 4 may be useful to address the shear concern. We will add additional guidelines in the SW guidelines chapter to forward this feedback to the PRS task group.

  6. The aggregation across MCIDs may be considered as a follow on extension. This may be served by a snapshot capture enhancement as requested in 4.

  7. All RCID and MCID values supported by the system may be simultaneously active. We will add an explicit statement to section 2 to clarify this.

  8. We will add an expand description of Rbwb to note that there is a linear relationship between the number and the bandwidth allocated. The section 4.1 notes that the bandwidth represented by a bandwidth block is unspecified by this specification. The reason was that the actual bandwidth may depend for example in case of a memory controller on the operating frequency and type of memory device installed. In case of a I/O link it may depend on the link speed negotiated with the peer device (e.g., PCIe Gen 5 vs. Gen 6). However each bandwidth block represents a unit of bandwidth and thus there is a linear relationship between bandwidth and the number of bandwidth blocks. For applications that need to do proportional bandwidth allocation the absolute bandwidth may not be needed. However, where the bandwidth represented by a bandwidth block is required it can be computed by determining the absolute bandwidth provided by the bandwidth controller based on the configuration/state of the resource and dividing that by NBWBLKS.

ved-rivos commented

The updates for 1, 2, 5, 7, and 8 are made in PR #27 .
The following enhancements are noted for consideration in a follow on extension:

  • Additional AT codes to distinguish prefetch vs. demand
  • A snapshot mechanism to sample multiple counters
  • A HW mechanism to aggregate information across multiple controllers

@martinmaas