diff for duplicates of <20171218220759.GF4094@dastard> diff --git a/a/1.txt b/N1/1.txt index 5c97265..c03c1fb 100644 --- a/a/1.txt +++ b/N1/1.txt @@ -7,97 +7,3 @@ requires automatically? I suspect this should be using atomic_read_acquire() and atomic_cmpxchg_release(), because AFAICT the atomic_cmpxchg needs to have release semantics to match the acquire semantics needed for the load of the current value. - ->From include/linux/atomics.h: - - * For compound atomics performing both a load and a store, ACQUIRE - * semantics apply only to the load and RELEASE semantics only to the - * store portion of the operation. Note that a failed cmpxchg_acquire - * does -not- imply any memory ordering constraints. - -Memory barriers hurt my brain. :/ - -At minimum, shouldn't the atomic op specific barriers be used rather -than full memory barriers? i.e: - -> diff --git a/include/linux/iversion.h b/include/linux/iversion.h -> index a9fbf99709df..39ec9aa9e08e 100644 -> --- a/include/linux/iversion.h -> +++ b/include/linux/iversion.h -> @@ -87,6 +87,25 @@ static inline void -> inode_set_iversion_raw(struct inode *inode, const u64 val) -> { -> atomic64_set(&inode->i_version, val); -> + smp_wmb(); - - smp_mb__after_atomic(); -..... -> +static inline u64 -> +inode_peek_iversion_raw(const struct inode *inode) -> +{ -> + smp_rmb(); - - smp_mb__before_atomic(); - -> + return atomic64_read(&inode->i_version); -> } - -And, of course, these will require comments explaining what they -match with and what they are ordering. - -> @@ -152,7 +171,7 @@ inode_maybe_inc_iversion(struct inode *inode, bool force) -> { -> u64 cur, old, new; -> -> - cur = (u64)atomic64_read(&inode->i_version); -> + cur = inode_peek_iversion_raw(inode); -> for (;;) { -> /* If flag is clear then we needn't do anything */ -> if (!force && !(cur & I_VERSION_QUERIED)) - -cmpxchg in this loop needs a release barrier so everyone will -see the change? - -> @@ -183,23 +202,6 @@ inode_inc_iversion(struct inode *inode) -> inode_maybe_inc_iversion(inode, true); -> } -> -> -/** -> - * inode_peek_iversion_raw - grab a "raw" iversion value -> - * @inode: inode from which i_version should be read -> - * -> - * Grab a "raw" inode->i_version value and return it. The i_version is not -> - * flagged or converted in any way. This is mostly used to access a self-managed -> - * i_version. -> - * -> - * With those filesystems, we want to treat the i_version as an entirely -> - * opaque value. -> - */ -> -static inline u64 -> -inode_peek_iversion_raw(const struct inode *inode) -> -{ -> - return atomic64_read(&inode->i_version); -> -} -> - -> /** -> * inode_iversion_need_inc - is the i_version in need of being incremented? -> * @inode: inode to check -> @@ -248,7 +250,7 @@ inode_query_iversion(struct inode *inode) -> { -> u64 cur, old, new; -> -> - cur = atomic64_read(&inode->i_version); -> + cur = inode_peek_iversion_raw(inode); -> for (;;) { -> /* If flag is already set, then no need to swap */ -> if (cur & I_VERSION_QUERIED) - -cmpxchg in this loop needs a release barrier so everyone will -see the change on load? - -Cheers, - -Dave. --- -Dave Chinner -david@fromorbit.com diff --git a/a/content_digest b/N1/content_digest index 0b82422..2ef1523 100644 --- a/a/content_digest +++ b/N1/content_digest @@ -41,100 +41,6 @@ "requires automatically? I suspect this should be using\n" "atomic_read_acquire() and atomic_cmpxchg_release(), because AFAICT\n" "the atomic_cmpxchg needs to have release semantics to match the\n" - "acquire semantics needed for the load of the current value.\n" - "\n" - ">From include/linux/atomics.h:\n" - "\n" - " * For compound atomics performing both a load and a store, ACQUIRE\n" - " * semantics apply only to the load and RELEASE semantics only to the\n" - " * store portion of the operation. Note that a failed cmpxchg_acquire\n" - " * does -not- imply any memory ordering constraints.\n" - "\n" - "Memory barriers hurt my brain. :/\n" - "\n" - "At minimum, shouldn't the atomic op specific barriers be used rather\n" - "than full memory barriers? i.e:\n" - "\n" - "> diff --git a/include/linux/iversion.h b/include/linux/iversion.h\n" - "> index a9fbf99709df..39ec9aa9e08e 100644\n" - "> --- a/include/linux/iversion.h\n" - "> +++ b/include/linux/iversion.h\n" - "> @@ -87,6 +87,25 @@ static inline void\n" - "> inode_set_iversion_raw(struct inode *inode, const u64 val)\n" - "> {\n" - "> \tatomic64_set(&inode->i_version, val);\n" - "> +\tsmp_wmb();\n" - "\n" - "\tsmp_mb__after_atomic();\n" - ".....\n" - "> +static inline u64\n" - "> +inode_peek_iversion_raw(const struct inode *inode)\n" - "> +{\n" - "> +\tsmp_rmb();\n" - "\n" - "\tsmp_mb__before_atomic();\n" - "\n" - "> +\treturn atomic64_read(&inode->i_version);\n" - "> }\n" - "\n" - "And, of course, these will require comments explaining what they\n" - "match with and what they are ordering.\n" - "\n" - "> @@ -152,7 +171,7 @@ inode_maybe_inc_iversion(struct inode *inode, bool force)\n" - "> {\n" - "> \tu64 cur, old, new;\n" - "> \n" - "> -\tcur = (u64)atomic64_read(&inode->i_version);\n" - "> +\tcur = inode_peek_iversion_raw(inode);\n" - "> \tfor (;;) {\n" - "> \t\t/* If flag is clear then we needn't do anything */\n" - "> \t\tif (!force && !(cur & I_VERSION_QUERIED))\n" - "\n" - "cmpxchg in this loop needs a release barrier so everyone will\n" - "see the change?\n" - "\n" - "> @@ -183,23 +202,6 @@ inode_inc_iversion(struct inode *inode)\n" - "> \tinode_maybe_inc_iversion(inode, true);\n" - "> }\n" - "> \n" - "> -/**\n" - "> - * inode_peek_iversion_raw - grab a \"raw\" iversion value\n" - "> - * @inode: inode from which i_version should be read\n" - "> - *\n" - "> - * Grab a \"raw\" inode->i_version value and return it. The i_version is not\n" - "> - * flagged or converted in any way. This is mostly used to access a self-managed\n" - "> - * i_version.\n" - "> - *\n" - "> - * With those filesystems, we want to treat the i_version as an entirely\n" - "> - * opaque value.\n" - "> - */\n" - "> -static inline u64\n" - "> -inode_peek_iversion_raw(const struct inode *inode)\n" - "> -{\n" - "> -\treturn atomic64_read(&inode->i_version);\n" - "> -}\n" - "> -\n" - "> /**\n" - "> * inode_iversion_need_inc - is the i_version in need of being incremented?\n" - "> * @inode: inode to check\n" - "> @@ -248,7 +250,7 @@ inode_query_iversion(struct inode *inode)\n" - "> {\n" - "> \tu64 cur, old, new;\n" - "> \n" - "> -\tcur = atomic64_read(&inode->i_version);\n" - "> +\tcur = inode_peek_iversion_raw(inode);\n" - "> \tfor (;;) {\n" - "> \t\t/* If flag is already set, then no need to swap */\n" - "> \t\tif (cur & I_VERSION_QUERIED)\n" - "\n" - "cmpxchg in this loop needs a release barrier so everyone will\n" - "see the change on load?\n" - "\n" - "Cheers,\n" - "\n" - "Dave.\n" - "-- \n" - "Dave Chinner\n" - david@fromorbit.com + acquire semantics needed for the load of the current value. -01bc4a73f7ddd2138fced7154ab7dfb287466c196fc22a37762d1ca42aa911a2 +b49381c40398477ebf22a77fdd2f54bd4e4e2be7498d67960f884a126929954d
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.