oktetlabs / test-environment

OKTET Labs Test Environment
Other
5 stars 9 forks source link

doc/cm: make interface ring sizes 64 bit #11

Closed okt-galaktionov closed 1 year ago

okt-galaktionov commented 1 year ago

Some interfaces (representors) return 4294967295, which is a valid value but leads to testing failures since it can't be represented by int32.

Switching to int64 will prevent issues like this and preserve the current "return -1 if not supported" behaviour.

Testing done: Creating representor interfaces no longer leads to failing reads from Configurator. tapi_cfg_if_set_ring_size can still be called successfully.

Note to reviewers: I'm not sure if this is the right way to approach this issue. Alternatively, eth_ring_size_get could be modified to catch this value and return -1. If this is more preferable, what should be done with other values that can be interpreted as negative? I failed to find any information regarding the actual values the kernel can return here. Please advise.

okt-galaktionov commented 1 year ago
  1. I'm trying to run tests that involve creating and using representor interfaces. One of the drivers reports:
    # ethtool -g eth10
    Ring parameters for eth10:
    Pre-set maximums:
    RX:             4294967295
    RX Mini:        n/a
    RX Jumbo:       n/a
    TX:             n/a
    Current hardware settings:
    RX:             64
    RX Mini:        n/a
    RX Jumbo:       n/a
    TX:             n/a

    It's probably a bug in the driver code, but I think it would be ideal if TE didn't break because of that.

  2. Good point. Judging from the code, it should be fine. I'll try to use this function from some test.
okt-galaktionov commented 1 year ago

I've done some more testing: tapi_cfg_if_set_ring_size still finishes successfully

ol-arteman commented 1 year ago

I've done some more testing: tapi_cfg_if_set_ring_size still finishes successfully However, it clearly assumes INT32 type and even more important the same is true for tapi_cfg_if_get_ring_size, so these functions must be updated.

okt-galaktionov commented 1 year ago

I've updated the patch. Functions that deal with ring sizes now use int64_t, some superficial testing shows that these functions still work. Please let me know if I missed anything

okt-galaktionov commented 1 year ago

Just to clarify: has this PR been rejected or queued for the next release?

ol-arteman commented 1 year ago

Yes, the patch is already integrated into the internal repo, so it is to appear in the next release.