Traffic Server
  1. Traffic Server
  2. TS-2168

Confusion about proxy.config.allocator.enable_reclaim setting

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 4.1.2
    • Component/s: Configuration
    • Labels:
      None

      Description

      The default records.config in 4.0.1 says the following which is contradictory:

      # The value of enable_reclaim should be 0 or 1. Default 1, reclaim enabled.
      CONFIG proxy.config.allocator.enable_reclaim INT 0
      

      mgmt/RecordsConfig.cc shows the following:

      {RECT_CONFIG, "proxy.config.allocator.enable_reclaim", RECD_INT, "1", RECU_NULL, RR_NULL, RECC_NULL, "[0-1]", RECA_NULL}

      So the default is 1, but records.config changes it to 0.

        Activity

        Hide
        ASF subversion and git services added a comment -

        Commit 1b75d0f0f27276cb2b123cce8e371454f7673e2a in branch refs/heads/master from Leif Hedstrom
        [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=1b75d0f ]

        TS-2168 Make RecordsConfig.cc more inline with default builds.

        Show
        ASF subversion and git services added a comment - Commit 1b75d0f0f27276cb2b123cce8e371454f7673e2a in branch refs/heads/master from Leif Hedstrom [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=1b75d0f ] TS-2168 Make RecordsConfig.cc more inline with default builds.
        Hide
        ASF subversion and git services added a comment -

        Commit 439e3ad07ba7e09c4e8ae2aec9b63ef9aee1ce08 in branch refs/heads/master from Leif Hedstrom
        [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=439e3ad ]

        TS-2168 Make RecordsConfig.cc more inline with default builds.

        Show
        ASF subversion and git services added a comment - Commit 439e3ad07ba7e09c4e8ae2aec9b63ef9aee1ce08 in branch refs/heads/master from Leif Hedstrom [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=439e3ad ] TS-2168 Make RecordsConfig.cc more inline with default builds.
        Hide
        Yunkai Zhang added a comment -

        In ./proxy/config/records.config.default.in:

          # The value of enable_reclaim should be 0 or 1. Default 1, reclaim enabled.
        CONFIG proxy.config.allocator.enable_reclaim INT @use_reclaimable_freelist@
        

        When user enable reclaimable freelist, this fix will cause new and more bad confusion.

        Show
        Yunkai Zhang added a comment - In ./proxy/config/records.config.default.in: # The value of enable_reclaim should be 0 or 1. Default 1, reclaim enabled. CONFIG proxy.config.allocator.enable_reclaim INT @use_reclaimable_freelist@ When user enable reclaimable freelist, this fix will cause new and more bad confusion.
        Hide
        Yunkai Zhang added a comment -

        I submmited a patch to fix inconsistency when enable reclaimable freelist.

        Show
        Yunkai Zhang added a comment - I submmited a patch to fix inconsistency when enable reclaimable freelist.
        Hide
        ASF subversion and git services added a comment -

        Commit 6de8354f20944032fc1ace6559e4bf7bdd1c0186 in branch refs/heads/master from Yunkai Zhang
        [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=6de8354 ]

        TS-2168: Use #if condition to control reclaimable freelist options

        1) Define reclaimabe freelist options only when compiles ATS
        with --enable-reclaimabe-freelist,

        2) "proxy.config.allocator.enable_reclaim" will be set to 1 by default
        for two reasons:
        a) It should keep consistent with the description in
        records.config.default.in: "Default 1, reclaim enabled",
        b) More important, most users want to get reclaim feature when
        they compiles ATS with --enable-reclaimable-freelist.

        3) Make the NOTE description in records.config.default.in more notable.

        Signed-off-by: Yunkai Zhang <qiushu.zyk@taobao.com>

        Show
        ASF subversion and git services added a comment - Commit 6de8354f20944032fc1ace6559e4bf7bdd1c0186 in branch refs/heads/master from Yunkai Zhang [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=6de8354 ] TS-2168 : Use #if condition to control reclaimable freelist options 1) Define reclaimabe freelist options only when compiles ATS with --enable-reclaimabe-freelist, 2) "proxy.config.allocator.enable_reclaim" will be set to 1 by default for two reasons: a) It should keep consistent with the description in records.config.default.in: "Default 1, reclaim enabled", b) More important, most users want to get reclaim feature when they compiles ATS with --enable-reclaimable-freelist. 3) Make the NOTE description in records.config.default.in more notable. Signed-off-by: Yunkai Zhang <qiushu.zyk@taobao.com>
        Hide
        ASF subversion and git services added a comment -

        Commit 03b88fca6fa6e2d5561a78ed055ed247d8001799 in branch refs/heads/master from Leif Hedstrom
        [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=03b88fc ]

        TS-2168 Clean up the verbose comments

        There's not much purpose to repeat the same thing 4 times . This
        also fixes a potential problem with debug_filter, since it currently
        has two bit fields in use, we should allow for values 0-3, right?

        There's still a huge problem with this code as it is now, because
        it would make it impossible to implement features that does
        consistency checks on records.config (see TS-1882).

        Show
        ASF subversion and git services added a comment - Commit 03b88fca6fa6e2d5561a78ed055ed247d8001799 in branch refs/heads/master from Leif Hedstrom [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=03b88fc ] TS-2168 Clean up the verbose comments There's not much purpose to repeat the same thing 4 times . This also fixes a potential problem with debug_filter, since it currently has two bit fields in use, we should allow for values 0-3, right? There's still a huge problem with this code as it is now, because it would make it impossible to implement features that does consistency checks on records.config (see TS-1882 ).
        Hide
        ASF subversion and git services added a comment -

        Commit 9fab65b0136cdf822bbde8fb103d441345ec79ce in branch refs/heads/master from Yunkai Zhang
        [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=9fab65b ]

        TS-2168: set 'enable_reclaim' to 1 as default in records.config

        Signed-off-by: Yunkai Zhang <qiushu.zyk@taobao.com>

        Show
        ASF subversion and git services added a comment - Commit 9fab65b0136cdf822bbde8fb103d441345ec79ce in branch refs/heads/master from Yunkai Zhang [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=9fab65b ] TS-2168 : set 'enable_reclaim' to 1 as default in records.config Signed-off-by: Yunkai Zhang <qiushu.zyk@taobao.com>
        Hide
        Leif Hedstrom added a comment -

        I thought we agreed to return the options in RecordsConfig.cc? Or did that happen in a separate commit?

        Cheers,

        – Leif

        Show
        Leif Hedstrom added a comment - I thought we agreed to return the options in RecordsConfig.cc? Or did that happen in a separate commit? Cheers, – Leif
        Hide
        Yunkai Zhang added a comment -

        Messages from IRC:

        (09:20:39 PM) yunkai_: zwoop: ping ...
        (09:22:39 PM) yunkai_: zwoop: what does your mean about "options in RecordsConfig.cc"?
        (09:24:20 PM) yunkai_: I'd prefer to make reclaimable freelist options in records.config conditional generating.
        (09:24:38 PM) yunkai_: But it's hard to do it, have any good idea?

        Show
        Yunkai Zhang added a comment - Messages from IRC: (09:20:39 PM) yunkai_: zwoop: ping ... (09:22:39 PM) yunkai_: zwoop: what does your mean about "options in RecordsConfig.cc"? (09:24:20 PM) yunkai_: I'd prefer to make reclaimable freelist options in records.config conditional generating. (09:24:38 PM) yunkai_: But it's hard to do it, have any good idea?
        Hide
        Leif Hedstrom added a comment -

        Right, but go back and read the thread . Since we can't make it conditional on records.config, doing it only in RecordsConfig.cc makes it impossible to properly implement the verification features that was requested in a different ticket (which is a great feature for good user experience).

        So, we should keep records.config in sync with RecordsConfig.cc at all time. That was the whole point of this exercise.

        Another option, which I'm sure you won't like, is to remove these configs from records.config. As you know, we don't have everything in there anyways. This would in fact be my personal preference until this feature is always enabled at build time, but I'm not going to argue over this endlessly.

        So, lets just make records.config and RecordsConfig.cc always match up; if a configuration is in records.config, it must always exist in RecordsConfig.cc, with the same value.

        Show
        Leif Hedstrom added a comment - Right, but go back and read the thread . Since we can't make it conditional on records.config, doing it only in RecordsConfig.cc makes it impossible to properly implement the verification features that was requested in a different ticket (which is a great feature for good user experience). So, we should keep records.config in sync with RecordsConfig.cc at all time. That was the whole point of this exercise. Another option, which I'm sure you won't like, is to remove these configs from records.config. As you know, we don't have everything in there anyways. This would in fact be my personal preference until this feature is always enabled at build time, but I'm not going to argue over this endlessly. So, lets just make records.config and RecordsConfig.cc always match up; if a configuration is in records.config, it must always exist in RecordsConfig.cc, with the same value.
        Hide
        Yunkai Zhang added a comment -

        Ok,

        so let me just remove "#if TS_USE_RECLAIMABLE_FREELIST" from RecordsConfig.cc(let the options to be there as before) ?

        Show
        Yunkai Zhang added a comment - Ok, so let me just remove "#if TS_USE_RECLAIMABLE_FREELIST" from RecordsConfig.cc(let the options to be there as before) ?
        Hide
        David Carlin added a comment - - edited

        Why not:

        1) proxy.config.allocator.enable_reclaim = 1 in RecordsConfig.cc
        2) proxy.config.allocator.enable_reclaim = 1 in records.config

        and most important

        3) Put a note in records.config that says "This setting has no effect unless compiled with --enable-reclaimable-freelist"

        If someone compiled with --enable-reclaimable-freelist they are going to want proxy.config.allocator.enable_reclaim = 1 anyways; if they did not compile with --enable-reclaimable-freelist the setting doesn't do anything, so its effectively set to zero.

        When I opened this bug I didn't know you had to use the compile time option --enable-reclaimable-freelist for the setting to do anything. Putting that info in records.config description will help.

        Show
        David Carlin added a comment - - edited Why not: 1) proxy.config.allocator.enable_reclaim = 1 in RecordsConfig.cc 2) proxy.config.allocator.enable_reclaim = 1 in records.config and most important 3) Put a note in records.config that says "This setting has no effect unless compiled with --enable-reclaimable-freelist" If someone compiled with --enable-reclaimable-freelist they are going to want proxy.config.allocator.enable_reclaim = 1 anyways; if they did not compile with --enable-reclaimable-freelist the setting doesn't do anything, so its effectively set to zero. When I opened this bug I didn't know you had to use the compile time option --enable-reclaimable-freelist for the setting to do anything. Putting that info in records.config description will help.
        Hide
        Yunkai Zhang added a comment - - edited

        1) Yes, that is what I mean (make enable_reclaim to 1 in both RecordsConfig.cc and records.config).

        2) I had put a note in records.config for a long time, but you didn't read it carefully:

        ##############################################################################
        #
        # Configuration for Reclaimable InkFreeList memory pool
        #
        # NOTE: The following options are meaningfull only when Traffic Server is
        #       compiled with the following option to configure:
        #
        #           --enable-reclaimable-freelist
        #
        ##############################################################################
        CONFIG proxy.config.allocator.enable_reclaim INT 1
          # The value of reclaim_factor should be in the 0.0 to 1.0 range. Allocators
          # use it to calculate size of unused memory, which is used to determine when
          # to reclaim memory. The larger the value, the more aggressive reclaims.
        CONFIG proxy.config.allocator.reclaim_factor FLOAT 0.300000
          # Allocator will reclaim memory only when it continuously satisfy the reclaim
          # condition for max_overage continuous checks.
        CONFIG proxy.config.allocator.max_overage INT 3
          #  For debugging, enable debug_filter, which is a bit-map with these fields:
          #     bit 0: reclaim memory in ink_freelist_new
          #     bit 1: allocate memory from partial-free Chunks(if exist) or OS
        CONFIG proxy.config.allocator.debug_filter INT 0
        
        Show
        Yunkai Zhang added a comment - - edited 1) Yes, that is what I mean (make enable_reclaim to 1 in both RecordsConfig.cc and records.config). 2) I had put a note in records.config for a long time, but you didn't read it carefully: ############################################################################## # # Configuration for Reclaimable InkFreeList memory pool # # NOTE: The following options are meaningfull only when Traffic Server is # compiled with the following option to configure: # # --enable-reclaimable-freelist # ############################################################################## CONFIG proxy.config.allocator.enable_reclaim INT 1 # The value of reclaim_factor should be in the 0.0 to 1.0 range. Allocators # use it to calculate size of unused memory, which is used to determine when # to reclaim memory. The larger the value, the more aggressive reclaims. CONFIG proxy.config.allocator.reclaim_factor FLOAT 0.300000 # Allocator will reclaim memory only when it continuously satisfy the reclaim # condition for max_overage continuous checks. CONFIG proxy.config.allocator.max_overage INT 3 # For debugging, enable debug_filter, which is a bit-map with these fields: # bit 0: reclaim memory in ink_freelist_new # bit 1: allocate memory from partial-free Chunks( if exist) or OS CONFIG proxy.config.allocator.debug_filter INT 0
        Hide
        Leif Hedstrom added a comment -

        That's basically what we are doing I think the comments are already there.

        Show
        Leif Hedstrom added a comment - That's basically what we are doing I think the comments are already there.
        Hide
        David Carlin added a comment - - edited

        Ah, you're right.. I didn't actually read the full records.config my apologies. What prompted me to write this bug was that I diff'd the records.config from 3.3.5 and 4.0.1 and I saw the following:

        -  # The value of enable_reclaim should be 0 or 1. Default 0, reclaim disabled.
        +  # The value of enable_reclaim should be 0 or 1. Default 1, reclaim enabled.
         CONFIG proxy.config.allocator.enable_reclaim INT 0
        

        So I was confused why someone changed the comment and not the setting. I didn't actually read the records.config, just the diff -u output and missed the big warning about --enable-reclaimable-freelist

        Show
        David Carlin added a comment - - edited Ah, you're right.. I didn't actually read the full records.config my apologies. What prompted me to write this bug was that I diff'd the records.config from 3.3.5 and 4.0.1 and I saw the following: - # The value of enable_reclaim should be 0 or 1. Default 0, reclaim disabled. + # The value of enable_reclaim should be 0 or 1. Default 1, reclaim enabled. CONFIG proxy.config.allocator.enable_reclaim INT 0 So I was confused why someone changed the comment and not the setting. I didn't actually read the records.config, just the diff -u output and missed the big warning about --enable-reclaimable-freelist
        Hide
        Yunkai Zhang added a comment -

        There was slight typo in the old code. Leif has adjusted the options' position, so that users can easily notice the comments.

        Show
        Yunkai Zhang added a comment - There was slight typo in the old code . Leif has adjusted the options' position, so that users can easily notice the comments.
        Hide
        ASF subversion and git services added a comment -

        Commit 2c194735ff40ef4272002ff9dde0c3c977d04009 in branch refs/heads/master from Yunkai Zhang
        [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=2c19473 ]

        TS-2168: keep records.config in sync with RecordsConfig.cc at all time

        According a long discussion on TS-2168 ticket:

        "Since we can't make it conditional on records.config, doing it only in
        RecordsConfig.cc makes it impossible to properly implement the verification
        features that was requested in a different ticket" – Leif

        Let's remove #if TS_USE_RECLAIMABLE_FREELIST to keep records.config in
        sync with RecordsConfig.cc at all time.

        Signed-off-by: Yunkai Zhang <qiushu.zyk@taobao.com>

        Show
        ASF subversion and git services added a comment - Commit 2c194735ff40ef4272002ff9dde0c3c977d04009 in branch refs/heads/master from Yunkai Zhang [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=2c19473 ] TS-2168 : keep records.config in sync with RecordsConfig.cc at all time According a long discussion on TS-2168 ticket: "Since we can't make it conditional on records.config, doing it only in RecordsConfig.cc makes it impossible to properly implement the verification features that was requested in a different ticket" – Leif Let's remove #if TS_USE_RECLAIMABLE_FREELIST to keep records.config in sync with RecordsConfig.cc at all time. Signed-off-by: Yunkai Zhang <qiushu.zyk@taobao.com>
        Hide
        ASF subversion and git services added a comment -

        Commit 2c194735ff40ef4272002ff9dde0c3c977d04009 in branch refs/heads/master from Yunkai Zhang
        [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=2c19473 ]

        TS-2168: keep records.config in sync with RecordsConfig.cc at all time

        According a long discussion on TS-2168 ticket:

        "Since we can't make it conditional on records.config, doing it only in
        RecordsConfig.cc makes it impossible to properly implement the verification
        features that was requested in a different ticket" – Leif

        Let's remove #if TS_USE_RECLAIMABLE_FREELIST to keep records.config in
        sync with RecordsConfig.cc at all time.

        Signed-off-by: Yunkai Zhang <qiushu.zyk@taobao.com>

        Show
        ASF subversion and git services added a comment - Commit 2c194735ff40ef4272002ff9dde0c3c977d04009 in branch refs/heads/master from Yunkai Zhang [ https://git-wip-us.apache.org/repos/asf?p=trafficserver.git;h=2c19473 ] TS-2168 : keep records.config in sync with RecordsConfig.cc at all time According a long discussion on TS-2168 ticket: "Since we can't make it conditional on records.config, doing it only in RecordsConfig.cc makes it impossible to properly implement the verification features that was requested in a different ticket" – Leif Let's remove #if TS_USE_RECLAIMABLE_FREELIST to keep records.config in sync with RecordsConfig.cc at all time. Signed-off-by: Yunkai Zhang <qiushu.zyk@taobao.com>

          People

          • Assignee:
            Leif Hedstrom
            Reporter:
            David Carlin
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development